date:20170203

Re: [PATCH weston 3/3] compositor: don't map surfaces without a buffer

2017-02-03 Thread Emilio Pozuelo Monfort

On 03/02/17 16:27, Derek Foreman wrote:
> On 03/02/17 09:10 AM, Emilio Pozuelo Monfort wrote:
>> We were calling weston_surface::committed on surfaces with
>> no buffer attached. Stop doing that, since surface::committed
>> will map the surfaces and put them in a visible layer. That may
>> not be a problem for a single surface as it wouldn't be visible
>> anyway because it's got no contents, but it is a problem if the
>> surface has subsurfaces.
>>
>> This fixes the subsurface_mapped test, so mark it as expected
>> to succeed.
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=94735
>>
>> Signed-off-by: Emilio Pozuelo Monfort 
>> ---
>>  libweston/compositor.c   | 10 +-
>>  tests/subsurface-shot-test.c |  2 +-
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>> index 81392063..8a018897 100644
>> --- a/libweston/compositor.c
>> +++ b/libweston/compositor.c
>> @@ -1589,6 +1589,12 @@ weston_surface_is_mapped(struct weston_surface 
>> *surface)
>>  return surface->is_mapped;
>>  }
>>
>> +static bool
>> +weston_surface_has_content(struct weston_surface *surface)
>> +{
>> +return surface->width > 0 && surface->height > 0;
> 
> Are these going to be 0 if a NULL buffer is attached to an existing surface?

Ah, good point. I'm thinking I could extend the test case to attach a NULL
buffer, then attach a buffer again and verify the result is correct.

As for your question, it looks like that's not the case. width_from_buffer seems
reset by weston_surface_calculate_size_from_buffer(), called by
weston_surface_attach(), but surface->width/height aren't reset AFAICS.

I'll look at this.

> 
> A quick read has me thinking width_from_buffer and height_from_buffer are 
> reset
> on a NULL attach, but width and height only get updated on commit - so 
> depending
> on when this function is called it might not return the expected result.
> 
> If that's the intended behaviour, perhaps a comment to avoid misuse in the
> future...
> 
>> +}
>> +
>>  static void
>>  surface_set_size(struct weston_surface *surface, int32_t width, int32_t 
>> height)
>>  {
>> @@ -2928,7 +2934,9 @@ weston_surface_commit_state(struct weston_surface 
>> *surface,
>>
>>  if (state->newly_attached || state->buffer_viewport.changed) {
>>  weston_surface_update_size(surface);
>> -if (surface->committed)
>> +if (surface->committed &&
>> +(state->newly_attached &&
>> + weston_surface_has_content(surface)))
>>  surface->committed(surface, state->sx, state->sy);
>>  }
>>
>> diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c
>> index e7da1e0e..275d4726 100644
>> --- a/tests/subsurface-shot-test.c
>> +++ b/tests/subsurface-shot-test.c
>> @@ -261,7 +261,7 @@ TEST(subsurface_z_order)
>>  buffer_destroy(bufs[i]);
>>  }
>>
>> -FAIL_TEST(subsurface_mapped)
>> +TEST(subsurface_mapped)
>>  {
>>  const char *test_name = get_test_name();
>>  struct client *client;
> 
> Why not re-order these commits so the subsurface test is introduced in a 
> passing
> state?

I think it's useful to add it before the fix, so one can verify the test case
was broken and the fix works.

Cheers,
Emilio
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 3/3] compositor: don't map surfaces without a buffer

2017-02-03 Thread Derek Foreman


On 03/02/17 09:10 AM, Emilio Pozuelo Monfort wrote:

We were calling weston_surface::committed on surfaces with
no buffer attached. Stop doing that, since surface::committed
will map the surfaces and put them in a visible layer. That may
not be a problem for a single surface as it wouldn't be visible
anyway because it's got no contents, but it is a problem if the
surface has subsurfaces.

This fixes the subsurface_mapped test, so mark it as expected
to succeed.

https://bugs.freedesktop.org/show_bug.cgi?id=94735

Signed-off-by: Emilio Pozuelo Monfort 
---
 libweston/compositor.c   | 10 +-
 tests/subsurface-shot-test.c |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/libweston/compositor.c b/libweston/compositor.c
index 81392063..8a018897 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -1589,6 +1589,12 @@ weston_surface_is_mapped(struct weston_surface *surface)
return surface->is_mapped;
 }

