[vlc-devel] [PATCH] mediacodec: fix crash with --no-mediacodec-dr

[Alexandre Janniaux]

Hi,

On Mon, Jan 04, 2021 at 10:24:36AM +0100, Alexandre Janniaux wrote:
> Hi,
>
> On Mon, Jan 04, 2021 at 10:04:32AM +0100, Steve Lhomme wrote:
> > On 2021-01-04 9:48, Alexandre Janniaux wrote:
> > > Hi,
> > >
> > > On Mon, Jan 04, 2021 at 09:38:45AM +0100, Steve Lhomme wrote:
> > > > This is correct but is not very resilient. ReleaseAllPictureContexts can be
> > > > called from CloseDecoder > CleanDecoder > StopMediaCodec. When it called the
> > > > video.ctx has already been freed. It's only because it's not set to NULL
> > > > that this code would work.
> > >
> > > I'm not sure of what you mean.
> >
> > You're testing a pointer value that is free'd. This is borderline IMO.
>
> I don't think I am, hence why I don't understand. ;)

To be more specific, if you check lines containing `video.ctx`,
you'll see that it's never reset to NULL and not freed. The
free would come from vlc_video_context_Release() and reference
count is one from the decoder (protecting calls like flush) and
one for every picture. This function would be called from the
video context release function which means that the context
still exists and is not freed.

In the case I'm writing this patch for, video.ctx is never
assigned and is always NULL.

Regards,
--
Alexandre Janniaux
Videolabs

> > But in fact when the VCTX was not NULL this code is not called.
> >
> > StopMediaCodec() is called only in CleanDecoder (if VCTX is NULL) or during
> > a decoder drain/restart. But the Flush has already called the
> > ReleaseAllPictureContexts() so in this case it's called twice ?
>
> That would not be an issue anyway.
>
> > > >
> > > > IMO it would be better to just initialize the table with a NULL VCTX.
> > >
> > > Which table?
> >
> > video.apic_ctxs, array if you prefer.
> >
> > > Regards,
> > > --
> > > Alexandre Janniaux
> > > Videolabs
> > >
> > > > On 2021-01-03 16:40, Alexandre Janniaux wrote:
> > > > > The android picture contexts are not initialized when mediacodec-dr has
> > > > > been disabled, since it's done at the creation of the video context.
> > > > > Since we don't have android picture, it's indeed expected that we don't
> > > > > need to release their context.
> > > > >
> > > > > Fix #24698
> > > > > ---
> > > > >    modules/codec/omxil/mediacodec.c | 4 ++++
> > > > >    1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/modules/codec/omxil/mediacodec.c b/modules/codec/omxil/mediacodec.c
> > > > > index e5dae9343f..ebf3178b38 100644
> > > > > --- a/modules/codec/omxil/mediacodec.c
> > > > > +++ b/modules/codec/omxil/mediacodec.c
> > > > > @@ -571,6 +571,10 @@ static void CleanFromVideoContext(void *priv)
> > > > >
> > > > >    static void ReleaseAllPictureContexts(decoder_sys_t *p_sys)
> > > > >    {
> > > > > +    /* No picture context if no direct rendering. */
> > > > > +    if (p_sys->video.ctx == NULL)
> > > > > +        return;
> > > > > +
> > > > >        for (size_t i = 0; i < ARRAY_SIZE(p_sys->video.apic_ctxs); ++i)
> > > > >        {
> > > > >            struct android_picture_ctx *apctx = &p_sys->video.apic_ctxs[i];
> > > > > --
> > > > > 2.30.0
> > > > > _______________________________________________
> > > > > 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
> > >
> > _______________________________________________
> > 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