| 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() | expand |
| 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 |
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; > >
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". >
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
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 --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;
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(-)