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

[Alexandre Janniaux]

Hi,

On Sat, Oct 26, 2019 at 01:11:15PM +0100, Rémi Denis-Courmont wrote:
> Hi,
>
> The implementation ought to allocate enough resources upfront that locking cannot fail. Otherwise, how do you clean up the OpenGL context that you can't even lock??

I agree that it would be the sane way, but we have multiple
example of things that were broken by Apple during the
deprecation of OpenGL, and the function doesn't specify why
it would fail.

So my only source of truth is the list of error code, where
you can find kCGLBadAlloc. But you could have perfectly
valid other issues from outside of this module like:
+ kCGLBadConnection
+ kCGLBadWindow
+ kCGLBadDisplay
+ kCGLBadDrawable

A lot of thing broke with the new Catalina version, and
Apple is doing things that have weird interactions with our
code (like the non reduced-transparency mode provoking
glitches everywhere or the viewport issues) and I'd prefer
relying on documented behaviour rather than assumptions.
Especially now that OpenGL is deprecated on Apple systems.

Regards,
--
Alexandre Janniaux
Videolabs

>
> AFAICT, the error only exists for repprting invalid parameters, which would be a bug in the app.
>
> Le 26 octobre 2019 12:04:43 GMT+01:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >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
> >_______________________________________________
> >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