[vlc-devel] [PATCH] decoder_helpers: fix clean order

[Thomas Guillem]

On Wed, Feb 20, 2019, at 11:16, Steve Lhomme wrote:
> On 20/02/2019 10:27, Thomas Guillem wrote:
> > On Wed, Feb 20, 2019, at 10:09, Steve Lhomme wrote:
> >> On 19/02/2019 13:24, Thomas Guillem wrote:
> >>> The module need to be unloaded before the dec struct is cleaned.
> >>>
> >>> This fixes an undefined behavior with threaded decoders like avcodec when they
> >>> called decoder_UpdateVideoFormat()/dec_NewPicture() from an other thread just
> >>> before they were unloaded.
> >> Are there cases where the decoder is still calling
> >> decoder_UpdateVideoFormat() and decoder_NewPicture() while being about
> >> to be unloaded ?
> > It is the case with avcodec with multi thread or every asynchronous decoders (MediaCodec/VideoToolBox).
> 
> OK, after double checking it seems they are called in the decoder loop 
> and in the decoder threads.
> 
> >
> >> I don't see how this can work if one thread of the
> >> decoder is calling decoder_UpdateVideoFormat() while another thread is
> >> unloading the decoder DLLs. If that can happen I think the undefined
> >> behavior is the least of our problem.
> > This won't happen since decoder implementations use mutex or semaphores to prevent that.
> 
> While this is the case for our favorite input decoder, it doesn't exist 
> in mosaic_bridge, SDIStream or the image decoder. That's typically the 
> kind of thing that should be factorized outside of the decoder.

When I said decoder implementation, I was talking about decoder modules (avocec/MediaCodec/Videotoolbox are using synchronization mechanism), so it doesn't depend on input/decoder.c

> 
> That means changing the dec->fmt_out in decoder_UpdateVideoFormat() is 
> not possible since it's not protected by a lock. I did add a lock to 
> protect the pool/context, it should be used for that too.
> 
> >
> >> AFAIK lavc always call decoder_UpdateVideoFormat()/decoder_NewPicture()
> >> from the same (decoder?) thread.
> >>
> >> This is an important thing because the decoder pool will be released
> >> there as well as the video context reference count.
> >>
> >>> ---
> >>>    src/input/decoder_helpers.c | 11 ++++++-----
> >>>    1 file changed, 6 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/input/decoder_helpers.c b/src/input/decoder_helpers.c
> >>> index 3cc02de8d6..b0a7c1890a 100644
> >>> --- a/src/input/decoder_helpers.c
> >>> +++ b/src/input/decoder_helpers.c
> >>> @@ -48,6 +48,12 @@ void decoder_Init( decoder_t *p_dec, const es_format_t *restrict p_fmt )
> >>>    
> >>>    void decoder_Clean( decoder_t *p_dec )
> >>>    {
> >>> +    if ( p_dec->p_module != NULL )
> >>> +    {
> >>> +        module_unneed(p_dec, p_dec->p_module);
> >>> +        p_dec->p_module = NULL;
> >>> +    }
> >>> +
> >>>        es_format_Clean( &p_dec->fmt_in );
> >>>        es_format_Clean( &p_dec->fmt_out );
> >>>    
> >>> @@ -56,11 +62,6 @@ void decoder_Clean( decoder_t *p_dec )
> >>>            vlc_meta_Delete(p_dec->p_description);
> >>>            p_dec->p_description = NULL;
> >>>        }
> >>> -    if ( p_dec->p_module != NULL )
> >>> -    {
> >>> -        module_unneed(p_dec, p_dec->p_module);
> >>> -        p_dec->p_module = NULL;
> >>> -    }
> >>>    }
> >>>    
> >>>    void decoder_Destroy( decoder_t *p_dec )
> >>> -- 
> >>> 2.20.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
> > _______________________________________________
> > 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