[vlc-devel] [PATCH] opengl: prevent opengl calls from different threads
Alexandre Janniaux
ajanni at videolabs.io
Thu Sep 26 02:31:08 CEST 2019
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.
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
More information about the vlc-devel
mailing list