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

[James Almer]

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 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.
>>
> 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 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".
>