[vlc-devel] [PATCH 1/2] avcodec: fix data-races
Thomas Guillem
thomas at gllm.fr
Thu Sep 26 11:17:32 CEST 2019
On Thu, Sep 26, 2019, at 11:16, Hugo Beauzée-Luyssen wrote:
> On Thu, Sep 26, 2019, at 11:02 AM, Thomas Guillem wrote:
> > There was various data-races between any avcodec threads and the pf_decode
> > thread. The scope of sem wait/post need to be increased. Indeed, dec->fmt_out,
> > pts, p_va must be accessed while being protected, the same for
> > lavc_UpdateVideoFormat() and decoder_NewPicture().
> >
> > PS: decoder_QueuePicture() and picture_Release() don'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.
> >
> > WARNING: ThreadSanitizer: data race (pid=6962)
> > Write of size 4 at 0x7b800002001c by thread T14 (mutexes: write M499,
> > write M491):
> > #0 date_Change ../../src/misc/mtime.c:83 (libvlccore.so.9+0xcec72)
> > #1 lavc_UpdateVideoFormat ../../modules/codec/avcodec/video.c:357
> > (libavcodec_plugin.so+0xc50fa)
> > #2 lavc_GetFrame ../../modules/codec/avcodec/video.c:1598
> > (libavcodec_plugin.so+0xc8d0e)
> > #3 get_buffer_internal src/libavcodec/decode.c:1940
> > (libavcodec_plugin.so+0x12b4f2)
> > #4 ff_get_buffer src/libavcodec/decode.c:1965
> > (libavcodec_plugin.so+0x12b4f2)
> >
> > Previous read of size 4 at 0x7b800002001c by thread T18:
> > #0 date_Increment ../../src/misc/mtime.c:91
> > (libvlccore.so.9+0xceced)
> > #1 interpolate_next_pts ../../modules/codec/avcodec/video.c:798
> > (libavcodec_plugin.so+0xc4856)
> > #2 DecodeBlock ../../modules/codec/avcodec/video.c:1199
> > (libavcodec_plugin.so+0xc7f4e)
> > #3 DecodeVideo ../../modules/codec/avcodec/video.c:1357
> > (libavcodec_plugin.so+0xc8996)
> > #4 DecoderThread_DecodeBlock ../../src/input/decoder.c:1278
> > (libvlccore.so.9+0x5cac7)
> > #5 DecoderThread_ProcessInput ../../src/input/decoder.c:1400
> > (libvlccore.so.9+0x5ca99)
> > #6 DecoderThread ../../src/input/decoder.c:1676
> > (libvlccore.so.9+0x5cd69)
> >
> > WARNING: ThreadSanitizer: data race (pid=6962)
> > Atomic write of size 8 at 0x7b4c0002fc70 by thread T18:
> > #0 __tsan_atomic64_fetch_sub
> > ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cc:646
> > (libtsan.so.0+0x648dd)
> > #1 picture_Release ../../include/vlc_picture.h:203
> > (libavcodec_plugin.so+0xc6aa8)
> > #2 lavc_ReleaseFrame ../../modules/codec/avcodec/video.c:1461
> > (libavcodec_plugin.so+0xc6aa8)
> > #3 buffer_replace src/libavutil/buffer.c:120
> > (libavcodec_plugin.so+0x999596)
> > #4 av_buffer_unref src/libavutil/buffer.c:130
> > (libavcodec_plugin.so+0x999596)
> > #5 DecoderThread_Flush ../../src/input/decoder.c:1420
> > (libvlccore.so.9+0x58c8e)
> > #6 DecoderThread ../../src/input/decoder.c:1581
> > (libvlccore.so.9+0x5ce24)
> >
> > Previous write of size 8 at 0x7b4c0002fc70 by thread T12 (mutexes:
> > write M494, write M491):
> > #0 malloc
> > ../../../../src/libsanitizer/tsan/tsan_interceptors.cc:606
> > (libtsan.so.0+0x2b1a3)
> > #1 malloc
> > ../../../../src/libsanitizer/tsan/tsan_interceptors.cc:601
> > (libtsan.so.0+0x2b1a3)
> > #2 picture_NewPrivate ../../src/misc/picture.c:200
> > (libvlccore.so.9+0xd3239)
> > #3 picture_NewFromResource ../../src/misc/picture.c:226
> > (libvlccore.so.9+0xd338c)
> > #4 picture_pool_ClonePicture ../../src/misc/picture_pool.c:109
> > (libvlccore.so.9+0xd4ba2)
> > #5 picture_pool_Wait ../../src/misc/picture_pool.c:271
> > (libvlccore.so.9+0xd5375)
> > #6 vout_GetPicture ../../src/video_output/video_output.c:326
> > (libvlccore.so.9+0xa9c2f)
> > #7 ModuleThread_NewVideoBuffer ../../src/input/decoder.c:605
> > (libvlccore.so.9+0x5af28)
> > #8 decoder_NewPicture ../../src/input/decoder_helpers.c:93
> > (libvlccore.so.9+0x5f8b6)
> > #9 lavc_dr_GetFrame ../../modules/codec/avcodec/video.c:1504
> > (libavcodec_plugin.so+0xc6b44)
> > #10 lavc_GetFrame ../../modules/codec/avcodec/video.c:1611
> > (libavcodec_plugin.so+0xc8d47)
> > #11 get_buffer_internal src/libavcodec/decode.c:1940
> > (libavcodec_plugin.so+0x12b4f2)
> > #12 ff_get_buffer src/libavcodec/decode.c:1965
> > (libavcodec_plugin.so+0x12b4f2)
> >
> > WARNING: ThreadSanitizer: data race (pid=7336)
> > Read of size 4 at 0x7b8000021020 by thread T13 (mutexes: write M444,
> > write M438):
> > #0 date_Change ../../src/misc/mtime.c:81 (libvlccore.so.9+0xcec41)
> > #1 lavc_UpdateVideoFormat ../../modules/codec/avcodec/video.c:357
> > (libavcodec_plugin.so+0xc50fa)
> > #2 lavc_GetFrame ../../modules/codec/avcodec/video.c:1598
> > (libavcodec_plugin.so+0xc8d0e)
> > #3 get_buffer_internal src/libavcodec/decode.c:1940
> > (libavcodec_plugin.so+0x12b4f2)
> > #4 ff_get_buffer src/libavcodec/decode.c:1965
> > (libavcodec_plugin.so+0x12b4f2)
> >
> > Previous write of size 4 at 0x7b8000021020 by thread T18:
> > #0 date_Set ../../include/vlc_tick.h:260
> > (libavcodec_plugin.so+0xc7f38)
> > #1 DecodeBlock ../../modules/codec/avcodec/video.c:1197
> > (libavcodec_plugin.so+0xc7f38)
> > #2 DecodeVideo ../../modules/codec/avcodec/video.c:1357
> > (libavcodec_plugin.so+0xc8996)
> > #3 DecoderThread_DecodeBlock ../../src/input/decoder.c:1278
> > (libvlccore.so.9+0x5cac7)
> > #4 DecoderThread_ProcessInput ../../src/input/decoder.c:1400
> > (libvlccore.so.9+0x5ca99)
> > #5 DecoderThread ../../src/input/decoder.c:1676
> > (libvlccore.so.9+0x5cd69)
> > ---
> > modules/codec/avcodec/video.c | 26 ++++++++++++++++++++------
> > 1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/modules/codec/avcodec/video.c
> > b/modules/codec/avcodec/video.c
> > index 2232b7a291..d055325a6e 100644
> > --- a/modules/codec/avcodec/video.c
> > +++ b/modules/codec/avcodec/video.c
> > @@ -1180,6 +1180,8 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > if( p_frame_info->b_eos )
> > p_sys->b_first_frame = true;
> >
> > + post_mt( p_sys );
> > +
> > /* Compute the PTS */
> > #ifdef FF_API_PKT_PTS
> > int64_t av_pts = frame->pts == AV_NOPTS_VALUE ? frame->pkt_dts
> > : frame->pts;
> > @@ -1206,6 +1208,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > ( p_dec->b_frame_drop_allowed && (frame->flags &
> > AV_FRAME_FLAG_CORRUPT) &&
> > !p_sys->b_show_corrupted ) )
> > {
> > + wait_mt( p_sys );
> > av_frame_free(&frame);
> > continue;
> > }
> > @@ -1223,6 +1226,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > = malloc( sizeof(video_palette_t) );
> > if( !p_palette )
> > {
> > + wait_mt( p_sys );
>
> Nitpick, but b_error doesn't need to be protected no?
No, it's local variable of DecodeBlock().
>
> > b_error = true;
> > av_frame_free(&frame);
> > break;
> > @@ -1235,6 +1239,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > p_dec->fmt_out.video.i_chroma = VLC_CODEC_RGBP;
> > if( decoder_UpdateVideoFormat( p_dec ) )
> > {
> > + wait_mt( p_sys );
> > av_frame_free(&frame);
> > continue;
> > }
> > @@ -1252,6 +1257,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> >
> > if( !p_pic )
> > {
> > + wait_mt( p_sys );
> > av_frame_free(&frame);
> > break;
> > }
> > @@ -1259,6 +1265,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > /* Fill picture_t from AVFrame */
> > if( lavc_CopyPicture( p_dec, p_pic, frame ) != VLC_SUCCESS
> > )
> > {
> > + wait_mt( p_sys );
> > av_frame_free(&frame);
> > picture_Release( p_pic );
> > break;
> > @@ -1274,6 +1281,7 @@ static int DecodeBlock( decoder_t *p_dec, block_t
> > **pp_block )
> > p_pic = picture_Clone( p_pic );
> > if( unlikely(p_pic == NULL) )
> > {
> > + wait_mt( p_sys );
> > av_frame_free(&frame);
> > break;
> > }
> > @@ -1312,11 +1320,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 */
> > @@ -1601,14 +1612,18 @@ static int lavc_GetFrame(struct AVCodecContext
> > *ctx, AVFrame *frame, int flags)
> > return -1;
> > }
> > }
> > - post_mt(sys);
> >
> > if (sys->p_va != NULL)
> > - return lavc_va_GetFrame(ctx, frame);
> > + {
> > + int ret = lavc_va_GetFrame(ctx, frame);
> > + post_mt(sys);
> > + return ret;
> > + }
> >
> > /* Some codecs set pix_fmt only after the 1st frame has been decoded,
> > * so we need to check for direct rendering again. */
> > int ret = lavc_dr_GetFrame(ctx, frame);
> > + post_mt(sys);
> > if (ret)
> > ret = avcodec_default_get_buffer2(ctx, frame, flags);
> > return ret;
> > @@ -1740,12 +1755,11 @@ no_reuse:
> > test_pic ? test_pic->p_sys : NULL);
> > if (test_pic)
> > picture_Release(test_pic);
> > + wait_mt(p_sys);
> > if (va == NULL)
> > - {
> > - wait_mt(p_sys);
> > continue; /* Unsupported codec profile or such */
> > - }
> >
> > + post_mt(p_sys);
> > p_sys->p_va = va;
> > p_sys->pix_fmt = hwfmt;
> > p_context->draw_horiz_band = NULL;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Hugo Beauzée-Luyssen
> hugo at beauzee.fr
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list