| Message ID | 20120827170310.4a0b6c8c@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 27 Aug 2012 17:03:10 +0300 Pekka Paalanen <ppaalanen@gmail.com> wrote: > Hi Rob, > > I've started reviewing your patch and fixing the remaining bugs. So far > I think I got most of the blend/opaque region stuff sorted out. I > haven't still gotten to the geometry, where I can trigger visual bugs > with and without getting your TODO printouts. > > I'm guessing the xwayland surfaces will come and haunt us, because I > think they contain both an opaque region with undefined alpha values, > and opaque and non-opaque regions with valid alpha values.I haven't > even tested them yet, but I guess a quick fix would be to paint opaque > regions always with a shader that forces texture alpha to 1.0. Would it > better as a different shader program or a uniform flag for a shader, I > don't know. > > I'm not sure we should have nested functions in Weston code base. Also > like you noted, compositor_wayland.c does not build anymore. > > Here's my WIP tree that may be rebased! The tree is still here and updated: http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders I don't think I will be rebasing it anymore. I merged Rob's updates there. Rob, I will leave cleaning up the series for you, once we are done. You probably want to squash some things etc. > I should get to reviewing the geometry tomorrow. I started on geometry, and wrote a simple debug feature: http://people.collabora.com/~pq/geometry-debug-2.png http://people.collabora.com/~pq/geometry-debug-3.png http://people.collabora.com/~pq/geometry-debug-4.png It draws the triangle edges. It's quite fun in realtime, apart from leaving a mess on the screen. When I was reading through the x1==x2 and y1==y2 special cases in calculate_edges(), I had the feeling the polygon winding order might get wrong sometimes. Also looking at it on paper I was certain I should be getting those TODO messages. Turned out the (!es->transform.enabled) special case was hiding all that. When forcing the transformed path, there's obviously something not quite right: http://people.collabora.com/~pq/geometry-debug-5.png That is in my git branch, too. There is also room for a simplification: as the intersection of two rectangles is guaranteed to be convex, we don't need the "center point" vertex, we can simply use any vertex on the perimeter. This should make the usual case of an aligned rectangle to collapse into two triangles instead of the current four, and work fine for all cases. And I still haven't gotten to the complex parts yet :-P Cheers! - pq
On Tue, Aug 28, 2012 at 8:27 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Mon, 27 Aug 2012 17:03:10 +0300 > Pekka Paalanen <ppaalanen@gmail.com> wrote: > >> Hi Rob, >> >> I've started reviewing your patch and fixing the remaining bugs. So far >> I think I got most of the blend/opaque region stuff sorted out. I >> haven't still gotten to the geometry, where I can trigger visual bugs >> with and without getting your TODO printouts. >> >> I'm guessing the xwayland surfaces will come and haunt us, because I >> think they contain both an opaque region with undefined alpha values, >> and opaque and non-opaque regions with valid alpha values.I haven't >> even tested them yet, but I guess a quick fix would be to paint opaque >> regions always with a shader that forces texture alpha to 1.0. Would it >> better as a different shader program or a uniform flag for a shader, I >> don't know. >> >> I'm not sure we should have nested functions in Weston code base. Also >> like you noted, compositor_wayland.c does not build anymore. >> >> Here's my WIP tree that may be rebased! > > The tree is still here and updated: > http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders > I don't think I will be rebasing it anymore. > > I merged Rob's updates there. > > Rob, I will leave cleaning up the series for you, once we are done. You > probably want to squash some things etc. Fyi, I've squashed/reordered a bit. And pushed some nice updates to your debug draw stuff, so now it is quite fun to look at: git://github.com/robclark/weston.git master BR, -R >> I should get to reviewing the geometry tomorrow. > > I started on geometry, and wrote a simple debug feature: > http://people.collabora.com/~pq/geometry-debug-2.png > http://people.collabora.com/~pq/geometry-debug-3.png > http://people.collabora.com/~pq/geometry-debug-4.png > > It draws the triangle edges. It's quite fun in realtime, apart from > leaving a mess on the screen. > > When I was reading through the x1==x2 and y1==y2 special cases in > calculate_edges(), I had the feeling the polygon winding order might > get wrong sometimes. Also looking at it on paper I was certain I should > be getting those TODO messages. > > Turned out the (!es->transform.enabled) special case was hiding all > that. When forcing the transformed path, there's obviously something > not quite right: > http://people.collabora.com/~pq/geometry-debug-5.png > > That is in my git branch, too. > > There is also room for a simplification: as the intersection of two > rectangles is guaranteed to be convex, we don't need the "center point" > vertex, we can simply use any vertex on the perimeter. This should make > the usual case of an aligned rectangle to collapse into two triangles > instead of the current four, and work fine for all cases. > > And I still haven't gotten to the complex parts yet :-P > > > Cheers! > - pq > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Wed, 29 Aug 2012 19:18:17 -0500 Rob Clark <rob.clark@linaro.org> wrote: > On Tue, Aug 28, 2012 at 8:27 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Mon, 27 Aug 2012 17:03:10 +0300 > > Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > >> Hi Rob, > >> > >> I've started reviewing your patch and fixing the remaining bugs. So far > >> I think I got most of the blend/opaque region stuff sorted out. I > >> haven't still gotten to the geometry, where I can trigger visual bugs > >> with and without getting your TODO printouts. > >> > >> I'm guessing the xwayland surfaces will come and haunt us, because I > >> think they contain both an opaque region with undefined alpha values, > >> and opaque and non-opaque regions with valid alpha values.I haven't > >> even tested them yet, but I guess a quick fix would be to paint opaque > >> regions always with a shader that forces texture alpha to 1.0. Would it > >> better as a different shader program or a uniform flag for a shader, I > >> don't know. > >> > >> I'm not sure we should have nested functions in Weston code base. Also > >> like you noted, compositor_wayland.c does not build anymore. > >> > >> Here's my WIP tree that may be rebased! > > > > The tree is still here and updated: > > http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders > > I don't think I will be rebasing it anymore. > > > > I merged Rob's updates there. > > > > Rob, I will leave cleaning up the series for you, once we are done. You > > probably want to squash some things etc. > > Fyi, I've squashed/reordered a bit. And pushed some nice updates to > your debug draw stuff, so now it is quite fun to look at: > > git://github.com/robclark/weston.git master Hi Rob, that's definitely interesting. I'm working based on your 'pq' branch, though, because you missed these commits: 1e17e4a compositor: fix blending for full-surface alpha 87af9c8 compositor: specialised fragment shader for RGBX 0c37514 compositor: re-enable full-surface alpha 82ffb58 compositor: fix build against WL_bind_wayland_display extension and it seems the 'pq' branch is up to date still. I got these with your 'pq' branch: http://people.collabora.com/~pq/geometry-debug-6.png It appears the clean-up painting paints something wrong, but I haven't been able to reproduce that otherwise. The flower is rotated about 45 degrees. Notice the three green pixels down from the word "Thu" in the panel clock. Those pixels flicker when I move the mouse cursor, even though there should not be damage anywhere near. The green pixels are where the surface's top-left corner is. http://people.collabora.com/~pq/geometry-debug-7.png This shows a problem with screen-aligned surfaces. The center vertex is not in the center, implying that there are duplicate vertices. Good thing I didn't remove the center vertex yet. If I disable the fan debug line clean-up, it is hard to spot or trigger any problems, apart from the center point thing. Thanks, pq
On Thu, 30 Aug 2012 14:11:41 +0300 Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Wed, 29 Aug 2012 19:18:17 -0500 > Rob Clark <rob.clark@linaro.org> wrote: > > > On Tue, Aug 28, 2012 at 8:27 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > On Mon, 27 Aug 2012 17:03:10 +0300 > > > Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > >> Hi Rob, > > >> > > >> I've started reviewing your patch and fixing the remaining bugs. So far > > >> I think I got most of the blend/opaque region stuff sorted out. I > > >> haven't still gotten to the geometry, where I can trigger visual bugs > > >> with and without getting your TODO printouts. > > >> > > >> I'm guessing the xwayland surfaces will come and haunt us, because I > > >> think they contain both an opaque region with undefined alpha values, > > >> and opaque and non-opaque regions with valid alpha values.I haven't > > >> even tested them yet, but I guess a quick fix would be to paint opaque > > >> regions always with a shader that forces texture alpha to 1.0. Would it > > >> better as a different shader program or a uniform flag for a shader, I > > >> don't know. > > >> > > >> I'm not sure we should have nested functions in Weston code base. Also > > >> like you noted, compositor_wayland.c does not build anymore. > > >> > > >> Here's my WIP tree that may be rebased! > > > > > > The tree is still here and updated: > > > http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders > > > I don't think I will be rebasing it anymore. > > > > > > I merged Rob's updates there. > > > > > > Rob, I will leave cleaning up the series for you, once we are done. You > > > probably want to squash some things etc. > > > > Fyi, I've squashed/reordered a bit. And pushed some nice updates to > > your debug draw stuff, so now it is quite fun to look at: > > > > git://github.com/robclark/weston.git master > > Hi Rob, > > that's definitely interesting. I'm working based on your 'pq' branch, > though, because you missed these commits: > > 1e17e4a compositor: fix blending for full-surface alpha > 87af9c8 compositor: specialised fragment shader for RGBX > 0c37514 compositor: re-enable full-surface alpha > 82ffb58 compositor: fix build against WL_bind_wayland_display extension > > and it seems the 'pq' branch is up to date still. > > I got these with your 'pq' branch: > > http://people.collabora.com/~pq/geometry-debug-6.png > It appears the clean-up painting paints something wrong, but I haven't > been able to reproduce that otherwise. The flower is rotated about 45 > degrees. Notice the three green pixels down from the word "Thu" in the > panel clock. Those pixels flicker when I move the mouse cursor, even > though there should not be damage anywhere near. The green pixels are > where the surface's top-left corner is. > > http://people.collabora.com/~pq/geometry-debug-7.png > This shows a problem with screen-aligned surfaces. The center vertex is > not in the center, implying that there are duplicate vertices. Good > thing I didn't remove the center vertex yet. > > If I disable the fan debug line clean-up, it is hard to spot or trigger > any problems, apart from the center point thing. It's so complex piece, that I decided to write a manual unit test. You can find a new app called cliptest in http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders It looks like this: http://people.collabora.com/~pq/geometry-debug-8.png Blue is the clip rectangle, red is the surface, and the green line is what vertices calculate_edges() produces. I plan to add the following controls: - arrow keys to move clip box - some other keys to rotate the surface box - print vertex indices next to them, so we spot duplicates Hmm, and we need box size controls, too. Anyway, that should make it easier to create reproducible test cases. The damage regions in weston tend to change all the time, and we cannot really affect the rects in a region in a sane way. Maybe with cairo we could actually produce automatic tests. Thanks, pq
On Thu, Aug 30, 2012 at 6:11 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Wed, 29 Aug 2012 19:18:17 -0500 > Rob Clark <rob.clark@linaro.org> wrote: > >> On Tue, Aug 28, 2012 at 8:27 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: >> > On Mon, 27 Aug 2012 17:03:10 +0300 >> > Pekka Paalanen <ppaalanen@gmail.com> wrote: >> > >> >> Hi Rob, >> >> >> >> I've started reviewing your patch and fixing the remaining bugs. So far >> >> I think I got most of the blend/opaque region stuff sorted out. I >> >> haven't still gotten to the geometry, where I can trigger visual bugs >> >> with and without getting your TODO printouts. >> >> >> >> I'm guessing the xwayland surfaces will come and haunt us, because I >> >> think they contain both an opaque region with undefined alpha values, >> >> and opaque and non-opaque regions with valid alpha values.I haven't >> >> even tested them yet, but I guess a quick fix would be to paint opaque >> >> regions always with a shader that forces texture alpha to 1.0. Would it >> >> better as a different shader program or a uniform flag for a shader, I >> >> don't know. >> >> >> >> I'm not sure we should have nested functions in Weston code base. Also >> >> like you noted, compositor_wayland.c does not build anymore. >> >> >> >> Here's my WIP tree that may be rebased! >> > >> > The tree is still here and updated: >> > http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders >> > I don't think I will be rebasing it anymore. >> > >> > I merged Rob's updates there. >> > >> > Rob, I will leave cleaning up the series for you, once we are done. You >> > probably want to squash some things etc. >> >> Fyi, I've squashed/reordered a bit. And pushed some nice updates to >> your debug draw stuff, so now it is quite fun to look at: >> >> git://github.com/robclark/weston.git master > > Hi Rob, > > that's definitely interesting. I'm working based on your 'pq' branch, > though, because you missed these commits: > > 1e17e4a compositor: fix blending for full-surface alpha > 87af9c8 compositor: specialised fragment shader for RGBX > 0c37514 compositor: re-enable full-surface alpha > 82ffb58 compositor: fix build against WL_bind_wayland_display extension oh woops, I quickly pushed 'em to my master branch.. would be a good idea to double check that there is nothing missing. > and it seems the 'pq' branch is up to date still. > > I got these with your 'pq' branch: > > http://people.collabora.com/~pq/geometry-debug-6.png > It appears the clean-up painting paints something wrong, but I haven't > been able to reproduce that otherwise. The flower is rotated about 45 > degrees. Notice the three green pixels down from the word "Thu" in the > panel clock. Those pixels flicker when I move the mouse cursor, even > though there should not be damage anywhere near. The green pixels are > where the surface's top-left corner is. three green pixels sound like some garbage left on one of the flipchain buffers.. I guess that itself is just from the debug lines, although the flower itself looks a bit messed up. > http://people.collabora.com/~pq/geometry-debug-7.png > This shows a problem with screen-aligned surfaces. The center vertex is > not in the center, implying that there are duplicate vertices. Good > thing I didn't remove the center vertex yet. hmm, yeah, this stuff is kinda relying on the duplicate vertex check in append_vertex().. if (x1 == x2) { append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); } else if (y1 == y2) { append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); } else { but I guess maybe we end up w/ a duplicate vertex which isn't the last vertex.. so this part is probably still not right.. BR, -R > If I disable the fan debug line clean-up, it is hard to spot or trigger > any problems, apart from the center point thing. > > > Thanks, > pq > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Thu, 30 Aug 2012 12:15:52 -0500 Rob Clark <rob.clark@linaro.org> wrote: > On Thu, Aug 30, 2012 at 6:11 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > Hi Rob, > > > > that's definitely interesting. I'm working based on your 'pq' branch, > > though, because you missed these commits: > > > > 1e17e4a compositor: fix blending for full-surface alpha > > 87af9c8 compositor: specialised fragment shader for RGBX > > 0c37514 compositor: re-enable full-surface alpha > > 82ffb58 compositor: fix build against WL_bind_wayland_display extension > > oh woops, I quickly pushed 'em to my master branch.. would be a good > idea to double check that there is nothing missing. Yup, I can't see anything really missing in the diff. > > and it seems the 'pq' branch is up to date still. > > > > I got these with your 'pq' branch: > > > > http://people.collabora.com/~pq/geometry-debug-6.png > > It appears the clean-up painting paints something wrong, but I haven't > > been able to reproduce that otherwise. The flower is rotated about 45 > > degrees. Notice the three green pixels down from the word "Thu" in the > > panel clock. Those pixels flicker when I move the mouse cursor, even > > though there should not be damage anywhere near. The green pixels are > > where the surface's top-left corner is. > > three green pixels sound like some garbage left on one of the > flipchain buffers.. I guess that itself is just from the debug lines, > although the flower itself looks a bit messed up. No, they flash and are very consistent, and follow the flower, even when there is no real triangle reaching that point, when I stop moving. I suspect there is a tiny bunch of at least three vertices. I will try to reproduce it with the cliptest program, or least verify with prints if there are extra vertices. > > http://people.collabora.com/~pq/geometry-debug-7.png > > This shows a problem with screen-aligned surfaces. The center vertex is > > not in the center, implying that there are duplicate vertices. Good > > thing I didn't remove the center vertex yet. > > hmm, yeah, this stuff is kinda relying on the duplicate vertex check > in append_vertex().. > > if (x1 == x2) { > append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); > append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); > } else if (y1 == y2) { > append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); > append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); > } else { > > but I guess maybe we end up w/ a duplicate vertex which isn't the last > vertex.. so this part is probably still not right.. Yeah, I'll try to reproduce problems with cliptest, and get you screenshots. Thanks, pq
On Fri, 31 Aug 2012 12:27:38 +0300 Pekka Paalanen <ppaalanen@gmail.com> wrote: > On Thu, 30 Aug 2012 12:15:52 -0500 > Rob Clark <rob.clark@linaro.org> wrote: > > > On Thu, Aug 30, 2012 at 6:11 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > I got these with your 'pq' branch: > > > > > > http://people.collabora.com/~pq/geometry-debug-6.png > > > It appears the clean-up painting paints something wrong, but I haven't > > > been able to reproduce that otherwise. The flower is rotated about 45 > > > degrees. Notice the three green pixels down from the word "Thu" in the > > > panel clock. Those pixels flicker when I move the mouse cursor, even > > > though there should not be damage anywhere near. The green pixels are > > > where the surface's top-left corner is. > > > > three green pixels sound like some garbage left on one of the > > flipchain buffers.. I guess that itself is just from the debug lines, > > although the flower itself looks a bit messed up. > > No, they flash and are very consistent, and follow the flower, even > when there is no real triangle reaching that point, when I stop > moving. I suspect there is a tiny bunch of at least three vertices. > I will try to reproduce it with the cliptest program, or least verify > with prints if there are extra vertices. I couldn't find the problem in cliptest, but I printed the vertices, and this looks suspicious: region verts: (56.9, 44.9) (56.9, 44.7) (57.0, 44.8) (57.0, 45.0) (56.7, 45.0) (56.9, 44.7) Those are what emit_vertex() was called with for a single texture_region() call. The coordinates fit well to the three pixel flashing garbage I see, and nothing else was near those coords. I now realize I should've printed out a little differently to be more useful. > > > http://people.collabora.com/~pq/geometry-debug-7.png > > > This shows a problem with screen-aligned surfaces. The center vertex is > > > not in the center, implying that there are duplicate vertices. Good > > > thing I didn't remove the center vertex yet. > > > > hmm, yeah, this stuff is kinda relying on the duplicate vertex check > > in append_vertex().. > > > > if (x1 == x2) { > > append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); > > append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); > > } else if (y1 == y2) { > > append_vertex(clip(x1, cx1, cx2), clip(y1, cy1, cy2)); > > append_vertex(clip(x2, cx1, cx2), clip(y2, cy1, cy2)); > > } else { > > > > but I guess maybe we end up w/ a duplicate vertex which isn't the last > > vertex.. so this part is probably still not right.. > > Yeah, I'll try to reproduce problems with cliptest, and get you > screenshots. I couldn't find exactly duplicated vertices in cliptest, but I found problems: http://people.collabora.com/~pq/geometry-test-1.png http://people.collabora.com/~pq/geometry-test-2.png http://people.collabora.com/~pq/geometry-test-3.png http://people.collabora.com/~pq/geometry-test-4.png http://people.collabora.com/~pq/geometry-test-5.png http://people.collabora.com/~pq/geometry-test-6.png Later I added surface orientation mark (the red dot) and vertex indices: http://people.collabora.com/~pq/geometry-test-7.png This one shows a correct result. The latest cliptest is at http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=shaders and hopefully the copy of calculate_edges() is still up-to-date. Cliptest is controlled by mouse left and right drag, and wheel. HTH, pq
On Fri, 31 Aug 2012 16:58:24 +0300 Pekka Paalanen <ppaalanen@gmail.com> wrote: > I couldn't find exactly duplicated vertices in cliptest, but I found > problems: > > http://people.collabora.com/~pq/geometry-test-1.png > http://people.collabora.com/~pq/geometry-test-2.png > http://people.collabora.com/~pq/geometry-test-3.png > http://people.collabora.com/~pq/geometry-test-4.png > http://people.collabora.com/~pq/geometry-test-5.png > http://people.collabora.com/~pq/geometry-test-6.png > > Later I added surface orientation mark (the red dot) and vertex > indices: > http://people.collabora.com/~pq/geometry-test-7.png > This one shows a correct result. An improved version if cliptest is at: http://cgit.collabora.com/git/user/pq/wayland-demos.git/log/?h=cliptest New features: keyboard controls and proper use of weston_surface::transform.enabled, see http://cgit.collabora.com/git/user/pq/wayland-demos.git/tree/clients/cliptest.c?h=cliptest#n24 The surface orientation dot is now bright, when the surface is not transformed. Thanks, pq