| Message ID | 20201027212506.21925-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/cbs_av1: add a range check to tg_end | 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 |
On 10/27/2020 6:25 PM, James Almer wrote: > Section 6.10.1 of the AV1 spec states: > > It is a requirement of bitstream conformance that the value of tg_start is > equal to the value of TileNum at the point that tile_group_obu is invoked. > It is a requirement of bitstream conformance that the value of tg_end is > greater than or equal to tg_start. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Not sure if we should clear priv->tile_num in other places, too. Ping. > > libavcodec/cbs_av1.h | 1 + > libavcodec/cbs_av1_syntax_template.c | 8 ++++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h > index a2d78e736f..386774750a 100644 > --- a/libavcodec/cbs_av1.h > +++ b/libavcodec/cbs_av1.h > @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context { > int all_lossless; > int tile_cols; > int tile_rows; > + int tile_num; > > AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES]; > } CodedBitstreamAV1Context; > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c > index 2df5619279..f0be7aab59 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -1719,6 +1719,8 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw, > > CHECK(FUNC(uncompressed_header)(ctx, rw, current)); > > + priv->tile_num = 0; > + > if (current->show_existing_frame) { > priv->seen_frame_header = 0; > } else { > @@ -1784,10 +1786,12 @@ static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw, > } else { > tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) + > cbs_av1_tile_log2(1, priv->tile_rows); > - fb(tile_bits, tg_start); > - fb(tile_bits, tg_end); > + fc(tile_bits, tg_start, priv->tile_num, priv->tile_num); > + fc(tile_bits, tg_end, current->tg_start, num_tiles - 1); > } > > + priv->tile_num += current->tg_end - current->tg_start + 1; > + > CHECK(FUNC(byte_alignment)(ctx, rw)); > > // Reset header for next frame. >
On 27/10/2020 21:25, James Almer wrote: > Section 6.10.1 of the AV1 spec states: > > It is a requirement of bitstream conformance that the value of tg_start is > equal to the value of TileNum at the point that tile_group_obu is invoked. > It is a requirement of bitstream conformance that the value of tg_end is > greater than or equal to tg_start. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Not sure if we should clear priv->tile_num in other places, too. > > libavcodec/cbs_av1.h | 1 + > libavcodec/cbs_av1_syntax_template.c | 8 ++++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h > index a2d78e736f..386774750a 100644 > --- a/libavcodec/cbs_av1.h > +++ b/libavcodec/cbs_av1.h > @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context { > int all_lossless; > int tile_cols; > int tile_rows; > + int tile_num; > > AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES]; > } CodedBitstreamAV1Context; > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c > index 2df5619279..f0be7aab59 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -1719,6 +1719,8 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw, > > CHECK(FUNC(uncompressed_header)(ctx, rw, current)); > > + priv->tile_num = 0; > + > if (current->show_existing_frame) { > priv->seen_frame_header = 0; > } else { > @@ -1784,10 +1786,12 @@ static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw, > } else { > tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) + > cbs_av1_tile_log2(1, priv->tile_rows); > - fb(tile_bits, tg_start); > - fb(tile_bits, tg_end); > + fc(tile_bits, tg_start, priv->tile_num, priv->tile_num); This isn't true if you lose a tile group OBU making up part of a frame, which we should be able to handle as something better than just a parsing failure in error resilient mode. I think set the lower bound only and let the upper bound be num_tiles - 1? > + fc(tile_bits, tg_end, current->tg_start, num_tiles - 1); > } > > + priv->tile_num += current->tg_end - current->tg_start + 1; priv->tile_num = current->tg_end + 1; > + > CHECK(FUNC(byte_alignment)(ctx, rw)); > > // Reset header for next frame. > Thanks, - Mark
On 11/10/2020 7:36 PM, Mark Thompson wrote: > On 27/10/2020 21:25, James Almer wrote: >> Section 6.10.1 of the AV1 spec states: >> >> It is a requirement of bitstream conformance that the value of >> tg_start is >> equal to the value of TileNum at the point that tile_group_obu is >> invoked. >> It is a requirement of bitstream conformance that the value of tg_end is >> greater than or equal to tg_start. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Not sure if we should clear priv->tile_num in other places, too. >> >> libavcodec/cbs_av1.h | 1 + >> libavcodec/cbs_av1_syntax_template.c | 8 ++++++-- >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h >> index a2d78e736f..386774750a 100644 >> --- a/libavcodec/cbs_av1.h >> +++ b/libavcodec/cbs_av1.h >> @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context { >> int all_lossless; >> int tile_cols; >> int tile_rows; >> + int tile_num; >> AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES]; >> } CodedBitstreamAV1Context; >> diff --git a/libavcodec/cbs_av1_syntax_template.c >> b/libavcodec/cbs_av1_syntax_template.c >> index 2df5619279..f0be7aab59 100644 >> --- a/libavcodec/cbs_av1_syntax_template.c >> +++ b/libavcodec/cbs_av1_syntax_template.c >> @@ -1719,6 +1719,8 @@ static int >> FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw, >> CHECK(FUNC(uncompressed_header)(ctx, rw, current)); >> + priv->tile_num = 0; >> + >> if (current->show_existing_frame) { >> priv->seen_frame_header = 0; >> } else { >> @@ -1784,10 +1786,12 @@ static int >> FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw, >> } else { >> tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) + >> cbs_av1_tile_log2(1, priv->tile_rows); >> - fb(tile_bits, tg_start); >> - fb(tile_bits, tg_end); >> + fc(tile_bits, tg_start, priv->tile_num, priv->tile_num); > > This isn't true if you lose a tile group OBU making up part of a frame, > which we should be able to handle as something better than just a > parsing failure in error resilient mode. > > I think set the lower bound only and let the upper bound be num_tiles - 1? Done and pushed. I suppose the CBS user should check the frame's integrity then? > >> + fc(tile_bits, tg_end, current->tg_start, num_tiles - 1); >> } >> + priv->tile_num += current->tg_end - current->tg_start + 1; > > priv->tile_num = current->tg_end + 1; > >> + >> CHECK(FUNC(byte_alignment)(ctx, rw)); >> // Reset header for next frame. >> > > Thanks, > > - Mark > _______________________________________________ > 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/cbs_av1.h b/libavcodec/cbs_av1.h index a2d78e736f..386774750a 100644 --- a/libavcodec/cbs_av1.h +++ b/libavcodec/cbs_av1.h @@ -446,6 +446,7 @@ typedef struct CodedBitstreamAV1Context { int all_lossless; int tile_cols; int tile_rows; + int tile_num; AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES]; } CodedBitstreamAV1Context; diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c index 2df5619279..f0be7aab59 100644 --- a/libavcodec/cbs_av1_syntax_template.c +++ b/libavcodec/cbs_av1_syntax_template.c @@ -1719,6 +1719,8 @@ static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw, CHECK(FUNC(uncompressed_header)(ctx, rw, current)); + priv->tile_num = 0; + if (current->show_existing_frame) { priv->seen_frame_header = 0; } else { @@ -1784,10 +1786,12 @@ static int FUNC(tile_group_obu)(CodedBitstreamContext *ctx, RWContext *rw, } else { tile_bits = cbs_av1_tile_log2(1, priv->tile_cols) + cbs_av1_tile_log2(1, priv->tile_rows); - fb(tile_bits, tg_start); - fb(tile_bits, tg_end); + fc(tile_bits, tg_start, priv->tile_num, priv->tile_num); + fc(tile_bits, tg_end, current->tg_start, num_tiles - 1); } + priv->tile_num += current->tg_end - current->tg_start + 1; + CHECK(FUNC(byte_alignment)(ctx, rw)); // Reset header for next frame.
Section 6.10.1 of the AV1 spec states: It is a requirement of bitstream conformance that the value of tg_start is equal to the value of TileNum at the point that tile_group_obu is invoked. It is a requirement of bitstream conformance that the value of tg_end is greater than or equal to tg_start. Signed-off-by: James Almer <jamrial@gmail.com> --- Not sure if we should clear priv->tile_num in other places, too. libavcodec/cbs_av1.h | 1 + libavcodec/cbs_av1_syntax_template.c | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-)