| Message ID | 20220203184450.5491-12-scott.the.elm@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rewrite avpriv_find_start_code() for clarity | expand |
Scott Theisen: > ff_cbs_insert_unit_data() does not modify the data or data_size fields of > the CodedBitstreamFragment. It also does not modify the value pointed to > by start. (We don't need that byte in this function anymore, anyway.) > --- > libavcodec/cbs_mpeg2.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index afe78eef9a..f2efedde5d 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -144,23 +144,24 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > int header) > { > - const uint8_t *start, *end; > + const uint8_t *start = frag->data, *end; > + const uint8_t * const buf_end = frag->data + frag->data_size; > CodedBitstreamUnitType unit_type; > uint32_t start_code = -1; > size_t unit_size; > - int err, i = 0, final = 0; > + int err, final = 0; > + int i = -1; // offset for pre-increment Using a pre-increment is unnatural (i is supposed to be the number of units of the fragment and so it should naturally be incremented after a unit has been successfully added to the fragment) and impairs clarity. > > - start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, > - &start_code, 1); > + start = avpriv_find_start_code(start, buf_end, &start_code, 1); > if (!avpriv_start_code_is_valid(start_code)) { > // No start code found. > return AVERROR_INVALIDDATA; > } > > - while (!final) { > + do { > unit_type = start_code & 0xff; > > - if (start == frag->data + frag->data_size) { > + if (start == buf_end) { > // 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 > @@ -168,10 +169,9 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > start_code = 0; > } > > - end = avpriv_find_start_code(start--, frag->data + frag->data_size, > - &start_code, 0); > - > - // start points to the byte containing the start_code_identifier > + end = avpriv_find_start_code(start, buf_end, &start_code, 0); > + start--; > + // decrement so start points to the byte containing the start_code_identifier > // (may be the last byte of fragment->data); end points to the byte > // following the byte containing the start code identifier (or to > // the end of fragment->data). > @@ -185,14 +185,14 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > final = 1; > } > > - err = ff_cbs_insert_unit_data(frag, i, unit_type, (uint8_t*)start, > + err = ff_cbs_insert_unit_data(frag, ++i, unit_type, > + (uint8_t*)start /* cast away the const to match parameter type */, redundant comment > unit_size, frag->data_ref); > if (err < 0) > return err; > > start = end; > - i++; > - } > + } while (!final); > > return 0; > }
On 2/4/22 21:16, Andreas Rheinhardt wrote: > Using a pre-increment is unnatural (i is supposed to be the number of > units of the fragment and so it should naturally be incremented after a > unit has been successfully added to the fragment) and impairs clarity. > It *is* incremented after a unit has been successfully added, on the *next* iteration. > redundant comment > Debatable, the variable is already a uint8_t*, albeit const qualified. The purpose of comments is to explain.
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index afe78eef9a..f2efedde5d 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -144,23 +144,24 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, int header) { - const uint8_t *start, *end; + const uint8_t *start = frag->data, *end; + const uint8_t * const buf_end = frag->data + frag->data_size; CodedBitstreamUnitType unit_type; uint32_t start_code = -1; size_t unit_size; - int err, i = 0, final = 0; + int err, final = 0; + int i = -1; // offset for pre-increment - start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, - &start_code, 1); + start = avpriv_find_start_code(start, buf_end, &start_code, 1); if (!avpriv_start_code_is_valid(start_code)) { // No start code found. return AVERROR_INVALIDDATA; } - while (!final) { + do { unit_type = start_code & 0xff; - if (start == frag->data + frag->data_size) { + if (start == buf_end) { // 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 @@ -168,10 +169,9 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, start_code = 0; } - end = avpriv_find_start_code(start--, frag->data + frag->data_size, - &start_code, 0); - - // start points to the byte containing the start_code_identifier + end = avpriv_find_start_code(start, buf_end, &start_code, 0); + start--; + // decrement so start points to the byte containing the start_code_identifier // (may be the last byte of fragment->data); end points to the byte // following the byte containing the start code identifier (or to // the end of fragment->data). @@ -185,14 +185,14 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, final = 1; } - err = ff_cbs_insert_unit_data(frag, i, unit_type, (uint8_t*)start, + err = ff_cbs_insert_unit_data(frag, ++i, unit_type, + (uint8_t*)start /* cast away the const to match parameter type */, unit_size, frag->data_ref); if (err < 0) return err; start = end; - i++; - } + } while (!final); return 0; }