+static bool
+weston_surface_has_content(struct weston_surface *surface)
+{
+   return surface->width > 0 && surface->height > 0;


Are these going to be 0 if a NULL buffer is attached to an existing surface?

A quick read has me thinking width_from_buffer and height_from_buffer 
are reset on a NULL attach, but width and height only get updated on 
commit - so depending on when this function is called it might not 
return the expected result.


If that's the intended behaviour, perhaps a comment to avoid misuse in 
the future...



+}
+
 static void
 surface_set_size(struct weston_surface *surface, int32_t width, int32_t height)
 {
@@ -2928,7 +2934,9 @@ weston_surface_commit_state(struct weston_surface 
*surface,

if (state->newly_attached || state->buffer_viewport.changed) {
weston_surface_update_size(surface);
-   if (surface->committed)
+   if (surface->committed &&
+   (state->newly_attached &&
+weston_surface_has_content(surface)))
surface->committed(surface, state->sx, state->sy);
}

diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c
index e7da1e0e..275d4726 100644
--- a/tests/subsurface-shot-test.c
+++ b/tests/subsurface-shot-test.c
@@ -261,7 +261,7 @@ TEST(subsurface_z_order)
buffer_destroy(bufs[i]);
 }

-FAIL_TEST(subsurface_mapped)
+TEST(subsurface_mapped)
 {
const char *test_name = get_test_name();
struct client *client;


Why not re-order these commits so the subsurface test is introduced in a 
passing state?


Thanks,
Derek
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 3/3] compositor: don't map surfaces without a buffer

2017-02-03 Thread Emilio Pozuelo Monfort

We were calling weston_surface::committed on surfaces with
no buffer attached. Stop doing that, since surface::committed
will map the surfaces and put them in a visible layer. That may
not be a problem for a single surface as it wouldn't be visible
anyway because it's got no contents, but it is a problem if the
surface has subsurfaces.

This fixes the subsurface_mapped test, so mark it as expected
to succeed.

https://bugs.freedesktop.org/show_bug.cgi?id=94735

Signed-off-by: Emilio Pozuelo Monfort 
---
 libweston/compositor.c   | 10 +-
 tests/subsurface-shot-test.c |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/libweston/compositor.c b/libweston/compositor.c
index 81392063..8a018897 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -1589,6 +1589,12 @@ weston_surface_is_mapped(struct weston_surface *surface)
return surface->is_mapped;
 }
 
