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

[Alexandre Janniaux]

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. ;)

> 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