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

[Thomas Guillem via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

On Tue, Sep 12, 2023, at 15:11, Andreas Rheinhardt wrote:
> 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.

Thanks for your patches and your clear explanations !

>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".