[vlc-devel] [PATCH] opengl: prevent opengl calls from different threads
Thomas Guillem
thomas at gllm.fr
Thu Sep 26 09:19:14 CEST 2019
On Thu, Sep 26, 2019, at 02:31, Alexandre Janniaux wrote:
> Hi,
>
> >On Wed, Sep 25, 2019 at 08:34:32PM +0300, Rémi Denis-Courmont wrote:
> > AFAIK, Wayland requires that you update the size and position synchronously as
> > a general rule. Is this so that that requirement only applies to
> > vlc_gl_Resize() and not the view port?
>
> The answer is that it doesn't apply to vlc_gl_Resize either,
> but it's an excellent remark.
>
> I'll try to post a detailed answer for documentation purpose,
> but I'm out of coffee so might not be 100% clear or right.
> Don't hesitate to highlight the blurred area.
>
> You might confuse glViewport with the wp_viewporter protocol
> of wayland. They are highly related, but have very different
> effects. wp_viewporter protocols allows to crop and rescale
> a given surface. Because of being a windowing-centred protocol
> the focus is on the final buffer.
>
> On the other side, glViewport only modify the opengl state for
> future gpu commands, and only defines an affine transform to
> get from normalized coordinates (-1.f to 1.f) to windows
> coordinates. But because this is an affine transform you can
> have x=2.f in normalized (clip) space and get 2*the width you
> set in glViewport + the defined offset. It doesn't define clipping,
> thus sizing when talking about wayland surface. It litteraly just
> map [-1.f, 1.f]^2 to the place_t coordinates, which is why
> subtitles are zoomed when using a FIT_TO_CROP-like mode on smartphone.
>
> 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).
>
> + recursively, you ask the children layers to do the same
>
> + when you reach the leaf, you can commit() them.
>
> + then you go backward in the tree and commit() the surfaces
> before setting back wl_surface_set_async().
>
> The steps between set_sync() and commit()+set_async() for each layer
> don't need to be synchronous in term of threads. However you have to
> make sure they are done before the commit().
>
> To get back to OpenGL, this commit() is done in the SwapBuffer call
> so even the vlc_gl_Resize doesn't have visible effects to wayland,
> and the Viewport hasn't even produced pixels. It's currently missing
> a display() step to produce this. But all this synchronization could
> be handled by the core to signal the fourth step of resizing.
>
> Unrelated to resizing, this change is also probably needed to solve
> the macosx glitches, which should probably suffer from the same issue
> as Qt. And using MakeCurrent in thread that you don't control is
> harmful, because OpenGL is absolutely not designed to handle this use
> case of different unaware clients in a single thread.
>
> I hope everything is clear.
Thanks for the very good explanation.
>
> Regards,
> --
> Alexandre Janniaux
> Videolabs
>
> >On Wed, Sep 25, 2019 at 08:34:32PM +0300, Rémi Denis-Courmont wrote:
> > Le keskiviikkona 25. syyskuuta 2019, 17.36.19 EEST Alexandre Janniaux a écrit
> > :
> > > Most display controls, especially sizing ones, have been made synchronous,
> > > while an OpenGL context should be used in only one thread at a time and
> > > should not interfere with the context of the control calling thread.
> > > It is especially harmful when the calling context is using OpenGL, like
> > > in interfaces.
> > > ---
> > > modules/video_output/opengl/display.c | 30 ++++++++++++++-------------
> > > 1 file changed, 16 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/modules/video_output/opengl/display.c
> > > b/modules/video_output/opengl/display.c index 456304ebdfb..cc9989ec056
> > > 100644
> > > --- a/modules/video_output/opengl/display.c
> > > +++ b/modules/video_output/opengl/display.c
> > > @@ -73,6 +73,8 @@ struct vout_display_sys_t
> > > vout_display_opengl_t *vgl;
> > > vlc_gl_t *gl;
> > > picture_pool_t *pool;
> > > + vout_display_place_t place;
> > > + bool place_changed;
> > > };
> > >
> > > /* Display callbacks */
> > > @@ -93,6 +95,7 @@ static int Open(vout_display_t *vd, const
> > > vout_display_cfg_t *cfg,
> > >
> > > sys->gl = NULL;
> > > sys->pool = NULL;
> > > + sys->place_changed = false;
> > >
> > > vout_window_t *surface = cfg->window;
> > > char *gl_name = var_InheritString(surface, MODULE_VARNAME);
> > > @@ -208,6 +211,15 @@ static void PictureDisplay (vout_display_t *vd,
> > > picture_t *pic)
> > >
> > > if (vlc_gl_MakeCurrent (sys->gl) == VLC_SUCCESS)
> > > {
> > > + if (sys->place_changed)
> > > + {
> > > + float window_ar = (float)sys->place.width / sys->place.height;
> > > + vout_display_opengl_SetWindowAspectRatio(sys->vgl, window_ar);
> > > + vout_display_opengl_Viewport(sys->vgl, sys->place.x,
> > > sys->place.y,
> > > + sys->place.width,
> > > sys->place.height);
> > > + sys->place_changed = false;
> >
> > AFAIK, Wayland requires that you update the size and position synchronously as
> > a general rule. Is this so that that requirement only applies to
> > vlc_gl_Resize() and not the view port?
> >
> > > + }
> > > +
> > > vout_display_opengl_Display (sys->vgl, &vd->source);
> > > vlc_gl_ReleaseCurrent (sys->gl);
> > > }
> > > @@ -230,7 +242,6 @@ static int Control (vout_display_t *vd, int query,
> > > va_list ap) {
> > > vout_display_cfg_t c = *va_arg (ap, const vout_display_cfg_t *);
> > > const video_format_t *src = &vd->source;
> > > - vout_display_place_t place;
> > >
> > > /* Reverse vertical alignment as the GL tex are Y inverted */
> > > if (c.align.vertical == VLC_VIDEO_ALIGN_TOP)
> > > @@ -238,13 +249,9 @@ static int Control (vout_display_t *vd, int query,
> > > va_list ap) else if (c.align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
> > > c.align.vertical = VLC_VIDEO_ALIGN_TOP;
> > >
> > > - vout_display_PlacePicture(&place, src, &c);
> > > + vout_display_PlacePicture(&sys->place, src, &c);
> > > + sys->place_changed = true;
> > > vlc_gl_Resize (sys->gl, c.display.width, c.display.height);
> > > - if (vlc_gl_MakeCurrent (sys->gl) != VLC_SUCCESS)
> > > - return VLC_SUCCESS;
> > > - vout_display_opengl_SetWindowAspectRatio(sys->vgl,
> > > (float)place.width / place.height); -
> > > vout_display_opengl_Viewport(sys->vgl, place.x, place.y, place.width,
> > > place.height);
> > > - vlc_gl_ReleaseCurrent (sys->gl);
> > > return VLC_SUCCESS;
> > > }
> > >
> > > @@ -252,14 +259,9 @@ static int Control (vout_display_t *vd, int query,
> > > va_list ap) case VOUT_DISPLAY_CHANGE_SOURCE_CROP:
> > > {
> > > const vout_display_cfg_t *cfg = va_arg (ap, const
> > > vout_display_cfg_t *); - vout_display_place_t place;
> > >
> > > - vout_display_PlacePicture(&place, &vd->source, cfg);
> > > - if (vlc_gl_MakeCurrent (sys->gl) != VLC_SUCCESS)
> > > - return VLC_SUCCESS;
> > > - vout_display_opengl_SetWindowAspectRatio(sys->vgl,
> > > (float)place.width / place.height); -
> > > vout_display_opengl_Viewport(sys->vgl, place.x, place.y, place.width,
> > > place.height); - vlc_gl_ReleaseCurrent (sys->gl);
> > > + vout_display_PlacePicture(&sys->place, &vd->source, cfg);
> > > + sys->place_changed = true;
> > > return VLC_SUCCESS;
> > > }
> > > case VOUT_DISPLAY_CHANGE_VIEWPOINT:
> >
> >
> > --
> > 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
> _______________________________________________
> 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