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

[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 <emilio.pozuelo at collabora.co.uk>
>> ---
>>  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