[vlc-devel] [PATCH] caopengllayer: do not return an error when crop/zoom/aspect/etc fail

[Alexandre Janniaux]

Hi,

On Thu, Oct 24, 2019 at 12:02:13PM +0200, Steve Lhomme wrote:
> On 2019-10-24 11:50, Rémi Denis-Courmont wrote:
> > The code is already quite confusing without the patch. It does not make
> > much sense to fail locking the GL, unless there is a bug somewhere else.
>
> There are 2 calls that can fail OpenglLock() one of them being a call to
> Apple API. I agree it should not happen in real like, hence the unlikely().

I'm not sure that failing to lock an OpenGL context isn't
different than failing to do an allocation. It rarely
happens but there are runtime reasons which might lead to
this kind of failures too.

> I think it's good to not silently return no error when in fact the original
> control was not handled properly. At some point we'll probably have to
> handle RESET_PICTURES in modules that currently don't handle the controls
> correctly.

I might not completely understand your point. The previous
code was actually returning an error when the control was
not handled properly so it alread seemed to match your
point. Do I miss something ?

> In this case we just avoid the code ending up in vlc_assert_unreachable().

I'd say that it might be the correct issue to fix here. If
we can fail and that the core can call RESET_PICTURES, we
should probably support this in the module instead of
silence the error to avoid the assertion, or keep the
assertion to highlight that we don't support it.

Regards,
--
Alexandre Janniaux
Videolabs

>
> > Le 24 octobre 2019 12:14:29 GMT+03:00, Marvin Scholz
> > <epirat07 at gmail.com> a écrit :
> >
> >
> >
> >     On 24 Oct 2019, at 11:10, Steve Lhomme wrote:
> >
> >         Otherwise the core will call VOUT_DISPLAY_RESET_PICTURES which we
> >         don't support.
> >
> >         This is what is done in macosx.m when vlc_gl_MakeCurrent() fails.
> >         ------------------------------------------------------------------------
> >         modules/video_output/caopengllayer.m | 4 ++--
> >         1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >         diff --git a/modules/video_output/caopengllayer.m
> >         b/modules/video_output/caopengllayer.m
> >         index 3b542d1e637..249f7b96d5d 100644
> >         --- a/modules/video_output/caopengllayer.m
> >         +++ b/modules/video_output/caopengllayer.m
> >         @@ -343,8 +343,8 @@ static int Control (vout_display_t *vd, int
> >         query,
> >         va_list ap)
> >
> >         vout_display_place_t place;
> >         vout_display_PlacePicture(&place, &vd->source, &cfg_tmp);
> >         - if (OpenglLock(sys->gl))
> >         - return VLC_EGENERIC;
> >         + if (unlikely(OpenglLock(sys->gl)))
> >         + return VLC_SUCCESS;
> >
> >
> >     Can you add a comment here explaining that? Else the code looks quite
> >     confusing here without context.
> >
> >     >
> >
> >         vout_display_opengl_SetWindowAspectRatio(sys->vgl,
> >         (float)place.width / place.height);
> >         OpenglUnlock(sys->gl);
> >         --         2.17.1
> >         ------------------------------------------------------------------------
> >         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
> >
> >
> > --
> > Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
> > ma brièveté.
> >
> > _______________________________________________
> > 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