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

[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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>