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

[Andreas Rheinhardt]

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.

>      } 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;
>  
>