| Message ID | AM7PR03MB666042964502BE402BAE58658F299@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avcodec/cbs_mpeg2: Remove redundant counter | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 2/4/22 10:16, Andreas Rheinhardt wrote: > avpriv_find_start_code() supports non-contiguous buffers > by maintaining a state that allows to find start codes > that span across multiple buffers; a consequence thereof > is that avpriv_find_start_code() is given a zero-sized > buffer, it does not modify this state, so that it appears > as if a start code was found if the state contained a start code. > > This can e.g. happen with Sequence End units in MPEG-2 and > to counter this, cbs_mpeg2_split_fragment() reset the state > when it has already encountered the end of the fragment > in order to add the last unit (if it is only of the form 00 00 01 xy) > only once; it also used a flag to set whether this is the final unit. > > Yet this can be improved by simply resetting state unconditionally > (thereby avoiding a branch); the flag can be removed by just checking > whether we have a valid start code (of the next unit to add) > at the end. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/cbs_mpeg2.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index 2bf38c6001..90d667ddc8 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -149,7 +149,6 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > uint32_t start_code = -1; > size_t unit_size; > int err; > - int final = 0; > > start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, > &start_code); > @@ -161,14 +160,11 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > do { > unit_type = start_code & 0xff; > > - if (start == frag->data + frag->data_size) { > - // The last four bytes form a start code which constitutes > - // a unit of its own. In this situation avpriv_find_start_code > - // won't modify start_code at all so modify start_code so that > - // the next unit will be treated as the last unit. > - start_code = 0; > - } > - > + // Reset start_code to ensure that avpriv_find_start_code() > + // really reads a new start code and does not reuse the old > + // start code in any way (as e.g. happens when there is a > + // Sequence End unit at the very end of a packet). > + start_code = UINT32_MAX; > end = avpriv_find_start_code(start--, frag->data + frag->data_size, > &start_code); > > @@ -183,7 +179,6 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > } else { > // We didn't find a start code, so this is the final unit. > unit_size = end - start; > - final = 1; > } > > err = ff_cbs_append_unit_data(frag, unit_type, (uint8_t*)start, > @@ -192,7 +187,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > return err; > > start = end; > - } while (!final); > + } while ((start_code >> 8) == 0x000001); > > return 0; > } I assume this is inspired by my patch series. Did you see v2 of my patch series where I made similar and more extensive clarifications? The check for sequence_end_codes should not be in the loop. It harms readability, although you also eliminated the branch. Regards, Scott
On 2/4/22 14:01, Scott Theisen wrote: > > I assume this is inspired by my patch series. Did you see v2 of my > patch series where I made similar and more extensive clarifications? > > The check for sequence_end_codes should not be in the loop. It harms > readability, although you also eliminated the branch. I retract my comments. -Scott
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 2bf38c6001..90d667ddc8 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -149,7 +149,6 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, uint32_t start_code = -1; size_t unit_size; int err; - int final = 0; start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, &start_code); @@ -161,14 +160,11 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, do { unit_type = start_code & 0xff; - if (start == frag->data + frag->data_size) { - // The last four bytes form a start code which constitutes - // a unit of its own. In this situation avpriv_find_start_code - // won't modify start_code at all so modify start_code so that - // the next unit will be treated as the last unit. - start_code = 0; - } - + // Reset start_code to ensure that avpriv_find_start_code() + // really reads a new start code and does not reuse the old + // start code in any way (as e.g. happens when there is a + // Sequence End unit at the very end of a packet). + start_code = UINT32_MAX; end = avpriv_find_start_code(start--, frag->data + frag->data_size, &start_code); @@ -183,7 +179,6 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, } else { // We didn't find a start code, so this is the final unit. unit_size = end - start; - final = 1; } err = ff_cbs_append_unit_data(frag, unit_type, (uint8_t*)start, @@ -192,7 +187,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, return err; start = end; - } while (!final); + } while ((start_code >> 8) == 0x000001); return 0; }
avpriv_find_start_code() supports non-contiguous buffers by maintaining a state that allows to find start codes that span across multiple buffers; a consequence thereof is that avpriv_find_start_code() is given a zero-sized buffer, it does not modify this state, so that it appears as if a start code was found if the state contained a start code. This can e.g. happen with Sequence End units in MPEG-2 and to counter this, cbs_mpeg2_split_fragment() reset the state when it has already encountered the end of the fragment in order to add the last unit (if it is only of the form 00 00 01 xy) only once; it also used a flag to set whether this is the final unit. Yet this can be improved by simply resetting state unconditionally (thereby avoiding a branch); the flag can be removed by just checking whether we have a valid start code (of the next unit to add) at the end. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/cbs_mpeg2.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)