[vlc-devel] [PATCH] opengl: prevent opengl calls from different threads

Alexandre Janniaux ajanni at videolabs.io
Thu Sep 26 23:04:01 CEST 2019


Hi,

You made a good remark here.

I don't think we have any issue with wl_shell, given it's deprecation,
its limits and its removal:

>From https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_shell_surface
> The size is a hint, in the sense that the client is free to ignore
> it if it doesn't resize, pick a smaller size (to satisfy aspect ratio
> or resize in steps of NxM pixels).

On the xdg-shell side, the constraints are what you said, you have
to commit only after.

>From https://github.com/wayland-project/wayland-protocols/blob/master/stable/xdg-shell/xdg-shell.xml#L485
> When a configure event is received, if a client commits the
> surface in response to the configure event, then the client
> must make an ack_configure request sometime before the commit
> request, passing along the serial of the configure event.

But this is a lot more flexible than this:

>From https://github.com/wayland-project/wayland-protocols/blob/master/stable/xdg-shell/xdg-shell.xml#L497
> A client is not required to commit immediately after sending
> an ack_configure request - it may even ack_configure several times
> before its next surface commit.
>
> A client may send multiple ack_configure requests before committing, but
> only the last request sent before a commit indicates which configure
> event the client really is responding to.

So it means that you can and should actually acknowledge the size before
trying to propagate the window. But it's a bug that is already present
and that is completely independant of this patch. Would you want to fix
it?

Here, the issue rely on the fact that the display can draw the wrong size
after the `ack_configure` is called.

+ If you try to enforce the resize + swap before the ack, you violate the
protocol requirements because you commited a surface "in response to the
configure event".

+ If you just enforce the resize without the swap, before the ack, then
you can have a race condition with the display() being called before the
ack_configure is sent, which is the same as above, since you're not
locking the vout display anymore.

So anyway, as I've mentionned, that's why I want this to be handled by
the core instead. There are not hundreds of way to achieve synchronized
resizing, be it with the windowing system or the composition system, and
especially when talking about wayland. The steps I've described for
composition also apply here:

+ the parent layer mixes two synchronization mechanism: VLC and Wayland.
  It has to prepare children synchronization and then has to call
  ack_configure

+ recursively, you ask your children layer to do the same via ReportSize
   |
   | The children layers are surfaces, so back to the previous steps I've
   | described.

+ When you reach the leaf, you can still commit.

+ then you go backward, same thing done.

+ In return VLC will signal when its job has been done.

The only point is that you have to signal and handle the synchronization
in you whole pipeline, meaning pausing display, preparing next deadline
and avoiding accepting new configure offers until you actually draw
something, except if you still have a fair amount of time until the
next deadline and can block the display before that.

I've made a few architecture throughts on my own time for this, and I
would have perfered viewing the vout_thread like an usual GPU pipeline
in a renderer API: you put commands and synchronization primitives like
fences and semaphores inside, that you use for signaling to the rest of
VLC. But it became quite different from now and I focused on other
subjects after.

About this part of your remarks:

> Committing an updated surface is not necessary before the acknowledgement. In
> fact, I think it's even forbidden. As such, I think it is not only acceptable,
> but necessary that swapping OpenGL buffers happens some unspecified time after
> the acknowledgement. However, any swap before the VOUT_DISPLAY_CHANGE_* must
> use the old size, and any swap after the VOUT_DISPLAY_CHANGE_* must use the
> new size - because the window provider will send the acknowlegdement right
> after vout_window_ReportSize() returns.

Yes, it should happen as immediately as possible, but immediate doesn't
exist when it comes to displaying video. You're always regulated by an
external source on which you have to synchronize. As the protocol doesn't
need anything special, there is no "deadline" that you have to follow.

The next available time slot to draw the picture seems a no-compromise
candidate given that the display would have sleeped until next VSYNC
deadline, or it would have been stuck in the display lock until the end
of the resize, or it would be waiting for the next picture deadline
which means that it is completely ready to draw a new picture. You're
likely to miss a frame only in the latest case, in case the VSYNC
timings have been eated, but it means that it's nearly the same case
as waiting for the next VSYNC. If you miss more frames, it means that
the wayland compositor will probably do a better job at compromise than
the application itself or that the whole environment is probably already
slow.

Finally, again, I hardly see the relation between all of this and my
patch. I'm solely touch the rendering API part, not the underlying
window resources part, which are handled by the vlc_gl_t provider and
not by the OpenGL API.

Thank you for your remark.

Regards,
--
Alexandre Janniaux
Videolabs

On Thu, Sep 26, 2019 at 09:31:52PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 26. syyskuuta 2019, 3.31.08 EEST Alexandre Janniaux a écrit :
> > vlc_gl_Resize is the correct way to do the sizing. But if you try
> > to resize the gl display in a wayland window currently, you'll see
> > that it's not synchronized at all. It's because there is a lot
> > missing in the "update the size and position synchronously" sentence.
> >
> > Wayland requires nothing specially, but if you want to achieve
> > perfect frame resizing, you have to do the following:
> >
> > + the parent layer in the hierarchy has to use wl_surface_set_sync()
> >   on the surface, and potentially modify its state (attachment,
> >   viewporter, scale, position).
>
> You are hereby assuming that there is a parent window, which should eventually
> become true. But currently, the only sane way to do video output on Wayland is
> the standalone XDG shell window provider - which, by definition, does not rely
> on a parent window.
>
> In that later case, as far as I understand, the vout display "merely" needs to
> guarantee that all surfaces committed before acknowledging the change of size
> use the old size, and all surfaces committed after acknowledging the change of
> size use the new size.
>
> Committing an updated surface is not necessary before the acknowledgement. In
> fact, I think it's even forbidden. As such, I think it is not only acceptable,
> but necessary that swapping OpenGL buffers happens some unspecified time after
> the acknowledgement. However, any swap before the VOUT_DISPLAY_CHANGE_* must
> use the old size, and any swap after the VOUT_DISPLAY_CHANGE_* must use the
> new size - because the window provider will send the acknowlegdement right
> after vout_window_ReportSize() returns.
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list