diff mbox series

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

Message ID 20230912114015.59937-3-thomas@gllm.fr
State New
Headers show
Series [FFmpeg-devel,1/3] error_resilience: set the decode_error_flags outside | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Thomas Guillem Sept. 12, 2023, 11:40 a.m. UTC 
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(-)

Comments

Andreas Rheinhardt Sept. 12, 2023, 1:11 p.m. UTC | #1 
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
Thomas Guillem Sept. 13, 2023, 6:46 a.m. UTC | #2 
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".
diff mbox series

Patch

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;