[FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES

[Andreas Rheinhardt]

Thomas Guillem via ffmpeg-devel:
> Same than the previous commit but with FF_DECODE_ERROR_DECODE_SLICES
> 
> Fix the following data-race:
> 
> WARNING: ThreadSanitizer: data race (pid=55935)
>   Write of size 4 at 0x7b5000009378 by thread T1 (mutexes: write M608):
>     #0 decode_nal_units src/libavcodec/h264dec.c:742 (ffmpeg+0xb19dd6)
>     #1 h264_decode_frame src/libavcodec/h264dec.c:1016 (ffmpeg+0xb19dd6)
>     #2 frame_worker_thread src/libavcodec/pthread_frame.c:228 (ffmpeg+0xdeea7e)
> 
>   Previous read of size 4 at 0x7b5000009378 by thread T14 (mutexes: write M610):
>     #0 frame_copy_props src/libavutil/frame.c:321 (ffmpeg+0x1793759)
>     #1 av_frame_replace src/libavutil/frame.c:530 (ffmpeg+0x1794714)
>     #2 ff_thread_replace_frame src/libavcodec/utils.c:898 (ffmpeg+0xfb1d0f)
>     #3 ff_h264_replace_picture src/libavcodec/h264_picture.c:159 (ffmpeg+0x149cd3d)
>     #4 ff_h264_update_thread_context src/libavcodec/h264_slice.c:413 (ffmpeg+0x14abf04)
>     #5 update_context_from_thread src/libavcodec/pthread_frame.c:355 (ffmpeg+0xdec39c)
>     #6 submit_packet src/libavcodec/pthread_frame.c:494 (ffmpeg+0xdecee3)
>     #7 ff_thread_decode_frame src/libavcodec/pthread_frame.c:545 (ffmpeg+0xdecee3)
>     #8 decode_simple_internal src/libavcodec/decode.c:431 (ffmpeg+0x9e1e20)
>     #9 decode_simple_receive_frame src/libavcodec/decode.c:607 (ffmpeg+0x9e1e20)
>     #10 decode_receive_frame_internal src/libavcodec/decode.c:635 (ffmpeg+0x9e1e20)
>     #11 avcodec_send_packet src/libavcodec/decode.c:732 (ffmpeg+0x9e28fa)
>     #12 packet_decode src/fftools/ffmpeg_dec.c:555 (ffmpeg+0x229888)
>     #13 decoder_thread src/fftools/ffmpeg_dec.c:702 (ffmpeg+0x229888)
> ---
>  libavcodec/h264dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 24e849fc5b..b82ca8f14f 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -739,7 +739,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
>  
>      // set decode_error_flags to allow users to detect concealed decoding errors
>      if ((ret < 0 || h->er.error_occurred) && h->cur_pic_ptr) {
> -        h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
> +        h->cur_pic_ptr->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
>      }
>  
>      ret = 0;

IIRC this does not work: The thread that decodes a frame is typically
not the same thread that outputs said frame. The H264Picture srcp in
output_frame() points to one of the H264Picture in the H264Context.DBP
of the outputting thread, not the decoding thread. The outputting
threads decode_error_flags will therefore always be zero.

My preferred way to fix this is to allocate the H264Pictures (or at
least the stuff needed by later decoding threads) separately and make
them shared between decoder threads (with the caveat that only the
actual decoding threads may modify them; and they may only do so in a
controlled manner (i.e. no changes after having signalled to finish the
picture etc.). But this would be a major rewrite which will probably
never happen.

In the meantime i will send an alternative patch for this.

- Andreas