[vlc-devel] [PATCH 1/2] avcodec: fix data-race

[Thomas Guillem]

On Fri, Sep 20, 2019, at 10:16, Steve Lhomme wrote:
> On 2019-09-20 9:31, Thomas Guillem wrote:
> > There was a data-race between any avcodec threads and the pf_decode thread. The
> > scope of sem wait/post need to be increased. Indeed, dec->fmt_out must be
> > accessed while being protected, the same for lavc_UpdateVideoFormat() and
> > decoder_NewPicture().
> > 
> > PS: decoder_QueuePicture() doesn't have to be protected, it's better to release
> > the sempahore before calling it in order to unlock avcodec threads while the
> > picture is being queued.
> 
> Aren't we supposed to queue pictures in display order ? In which case we 
> have to protect it from other decoder threads trying to queue future 
> pictures in our back.

Yes but for now, pictures are only queued from the pf_decode thread.

> 
> > ---
> >   modules/codec/avcodec/video.c | 19 ++++++++++++++++---
> >   1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/modules/codec/avcodec/video.c b/modules/codec/avcodec/video.c
> > index 2232b7a291..fea2a7979b 100644
> > --- a/modules/codec/avcodec/video.c
> > +++ b/modules/codec/avcodec/video.c
> > @@ -1120,6 +1120,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >                       b_error = true;
> >                   }
> >                   av_packet_unref( &pkt );
> > +                wait_mt( p_sys );
> >                   break;
> >               }
> >   
> > @@ -1142,6 +1143,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >           if (unlikely(frame == NULL))
> >           {
> >               b_error = true;
> > +            wait_mt( p_sys );
> >               break;
> >           }
> >   
> > @@ -1155,12 +1157,13 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               }
> >               av_frame_free(&frame);
> >               if( ret == AVERROR_EOF )
> > +            {
> > +                wait_mt( p_sys );
> >                   break;
> > +            }
> >           }
> >           bool not_received_frame = ret;
> >   
> > -        wait_mt( p_sys );
> > -
> >           if( p_block )
> >           {
> >               /* Consumed bytes */
> > @@ -1172,6 +1175,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >           if( not_received_frame )
> >           {
> >               av_frame_free(&frame);
> > +            wait_mt( p_sys );
> >               if( i_used == 0 ) break;
> >               continue;
> >           }
> > @@ -1207,6 +1211,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >                !p_sys->b_show_corrupted ) )
> >           {
> >               av_frame_free(&frame);
> > +            wait_mt( p_sys );
> >               continue;
> >           }
> >   
> > @@ -1225,6 +1230,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               {
> >                   b_error = true;
> >                   av_frame_free(&frame);
> > +                wait_mt( p_sys );
> >                   break;
> >               }
> >               static_assert( sizeof(p_palette->palette) == AVPALETTE_SIZE,
> > @@ -1236,6 +1242,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               if( decoder_UpdateVideoFormat( p_dec ) )
> >               {
> >                   av_frame_free(&frame);
> > +                wait_mt( p_sys );
> >                   continue;
> >               }
> >           }
> > @@ -1253,6 +1260,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               if( !p_pic )
> >               {
> >                   av_frame_free(&frame);
> > +                wait_mt( p_sys );
> >                   break;
> >               }
> >   
> > @@ -1261,6 +1269,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               {
> >                   av_frame_free(&frame);
> >                   picture_Release( p_pic );
> > +                wait_mt( p_sys );
> >                   break;
> >               }
> >           }
> > @@ -1275,6 +1284,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               if( unlikely(p_pic == NULL) )
> >               {
> >                   av_frame_free(&frame);
> > +                wait_mt( p_sys );
> >                   break;
> >               }
> >           }
> > @@ -1312,11 +1322,14 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >               if(p_frame_info->b_eos)
> >                   p_pic->b_still = true;
> >               p_sys->b_first_frame = false;
> > +            wait_mt( p_sys );
> >               decoder_QueueVideo( p_dec, p_pic );
> >           }
> >           else
> > +        {
> > +            wait_mt( p_sys );
> >               picture_Release( p_pic );
> > -
> > +        }
> >       } while( true );
> >   
> >       /* After draining, we need to reset decoder with a flush */
> > -- 
> > 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