[vlc-devel] [PATCH 2/2] wayland: add background surface and center video

[ajanni at videolabs.io]

Hi, thank you for review, most answers inline.

There are also some linked issues that I've not tackled and forgot to 
mention:

+ synchronized resize of window + vout, because it's hard and I don't 
have
   a solution currently.
+ parent surface lifetime (which is currently less than vout), mostly 
because
   I thought I could solve it this way but subsurfaces didn't work the 
way I
   expected.


On 2019-02-05 17:09, Rémi Denis-Courmont wrote:
> Hi,
> 
> Le tiistaina 5. helmikuuta 2019, 15.51.11 EET Alexandre Janniaux a 
> écrit :
>> Add a subsurface which display the video, allowing to change its
>> position from the vout display to center it, and use the vout
>> window surface to render a black area of the size of the display area.
> 
> It is generally not a good idea to implement a fix for a bug that is 
> assigned
> to somebody else.

Sorry, my mistake. I must have missed the ticket as I the SPU one but 
not this
one. Either way my main reason was that I needed the feature and it has 
been
sleeping in my branch for two months. Is it still ok for you?

For reference to the mailing list, the mentionned one is there:
https://trac.videolan.org/vlc/ticket/18045

>> +    struct
>> +    {
>> +        int fd;
> 
> I don't exactly see why you'd need to keep the descriptor around.

You're right, I'll clean it.

>> +        struct wl_buffer *buffer;
>> +        struct wp_viewport *viewport;
> 
> Scaling black seems rather weird, and AFAIK you cannot assume that 
> viewport is
> supported anyway.

I agree, the other possibility is to copy the {subsurface, surface} 
instead of
the surface only, and create the subsurface outside of the video output. 
It was
my first version but I thought owning the video surface in the video 
output was
more interesting. Especially, shm buffer are usually uploaded to GPU 
textures
but GPU wl_buffer are not and might be kept by some compositor even when 
the
surface is unmapped. Deleting the surface force them to be released when 
the
vout is closed.

But either way you have to put something behind the video subsurface. We 
could
provide /dev/zero's correctly sized buffer, but the compositor will 
likely
upload them again in a new texture, so this seems not performance-wise 
to me.

>> +    wl_surface_attach(sys->embed->handle.wl, sys->back_bg.buffer, 0, 
>> 0);
>> +    wl_surface_damage(sys->embed->handle.wl, 0, 0, 
>> sys->display_width,
>> sys->display_height);
>> wl_display_flush(display);
> 
> It' seems a bit dubious that you need to attach and damage both 
> surfaces. Also
> seems to contradict use of unsynchronized mode.
> 
>>      sys->active_buffers++;
>> @@ -144,9 +159,9 @@ static void Display(vout_display_t *vd, picture_t 
>> *pic)
>>  {
>>      vout_display_sys_t *sys = vd->sys;
>>      struct wl_display *display = sys->embed->display.wl;
>> -    struct wl_surface *surface = sys->embed->handle.wl;
>> 
>> -    wl_surface_commit(surface);
>> +    wl_surface_commit(sys->surface);
>> +    wl_surface_commit(sys->embed->handle.wl);
>>      wl_display_roundtrip_queue(display, sys->eventq);
> 
> Likewise.

I agree, and would have expected this, but like the protocol says:

>       Sub-surfaces have also other kind of state, which is managed by
>       wl_subsurface requests, as opposed to wl_surface requests. This
>       state includes the sub-surface position relative to the parent
>       surface (wl_subsurface.set_position), and the stacking order of
>       the parent and its sub-surfaces (wl_subsurface.place_above and
>       .place_below). This state is applied when the parent surface's
>       wl_surface state is applied, regardless of the sub-surface's 
> mode.

Putting this in the Prepare/Display couple was the easy solution. 
Otherwise
I can move this close to the subsurface_set_position call but it was a 
bit
odd to have the surface_attach line there.

>> 
>>      (void) pic;
>> @@ -207,6 +222,17 @@ static int Control(vout_display_t *vd, int query,
>> va_list ap) wl_fixed_from_int(fmt.i_visible_height));
>> wp_viewport_set_destination(sys->viewport,
>>                                              place.width, 
>> place.height);
>> +
>> +                wl_subsurface_set_position(sys->subsurface, place.x,
>> place.y); +
>> +                wp_viewport_set_source(sys->back_bg.viewport,
>> +                                       wl_fixed_from_int(0),
>> +                                       wl_fixed_from_int(0),
>> +                                       wl_fixed_from_int(1),
>> +                                       wl_fixed_from_int(1));
>> +                wp_viewport_set_destination(sys->back_bg.viewport,
>> +                                            cfg->display.width,
>> +                                            cfg->display.height);
>>              }
>>              else
>>                  return VLC_EGENERIC;
> 
> Missing handling here.

Indeed, thank you.

>> +    /* UI surface is updated less frequently than the video
>> +     * so we need to be in a desync mode */
>> +    wl_subsurface_set_desync(sys->subsurface);
>> +    wl_subsurface_place_above(sys->subsurface, 
>> sys->embed->handle.wl);
>> +
>> +    /* Clients like Qt can expect every surface to be their surface, 
>> and
>> get
>> +     * events from pointer object,
> 
> No it can't. This module use a separate event queue exactly to prevent 
> that
> sort of problems.

I might be wrong on this issue, but without I've got a crash in Qt code 
for
handling wl_pointer::enter event as soon as the mouse goes on the 
surface.

My understanding is that because the Qt wl_pointer's proxy object is not
associated with this event queue, having the surface in a different 
queue
doesn't change the issue.

However, I agree it's more like a hack to fix a bug in QtWayland and a 
flaw
in wayland's wl_pointer object conception than a feature.

>> 
>> +    /* Initialize background wl_buffer */
>> +
>> +    sys->back_bg.fd = vlc_memfd();
> 
> Might as well use /dev/zero AFAICT.

I didn't thought about this, quite smart.


-- 
Alexandre Janniaux