+static bool
+weston_surface_has_content(struct weston_surface *surface)
+{
+   return surface->width > 0 && surface->height > 0;
+}
+
 static void
 surface_set_size(struct weston_surface *surface, int32_t width, int32_t height)
 {
@@ -2928,7 +2934,9 @@ weston_surface_commit_state(struct weston_surface 
*surface,
 
if (state->newly_attached || state->buffer_viewport.changed) {
weston_surface_update_size(surface);
-   if (surface->committed)
+   if (surface->committed &&
+   (state->newly_attached &&
+weston_surface_has_content(surface)))
surface->committed(surface, state->sx, state->sy);
}
 
diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c
index e7da1e0e..275d4726 100644
--- a/tests/subsurface-shot-test.c
+++ b/tests/subsurface-shot-test.c
@@ -261,7 +261,7 @@ TEST(subsurface_z_order)
buffer_destroy(bufs[i]);
 }
 
-FAIL_TEST(subsurface_mapped)
+TEST(subsurface_mapped)
 {
const char *test_name = get_test_name();
struct client *client;
-- 
2.11.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

2017-02-03 Thread Emilio Pozuelo Monfort

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 
---
 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 
..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)w43YGIc&-r>L$&*16m=d#Wzp$P!YDfm(V

literal 0
HcmV?d1

diff --git a/tests/reference/subsurface_mapped-01.png 
b/tests/reference/subsurface_mapped-01.png
new file mode 100644
index 
..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}^{z1zopr
E02fd6egFUf

literal 0
HcmV?d1

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);
+
+   /* 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);
+
+   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);
+
+   /* 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);
+
+   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);
+}
-- 
2.11.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/3] tests: add a create_test_surface function

2017-02-03 Thread Emilio Pozuelo Monfort

This doesn't attach a buffer to the surface. This is needed for the
next commit, where we have a test case with a surface that doesn't
have a buffer attached.

Signed-off-by: Emilio Pozuelo Monfort 
---
 tests/weston-test-client-helper.c | 29 -
 tests/weston-test-client-helper.h |  3 +++
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/tests/weston-test-client-helper.c 
b/tests/weston-test-client-helper.c
index dd411925..4c27988d 100644
--- a/tests/weston-test-client-helper.c
+++ b/tests/weston-test-client-helper.c
@@ -884,18 +884,13 @@ create_client(void)
return client;
 }
 
-struct client *
-create_client_and_test_surface(int x, int y, int width, int height)
+struct surface *
+create_test_surface(struct client *client)
 {
-   struct client *client;
struct surface *surface;
-   pixman_color_t color = { 16384, 16384, 16384, 16384 }; /* uint16_t */
-   pixman_image_t *solid;
-
-   client = create_client();
 
-   /* initialize the client surface */
surface = xzalloc(sizeof *surface);
+
surface->wl_surface =
wl_compositor_create_surface(client->wl_compositor);
assert(surface->wl_surface);
@@ -903,9 +898,25 @@ create_client_and_test_surface(int x, int y, int width, 
int height)
wl_surface_add_listener(surface->wl_surface, &surface_listener,
surface);
 
-   client->surface = surface;
wl_surface_set_user_data(surface->wl_surface, surface);
 
