[PATCH weston 2/3] shot: add test for sub-surface with unmapped parent

[Pekka Paalanen]

On Fri,  3 Feb 2017 16:10:38 +0100
Emilio Pozuelo Monfort <pochu27 at 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.pozuelo at 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 at N?(olHy`uVBq!ia0y~yU~~YoKX5Ps$$$P at Hb9Ck$=lt9;Xep2*t>i(0|V0)  
> zPZ!6KiaBpDJMtb-U^sBV(DcSZzE>X)w43Y<zTkg6<~Q at 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 at N?(olHy`uVBq!ia0y~yU~~YoKX5Ps$$$P at Hb9Ck$=lt9;Xep2*t>i(0|V1P  
> zPZ!6KiaBquZsa}Wz`){YUu2kf{Oyc6Wg6{8K0QxPoXkGz!#aIUUe52048Hw0nHzqy
> z at 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 at 27 is a wl_shell_surface "org.kde.systemsettings5" and has
  content
- wl_surface at 44 is a sub-surface for parent wl_surface at 27
- wl_surface at 46 is a sub-surface for parent wl_surface at 44
- wl_surface at 49 is a sub-surface for parent wl_surface at 46
- wl_surface at 51 is a sub-surface for parent wl_surface at 49
- wl_surface at 53 is a sub-surface for parent wl_surface at 51
- wl_surface at 57 is a sub-surface for parent wl_surface at 53
- wl_surface at 59 is a sub-surface for parent wl_surface at 57
- wl_surface at 61 is a sub-surface for parent wl_surface at 59
- wl_surface at 63 is a sub-surface for parent wl_surface at 61
- wl_surface at 65 is a sub-surface for parent wl_surface at 63
- wl_surface at 97 is a sub-surface for parent wl_surface at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170221/d873687e/attachment-0001.sig>