On Fri, 3 Feb 2017 16:10:38 +0100 Emilio Pozuelo Monfort <poch...@gmail.com> wrote:
> This reproduces https://bugs.freedesktop.org/show_bug.cgi?id=94735. > > The test currently fails, so mark it as expected to fail. > > Signed-off-by: Emilio Pozuelo Monfort <emilio.pozu...@collabora.co.uk> Hi, there is a problem with this test that the conditions are not quite right for the bug we want to reproduce. Because of that, you also need to re-evaluate the fix (the following patch). I suspect the fix needs to happen somewhere else, but we'll see only after the test conditions are right. > --- > tests/reference/subsurface_mapped-00.png | Bin 0 -> 799 bytes > tests/reference/subsurface_mapped-01.png | Bin 0 -> 841 bytes > tests/subsurface-shot-test.c | 79 > +++++++++++++++++++++++++++++++ > 3 files changed, 79 insertions(+) > create mode 100644 tests/reference/subsurface_mapped-00.png > create mode 100644 tests/reference/subsurface_mapped-01.png > > diff --git a/tests/reference/subsurface_mapped-00.png > b/tests/reference/subsurface_mapped-00.png > new file mode 100644 > index > 0000000000000000000000000000000000000000..32cf32a0ed11d38b9028eda29109e06c99dce000 > GIT binary patch > literal 799 > zcmeAS@N?(olHy`uVBq!ia0y~yU~~YoKX5Ps$$$P@Hb9Ck$=lt9;Xep2*t>i(0|V0) > zPZ!6KiaBpDJMtb-U^sBV(DcSZzE>X)w43Y<zTkg6<~Q@Kf7clg*fKlMXjD9NKp-uF > u!`PrjQsNK~Pa<2J!Km<Pm<+lE!M8n->GIc&-r>L$&*16m=d#Wzp$P!YDfm(V > > literal 0 > HcmV?d00001 > > diff --git a/tests/reference/subsurface_mapped-01.png > b/tests/reference/subsurface_mapped-01.png > new file mode 100644 > index > 0000000000000000000000000000000000000000..4e7196a2229a2e63adbf49d32a8c47db043f1c1c > GIT binary patch > literal 841 > zcmeAS@N?(olHy`uVBq!ia0y~yU~~YoKX5Ps$$$P@Hb9Ck$=lt9;Xep2*t>i(0|V1P > zPZ!6KiaBquZsa}Wz`){YUu2kf{Oyc6Wg6{8K0QxPoXkGz!#aIUUe52048Hw0nHzqy > z@FX&|88AA}Xi)SyAfT4OA#BjXDRF32cth#8hDqmtn^qrc-8g5ndd!V)&);y?A7max > zA$7y5+Tmoxxtqcd+SvzD9B$b7_V^p#1HN)}PDa0(vIRL#J}^{z1<V@^p00i_>zopr > E02fd6egFUf > > literal 0 > HcmV?d00001 > > diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c > index df72ec28..e7da1e0e 100644 > --- a/tests/subsurface-shot-test.c > +++ b/tests/subsurface-shot-test.c > @@ -260,3 +260,82 @@ TEST(subsurface_z_order) > if (bufs[i]) > buffer_destroy(bufs[i]); > } > + > +FAIL_TEST(subsurface_mapped) > +{ > + const char *test_name = get_test_name(); > + struct client *client; > + struct wl_surface *surface; > + struct rectangle clip; > + int fail = 0; > + struct wl_subcompositor *subco; > + struct wl_surface *child; > + struct wl_subsurface *sub; > + struct buffer *buf, *child_buf; > + pixman_color_t red, blue; > + > + color(&red, 255, 0, 0); > + color(&blue, 0, 0, 255); > + > + /* Create the client */ > + client = create_client(); > + assert(client); > + client->surface = create_test_surface(client); > + surface = client->surface->wl_surface; > + > + /* move the pointer clearly away from our screenshooting area */ > + weston_test_move_pointer(client->test->weston_test, 2, 30); > + > + client->surface->x = 100; > + client->surface->y = 100; > + weston_test_move_surface(client->test->weston_test, > client->surface->wl_surface, > + client->surface->x, client->surface->y); Quite subtle behaviour here, because of of the test plugin works. weston-test.c:test_surface_committed() marks both the view and the surface mapped regardless whether there is content. A contentless surface can become mapped if the weston_surface::committed hook is triggered by viewport changes or an attach with a NULL buffer. Whether that is a bug or a feature of the test plugin, I'm not sure. It certainly allows a surface to become mapped without content which is kind of not expected.(*) > + > + /* we need one of these so that buffer_viewport.changed gets set and > the commit > + * has an effect */ > + wl_surface_set_buffer_scale(surface, 1); > + wl_surface_set_buffer_transform(surface, WL_OUTPUT_TRANSFORM_NORMAL); > + > + wl_surface_commit(surface); Here we deliberately trigger that effect, so now we have a mapped surface without content. > + > + clip.x = 100; > + clip.y = 100; > + clip.width = 100; > + clip.height = 100; > + printf("Clip: %d,%d %d x %d\n", clip.x, clip.y, clip.width, > clip.height); > + > + /* create a wl_surface */ > + subco = get_subcompositor(client); > + assert(subco); > + > + child = wl_compositor_create_surface(client->wl_compositor); > + assert(child); > + > + /* make it a sub-surface */ > + sub = wl_subcompositor_get_subsurface(subco, child, surface); > + assert(sub); > + > + /* let the subsurface be drawn async from its parent */ > + wl_subsurface_set_desync(sub); > + > + /* give it content */ > + child_buf = surface_commit_color(client, child, &red, 50, 50); > + > + /* verify that the sub-surface is not mapped */ > + fail += check_screen(client, test_name, 0, &clip, 0); That causes a dilemma here. The test plugin clearly marks the surface as mapped, but here we assume it is not mapped and test behaviour based on that. I would argue that the test here is wrong by deliberately invoking a mapped surface, then checking it's not mapped. > + > + /* give a buffer to the parent surface and make sure both the > + * parent and the child get mapped */ > + buf = surface_commit_color(client, surface, &blue, 100, 100); > + > + fail += check_screen(client, test_name, 1, &clip, 1); This part is correct. > + > + printf("Test complete\n"); > + assert(fail == 0); > + > + wl_subsurface_destroy(sub); > + wl_surface_destroy(child); > + buffer_destroy(child_buf); > + wl_surface_destroy(surface); > + buffer_destroy(buf); > +} Reading again the log from https://bugs.freedesktop.org/show_bug.cgi?id=94735 - wl_surface@27 is a wl_shell_surface "org.kde.systemsettings5" and has content - wl_surface@44 is a sub-surface for parent wl_surface@27 - wl_surface@46 is a sub-surface for parent wl_surface@44 - wl_surface@49 is a sub-surface for parent wl_surface@46 - wl_surface@51 is a sub-surface for parent wl_surface@49 - wl_surface@53 is a sub-surface for parent wl_surface@51 - wl_surface@57 is a sub-surface for parent wl_surface@53 - wl_surface@59 is a sub-surface for parent wl_surface@57 - wl_surface@61 is a sub-surface for parent wl_surface@59 - wl_surface@63 is a sub-surface for parent wl_surface@61 - wl_surface@65 is a sub-surface for parent wl_surface@63 - wl_surface@97 is a sub-surface for parent wl_surface@65 If you read the protocol dump, mind that wl_surface ids are re-used, one needs to track back from the point where wl_surface@97 is created. All mentioned wl_surfaces 44 - 65 go through { set_buffer_scale, set_buffer_transform, commit } with no attach. So I think the test case should be: - main surface, has content, mapped - sub-surface, no content - subsurface, has content, bug: visible when should not That should avoid the issues of the test plugin marking a surface without content to be mapped, which can confuse sub-surface logic. (*) is probably what happens, but not just in the test plugin; that would be the real bug, I imagine. Thanks, pq
pgprcR7kYh1w_.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
