diff mbox series

[FFmpeg-devel] avcodec/h264_parse: add some missing checks to ff_h264_init_poc()

Message ID 20210812012454.10595-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/h264_parse: add some missing checks to ff_h264_init_poc()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Aug. 12, 2021, 1:24 a.m. UTC
poc.delta_poc_bottom and poc.delta_poc[1] are only coded in the bitstream if
pps->pic_order_present is true, so ensure they are not used otherwise, to
prevent the potential use of stale values.

Signed-off-by: James Almer <jamrial@gmail.com>
---
This complements ce4a31cd1ff0348d279af74d49556d0315171e94, and is a more
thorough fix for the issue described in it, affecting all users of
ff_h264_init_poc(), like the parser, and not just the decoder.

 libavcodec/h264_parse.c  | 7 ++++---
 libavcodec/h264_parse.h  | 2 +-
 libavcodec/h264_parser.c | 2 +-
 libavcodec/h264_slice.c  | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt Aug. 12, 2021, 2:26 a.m. UTC | #1
James Almer:
> poc.delta_poc_bottom and poc.delta_poc[1] are only coded in the bitstream if
> pps->pic_order_present is true, so ensure they are not used otherwise, to
> prevent the potential use of stale values.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This complements ce4a31cd1ff0348d279af74d49556d0315171e94, and is a more
> thorough fix for the issue described in it, affecting all users of
> ff_h264_init_poc(), like the parser, and not just the decoder.
> 
>  libavcodec/h264_parse.c  | 7 ++++---
>  libavcodec/h264_parse.h  | 2 +-
>  libavcodec/h264_parser.c | 2 +-
>  libavcodec/h264_slice.c  | 2 +-
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
> index 1c1d1c04b0..5d642c7201 100644
> --- a/libavcodec/h264_parse.c
> +++ b/libavcodec/h264_parse.c
> @@ -275,9 +275,10 @@ fail:
>  }
>  
>  int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
> -                     const SPS *sps, H264POCContext *pc,
> +                     const PPS *pps, H264POCContext *pc,
>                       int picture_structure, int nal_ref_idc)
>  {
> +    const SPS *sps = pps->sps;
>      const int max_frame_num = 1 << sps->log2_max_frame_num;
>      int64_t field_poc[2];
>  
> @@ -300,7 +301,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>              pc->poc_msb = pc->prev_poc_msb;
>          field_poc[0] =
>          field_poc[1] = pc->poc_msb + pc->poc_lsb;
> -        if (picture_structure == PICT_FRAME)
> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>              field_poc[1] += pc->delta_poc_bottom;
>      } else if (sps->poc_type == 1) {
>          int abs_frame_num;
> @@ -336,7 +337,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>          field_poc[0] = expectedpoc + pc->delta_poc[0];
>          field_poc[1] = field_poc[0] + sps->offset_for_top_to_bottom_field;
>  
> -        if (picture_structure == PICT_FRAME)
> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>              field_poc[1] += pc->delta_poc[1];

delta_pic_order_cnt_bottom and both delta_pic_order_cnt elements are
supposed to be inferred to zero when they are not present. If this were
done, the above additions wouldn't make a difference. I thought that
ce4a31cd1ff0348d279af74d49556d0315171e94 actually did exactly that,
which makes me not understand the current patch.

>      } else {
>          int poc = 2 * (pc->frame_num_offset + pc->frame_num);
> diff --git a/libavcodec/h264_parse.h b/libavcodec/h264_parse.h
> index 4d01620125..35c4810f34 100644
> --- a/libavcodec/h264_parse.h
> +++ b/libavcodec/h264_parse.h
> @@ -78,7 +78,7 @@ int ff_h264_parse_ref_count(int *plist_count, int ref_count[2],
>                              int slice_type_nos, int picture_structure, void *logctx);
>  
>  int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
> -                     const SPS *sps, H264POCContext *poc,
> +                     const PPS *pps, H264POCContext *poc,
>                       int picture_structure, int nal_ref_idc);
>  
>  int ff_h264_decode_extradata(const uint8_t *data, int size, H264ParamSets *ps,
> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> index d3c56cc188..6e2e7cde67 100644
> --- a/libavcodec/h264_parser.c
> +++ b/libavcodec/h264_parser.c
> @@ -440,7 +440,7 @@ static inline int parse_nal_units(AVCodecParserContext *s,
>              /* Decode POC of this picture.
>               * The prev_ values needed for decoding POC of the next picture are not set here. */
>              field_poc[0] = field_poc[1] = INT_MAX;
> -            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, sps,
> +            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, p->ps.pps,
>                               &p->poc, p->picture_structure, nal.ref_idc);
>              if (ret < 0)
>                  goto fail;
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 0d7107d455..223cf21267 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1742,7 +1742,7 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl,
>      }
>  
>      ret = ff_h264_init_poc(h->cur_pic_ptr->field_poc, &h->cur_pic_ptr->poc,
> -                     h->ps.sps, &h->poc, h->picture_structure, nal->ref_idc);
> +                           h->ps.pps, &h->poc, h->picture_structure, nal->ref_idc);
>      if (ret < 0)
>          return ret;
>  
>
James Almer Aug. 12, 2021, 2:30 a.m. UTC | #2
On 8/11/2021 11:26 PM, Andreas Rheinhardt wrote:
> James Almer:
>> poc.delta_poc_bottom and poc.delta_poc[1] are only coded in the bitstream if
>> pps->pic_order_present is true, so ensure they are not used otherwise, to
>> prevent the potential use of stale values.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This complements ce4a31cd1ff0348d279af74d49556d0315171e94, and is a more
>> thorough fix for the issue described in it, affecting all users of
>> ff_h264_init_poc(), like the parser, and not just the decoder.
>>
>>   libavcodec/h264_parse.c  | 7 ++++---
>>   libavcodec/h264_parse.h  | 2 +-
>>   libavcodec/h264_parser.c | 2 +-
>>   libavcodec/h264_slice.c  | 2 +-
>>   4 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
>> index 1c1d1c04b0..5d642c7201 100644
>> --- a/libavcodec/h264_parse.c
>> +++ b/libavcodec/h264_parse.c
>> @@ -275,9 +275,10 @@ fail:
>>   }
>>   
>>   int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>> -                     const SPS *sps, H264POCContext *pc,
>> +                     const PPS *pps, H264POCContext *pc,
>>                        int picture_structure, int nal_ref_idc)
>>   {
>> +    const SPS *sps = pps->sps;
>>       const int max_frame_num = 1 << sps->log2_max_frame_num;
>>       int64_t field_poc[2];
>>   
>> @@ -300,7 +301,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>>               pc->poc_msb = pc->prev_poc_msb;
>>           field_poc[0] =
>>           field_poc[1] = pc->poc_msb + pc->poc_lsb;
>> -        if (picture_structure == PICT_FRAME)
>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>               field_poc[1] += pc->delta_poc_bottom;
>>       } else if (sps->poc_type == 1) {
>>           int abs_frame_num;
>> @@ -336,7 +337,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>>           field_poc[0] = expectedpoc + pc->delta_poc[0];
>>           field_poc[1] = field_poc[0] + sps->offset_for_top_to_bottom_field;
>>   
>> -        if (picture_structure == PICT_FRAME)
>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>               field_poc[1] += pc->delta_poc[1];
> 
> delta_pic_order_cnt_bottom and both delta_pic_order_cnt elements are
> supposed to be inferred to zero when they are not present. If this were
> done, the above additions wouldn't make a difference. I thought that
> ce4a31cd1ff0348d279af74d49556d0315171e94 actually did exactly that,
> which makes me not understand the current patch.

I explained it above. It complements ce4a31cd1f, being a more thorough 
fix that also affects the parser (where much like the decoder before the 
aforementioned commit, it's not inferring them to zero), by making this 
function not depend on the caller ensuring the POC struct is clean of 
stale values.

> 
>>       } else {
>>           int poc = 2 * (pc->frame_num_offset + pc->frame_num);
>> diff --git a/libavcodec/h264_parse.h b/libavcodec/h264_parse.h
>> index 4d01620125..35c4810f34 100644
>> --- a/libavcodec/h264_parse.h
>> +++ b/libavcodec/h264_parse.h
>> @@ -78,7 +78,7 @@ int ff_h264_parse_ref_count(int *plist_count, int ref_count[2],
>>                               int slice_type_nos, int picture_structure, void *logctx);
>>   
>>   int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>> -                     const SPS *sps, H264POCContext *poc,
>> +                     const PPS *pps, H264POCContext *poc,
>>                        int picture_structure, int nal_ref_idc);
>>   
>>   int ff_h264_decode_extradata(const uint8_t *data, int size, H264ParamSets *ps,
>> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
>> index d3c56cc188..6e2e7cde67 100644
>> --- a/libavcodec/h264_parser.c
>> +++ b/libavcodec/h264_parser.c
>> @@ -440,7 +440,7 @@ static inline int parse_nal_units(AVCodecParserContext *s,
>>               /* Decode POC of this picture.
>>                * The prev_ values needed for decoding POC of the next picture are not set here. */
>>               field_poc[0] = field_poc[1] = INT_MAX;
>> -            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, sps,
>> +            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, p->ps.pps,
>>                                &p->poc, p->picture_structure, nal.ref_idc);
>>               if (ret < 0)
>>                   goto fail;
>> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
>> index 0d7107d455..223cf21267 100644
>> --- a/libavcodec/h264_slice.c
>> +++ b/libavcodec/h264_slice.c
>> @@ -1742,7 +1742,7 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl,
>>       }
>>   
>>       ret = ff_h264_init_poc(h->cur_pic_ptr->field_poc, &h->cur_pic_ptr->poc,
>> -                     h->ps.sps, &h->poc, h->picture_structure, nal->ref_idc);
>> +                           h->ps.pps, &h->poc, h->picture_structure, nal->ref_idc);
>>       if (ret < 0)
>>           return ret;
>>   
>>
> 
> _______________________________________________
> 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".
>
Andreas Rheinhardt Aug. 12, 2021, 2:33 a.m. UTC | #3
James Almer:
> On 8/11/2021 11:26 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> poc.delta_poc_bottom and poc.delta_poc[1] are only coded in the
>>> bitstream if
>>> pps->pic_order_present is true, so ensure they are not used
>>> otherwise, to
>>> prevent the potential use of stale values.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> This complements ce4a31cd1ff0348d279af74d49556d0315171e94, and is a more
>>> thorough fix for the issue described in it, affecting all users of
>>> ff_h264_init_poc(), like the parser, and not just the decoder.
>>>
>>>   libavcodec/h264_parse.c  | 7 ++++---
>>>   libavcodec/h264_parse.h  | 2 +-
>>>   libavcodec/h264_parser.c | 2 +-
>>>   libavcodec/h264_slice.c  | 2 +-
>>>   4 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
>>> index 1c1d1c04b0..5d642c7201 100644
>>> --- a/libavcodec/h264_parse.c
>>> +++ b/libavcodec/h264_parse.c
>>> @@ -275,9 +275,10 @@ fail:
>>>   }
>>>     int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>>> -                     const SPS *sps, H264POCContext *pc,
>>> +                     const PPS *pps, H264POCContext *pc,
>>>                        int picture_structure, int nal_ref_idc)
>>>   {
>>> +    const SPS *sps = pps->sps;
>>>       const int max_frame_num = 1 << sps->log2_max_frame_num;
>>>       int64_t field_poc[2];
>>>   @@ -300,7 +301,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int
>>> *pic_poc,
>>>               pc->poc_msb = pc->prev_poc_msb;
>>>           field_poc[0] =
>>>           field_poc[1] = pc->poc_msb + pc->poc_lsb;
>>> -        if (picture_structure == PICT_FRAME)
>>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>>               field_poc[1] += pc->delta_poc_bottom;
>>>       } else if (sps->poc_type == 1) {
>>>           int abs_frame_num;
>>> @@ -336,7 +337,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int
>>> *pic_poc,
>>>           field_poc[0] = expectedpoc + pc->delta_poc[0];
>>>           field_poc[1] = field_poc[0] +
>>> sps->offset_for_top_to_bottom_field;
>>>   -        if (picture_structure == PICT_FRAME)
>>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>>               field_poc[1] += pc->delta_poc[1];
>>
>> delta_pic_order_cnt_bottom and both delta_pic_order_cnt elements are
>> supposed to be inferred to zero when they are not present. If this were
>> done, the above additions wouldn't make a difference. I thought that
>> ce4a31cd1ff0348d279af74d49556d0315171e94 actually did exactly that,
>> which makes me not understand the current patch.
> 
> I explained it above. It complements ce4a31cd1f, being a more thorough
> fix that also affects the parser (where much like the decoder before the
> aforementioned commit, it's not inferring them to zero), by making this
> function not depend on the caller ensuring the POC struct is clean of
> stale values.
> 
So this is supposed to help callers who do not use our slice parsing
code, but rather some other code that does not properly infer values for
elements that are absent?

- Andreas
James Almer Aug. 12, 2021, 2:37 a.m. UTC | #4
On 8/11/2021 11:33 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 8/11/2021 11:26 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> poc.delta_poc_bottom and poc.delta_poc[1] are only coded in the
>>>> bitstream if
>>>> pps->pic_order_present is true, so ensure they are not used
>>>> otherwise, to
>>>> prevent the potential use of stale values.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> This complements ce4a31cd1ff0348d279af74d49556d0315171e94, and is a more
>>>> thorough fix for the issue described in it, affecting all users of
>>>> ff_h264_init_poc(), like the parser, and not just the decoder.
>>>>
>>>>    libavcodec/h264_parse.c  | 7 ++++---
>>>>    libavcodec/h264_parse.h  | 2 +-
>>>>    libavcodec/h264_parser.c | 2 +-
>>>>    libavcodec/h264_slice.c  | 2 +-
>>>>    4 files changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
>>>> index 1c1d1c04b0..5d642c7201 100644
>>>> --- a/libavcodec/h264_parse.c
>>>> +++ b/libavcodec/h264_parse.c
>>>> @@ -275,9 +275,10 @@ fail:
>>>>    }
>>>>      int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
>>>> -                     const SPS *sps, H264POCContext *pc,
>>>> +                     const PPS *pps, H264POCContext *pc,
>>>>                         int picture_structure, int nal_ref_idc)
>>>>    {
>>>> +    const SPS *sps = pps->sps;
>>>>        const int max_frame_num = 1 << sps->log2_max_frame_num;
>>>>        int64_t field_poc[2];
>>>>    @@ -300,7 +301,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int
>>>> *pic_poc,
>>>>                pc->poc_msb = pc->prev_poc_msb;
>>>>            field_poc[0] =
>>>>            field_poc[1] = pc->poc_msb + pc->poc_lsb;
>>>> -        if (picture_structure == PICT_FRAME)
>>>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>>>                field_poc[1] += pc->delta_poc_bottom;
>>>>        } else if (sps->poc_type == 1) {
>>>>            int abs_frame_num;
>>>> @@ -336,7 +337,7 @@ int ff_h264_init_poc(int pic_field_poc[2], int
>>>> *pic_poc,
>>>>            field_poc[0] = expectedpoc + pc->delta_poc[0];
>>>>            field_poc[1] = field_poc[0] +
>>>> sps->offset_for_top_to_bottom_field;
>>>>    -        if (picture_structure == PICT_FRAME)
>>>> +        if (pps->pic_order_present && picture_structure == PICT_FRAME)
>>>>                field_poc[1] += pc->delta_poc[1];
>>>
>>> delta_pic_order_cnt_bottom and both delta_pic_order_cnt elements are
>>> supposed to be inferred to zero when they are not present. If this were
>>> done, the above additions wouldn't make a difference. I thought that
>>> ce4a31cd1ff0348d279af74d49556d0315171e94 actually did exactly that,
>>> which makes me not understand the current patch.
>>
>> I explained it above. It complements ce4a31cd1f, being a more thorough
>> fix that also affects the parser (where much like the decoder before the
>> aforementioned commit, it's not inferring them to zero), by making this
>> function not depend on the caller ensuring the POC struct is clean of
>> stale values.
>>
> So this is supposed to help callers who do not use our slice parsing
> code, but rather some other code that does not properly infer values for
> elements that are absent?

Like the parser, yes. It's independent of the decoder, and reimplements 
slice header parsing.
I was going to send a patch to infer these values to 0 on it too like i 
did for the decoder, but i figured this was a better fix, adding the 
proper syntax checks from the spec.

> 
> - 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/h264_parse.c b/libavcodec/h264_parse.c
index 1c1d1c04b0..5d642c7201 100644
--- a/libavcodec/h264_parse.c
+++ b/libavcodec/h264_parse.c
@@ -275,9 +275,10 @@  fail:
 }
 
 int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
-                     const SPS *sps, H264POCContext *pc,
+                     const PPS *pps, H264POCContext *pc,
                      int picture_structure, int nal_ref_idc)
 {
+    const SPS *sps = pps->sps;
     const int max_frame_num = 1 << sps->log2_max_frame_num;
     int64_t field_poc[2];
 
@@ -300,7 +301,7 @@  int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
             pc->poc_msb = pc->prev_poc_msb;
         field_poc[0] =
         field_poc[1] = pc->poc_msb + pc->poc_lsb;
-        if (picture_structure == PICT_FRAME)
+        if (pps->pic_order_present && picture_structure == PICT_FRAME)
             field_poc[1] += pc->delta_poc_bottom;
     } else if (sps->poc_type == 1) {
         int abs_frame_num;
@@ -336,7 +337,7 @@  int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
         field_poc[0] = expectedpoc + pc->delta_poc[0];
         field_poc[1] = field_poc[0] + sps->offset_for_top_to_bottom_field;
 
-        if (picture_structure == PICT_FRAME)
+        if (pps->pic_order_present && picture_structure == PICT_FRAME)
             field_poc[1] += pc->delta_poc[1];
     } else {
         int poc = 2 * (pc->frame_num_offset + pc->frame_num);
diff --git a/libavcodec/h264_parse.h b/libavcodec/h264_parse.h
index 4d01620125..35c4810f34 100644
--- a/libavcodec/h264_parse.h
+++ b/libavcodec/h264_parse.h
@@ -78,7 +78,7 @@  int ff_h264_parse_ref_count(int *plist_count, int ref_count[2],
                             int slice_type_nos, int picture_structure, void *logctx);
 
 int ff_h264_init_poc(int pic_field_poc[2], int *pic_poc,
-                     const SPS *sps, H264POCContext *poc,
+                     const PPS *pps, H264POCContext *poc,
                      int picture_structure, int nal_ref_idc);
 
 int ff_h264_decode_extradata(const uint8_t *data, int size, H264ParamSets *ps,
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index d3c56cc188..6e2e7cde67 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -440,7 +440,7 @@  static inline int parse_nal_units(AVCodecParserContext *s,
             /* Decode POC of this picture.
              * The prev_ values needed for decoding POC of the next picture are not set here. */
             field_poc[0] = field_poc[1] = INT_MAX;
-            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, sps,
+            ret = ff_h264_init_poc(field_poc, &s->output_picture_number, p->ps.pps,
                              &p->poc, p->picture_structure, nal.ref_idc);
             if (ret < 0)
                 goto fail;
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 0d7107d455..223cf21267 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1742,7 +1742,7 @@  static int h264_field_start(H264Context *h, const H264SliceContext *sl,
     }
 
     ret = ff_h264_init_poc(h->cur_pic_ptr->field_poc, &h->cur_pic_ptr->poc,
-                     h->ps.sps, &h->poc, h->picture_structure, nal->ref_idc);
+                           h->ps.pps, &h->poc, h->picture_structure, nal->ref_idc);
     if (ret < 0)
         return ret;