+   return surface;
+}
+
+struct client *
+create_client_and_test_surface(int x, int y, int width, int height)
+{
+   struct client *client;
+   struct surface *surface;
+   pixman_color_t color = { 16384, 16384, 16384, 16384 }; /* uint16_t */
+   pixman_image_t *solid;
+
+   client = create_client();
+
+   /* initialize the client surface */
+   surface = create_test_surface(client);
+   client->surface = surface;
+
surface->width = width;
surface->height = height;
surface->buffer = create_shm_buffer_a8r8g8b8(client, width, height);
diff --git a/tests/weston-test-client-helper.h 
b/tests/weston-test-client-helper.h
index 8908ae7e..8ab59c61 100644
--- a/tests/weston-test-client-helper.h
+++ b/tests/weston-test-client-helper.h
@@ -155,6 +155,9 @@ struct rectangle {
 struct client *
 create_client(void);
 
+struct surface *
+create_test_surface(struct client *client);
+
 struct client *
 create_client_and_test_surface(int x, int y, int width, int height);
 
-- 
2.11.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protocols v3] linux-dmabuf: add immediate dmabuf import path

2017-02-03 Thread Pekka Paalanen

On Tue, 31 Jan 2017 18:41:52 +0530
Varad Gautam  wrote:

> From: Varad Gautam 
> 
> provide a mechanism that allows clients to import the added dmabufs
> and immediately use the newly created wl_buffers without waiting on
> an event. this is useful to clients that are sure of their import
> request succeeding, and wish to avoid the wl_buffer communication
> roundtrip.
> 
> bump zwp_linux_dmabuf_v1, zwp_linux_buffer_params_v1 interface
> versions.
> 
> v2: specify using incorrectly imported dmabufs as undefined behavior
> instead of sending success/failure events. (pq, daniels)
> v3: preserve the optional protocol error added in v2 and explicitly
> state the outcome of import success or failure (pq)
> 
> Signed-off-by: Varad Gautam 
> ---
>  unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml | 58 
> +++---
>  1 file changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml 
> b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> index ed2c4bb..ddb49cc 100644
> --- a/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> +++ b/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml
> @@ -24,13 +24,13 @@
>  DEALINGS IN THE SOFTWARE.
>
>  
> -  
> +  
>  
>Following the interfaces from:
>
> https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_import.txt
>and the Linux DRM sub-system's AddFb2 ioctl.
>  
> -  This interface offers a way to create generic dmabuf-based
> +  This interface offers ways to create generic dmabuf-based
>wl_buffers. Immediately after a client binds to this interface,
>the set of supported formats is sent with 'format' events.
>  
> @@ -56,10 +56,23 @@
>To create a wl_buffer from one or more dmabufs, a client creates a
>zwp_linux_dmabuf_params_v1 object with a 
> zwp_linux_dmabuf_v1.create_params
>request. All planes required by the intended format are added with
> -  the 'add' request. Finally, a 'create' request is issued. The server
> -  will reply with either a 'created' event which provides the final
> -  wl_buffer or a 'failed' event saying that it cannot use the dmabufs
> -  provided.
> +  the 'add' request. Finally, a 'create' or 'create_immed' request is
> +  issued, which has the following outcome depending on the import 
> success.
> +
> +  The 'create' request,
> +  - on success, triggers a 'created' event which provides the final
> +wl_buffer to the client.
> +  - on failure, triggers a 'failed' event to convey that the server
> +cannot use the dmabufs received from the client.
> +
> +  For the 'create_immed' request,
> +  - on success, the server immediately imports the added dmabufs to
> +create a wl_buffer. No event is sent from the server in this case.
> +  - on failure, the server can choose to either:
> +- terminate the client by raising a fatal error.
> +- create an invalid wl_buffer handle and send a 'failed' event to
> +  the client. The result of using this invalid wl_buffer in the
> +  client is left implementation-defined by the protocol.

Hi,

I would phrase the latter as:

- Mark the wl_buffer as failed and send a 'failed' event to the
  client. If the client uses a failed wl_buffer as an argument
  to any request, the behaviour is compositor
  implementation-defined.

>  
>Warning! The protocol described in this file is experimental and
>backward incompatible changes may be made. Backward compatible changes
> @@ -105,7 +118,7 @@
>  
>
>  
> -  
> +  
>  
>This temporary object is a collection of dmabufs and other
>parameters that together form a single logical buffer. The temporary
> @@ -138,6 +151,9 @@
>   summary="invalid width or height"/>
>   summary="offset + stride * height goes out of dmabuf bounds"/>
> +   + summary="invalid wl_buffer resulted from importing dmabufs from
> +   the given buffer_params"/>

I might have called it "import_failure". Not a big deal.

>  
>  
>  
> @@ -269,6 +285,34 @@
>  zlinux_buffer_params object.
>
>  
> +
> +
> +  
> +This asks for immediate creation of a wl_buffer by importing the
> +added dmabufs.
> +
> +In case of import success, no event is sent from the server, and the
> +wl_buffer is ready to be used by the client.
> +
> +Upon import failure, either of the following may happen, as seen fit
> +by the implementation:
> +- the client is terminated with a fatal protocol error.

Could mention the error code here, since there is one specifically for
this case.

> +- the server creates an invalid wl_buffer and sends a 'failed' event
> +  to the client. The result of using this invalid wl_buffer created
> +

Re: [PATCH weston 3/3] compositor: don't map surfaces without a buffer

Re: [PATCH weston 3/3] compositor: don't map surfaces without a buffer

[PATCH weston 3/3] compositor: don't map surfaces without a buffer

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

[PATCH weston 1/3] tests: add a create_test_surface function

Re: [PATCH wayland-protocols v3] linux-dmabuf: add immediate dmabuf import path

6 matches

Site Navigation

Mail list logo

Footer information