[FFmpeg-devel,v2,13/13] cbs_mpeg2.c: improve readability of start code search (part 3)

Separate from part 2 for a clearer diff.

Now the true loop condition has been revealed: start < buf_end

Clarify loop by moving the detection of sequence_end_codes out of the loop.
See also commit fd93d5efe64206d5f1bce8c702602353444c0c1a regarding sequence_end_codes
---
 libavcodec/cbs_mpeg2.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Scott Theisen:
> Separate from part 2 for a clearer diff.
> 
> Now the true loop condition has been revealed: start < buf_end
> 
> Clarify loop by moving the detection of sequence_end_codes out of the loop.
> See also commit fd93d5efe64206d5f1bce8c702602353444c0c1a regarding sequence_end_codes
> ---
>  libavcodec/cbs_mpeg2.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index bf95fb7546..53aa0ed3f6 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -149,7 +149,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>      CodedBitstreamUnitType unit_type;
>      uint32_t start_code = -1;
>      size_t unit_size;
> -    int err, final = 0;
> +    int err;
>      int i = -1; // offset for pre-increment
>  
>      start = avpriv_find_start_code(start, buf_end, &start_code, 1);
> @@ -161,16 +161,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>      do {
>          unit_type = start_code & 0xff;
>  
> -        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
> -            // the next unit will be treated as the last unit.
> -            start_code = 0;
> -        }
> -        else {
> -            end = avpriv_find_start_code(start, buf_end, &start_code, 1);
> -        }
> +        end = avpriv_find_start_code(start, buf_end, &start_code, 1);
>          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
> @@ -182,8 +173,8 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>              unit_size = (end - 4) - start;
>          } else {
>             // We didn't find a start code, so this is the final unit.
> +           // There is no start code to remove from end, hence not (end - 4).
>             unit_size = end - start;
> -           final     = 1;
>          }
>  
>          err = ff_cbs_insert_unit_data(frag, ++i, unit_type,
> @@ -193,7 +184,21 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>              return err;
>  
>          start = end;
> -    } while (!final);
> +    } while (start < buf_end);
> +
> +    if (avpriv_start_code_is_valid(start_code)) {
> +        // The last four bytes form a start code which constitutes
> +        // a unit of its own, with size 1.
> +
> +        start--; // since start == buf_end because of the loop condition,
> +        // decrement so start points to the byte containing the start_code_identifier
> +        err = ff_cbs_insert_unit_data(frag, ++i, start_code & 0xFF,
> +                                      (uint8_t*)start /* cast away the const to match parameter type */,
> +                                      1, frag->data_ref);
> +        if (err < 0) {
> +            return err;
> +        }
> +    }
>  
>      return 0;
>  }

I disagree that this is the true loop condition that just needs to be
revealed; in fact, this is basically the loop condition from before
fd93d5efe64206d5f1bce8c702602353444c0c1a, just with an added block to
deal with size one units at the end. I considered this and chose the
current one because it leads to less code duplication for this special case.
Anyway, now that I have taken another look at this and I finally found
the true loop condition: Is there a unit that we have not added to the
fragment yet? This is easily implementable if one always resets the
start code, so that there being an outstanding unit is equivalent to the
start code being valid.

- Andreas

On 2/4/22 22:54, Andreas Rheinhardt wrote:
> I disagree that this is the true loop condition that just needs to be
> revealed; in fact, this is basically the loop condition from before
> fd93d5efe64206d5f1bce8c702602353444c0c1a, just with an added block to
> deal with size one units at the end. I considered this and chose the
> current one because it leads to less code duplication for this special case.
> Anyway, now that I have taken another look at this and I finally found
> the true loop condition: Is there a unit that we have not added to the
> fragment yet? This is easily implementable if one always resets the
> start code, so that there being an outstanding unit is equivalent to the
> start code being valid.

Looking at your patch series again, I agree your changes to cbs_mpeg2.c 
are clearer.

However, I think my changes from patch 11 are a further helpful 
clarification (ignoring the index and loop changes (since you already 
did that) and the "redundant" comment):

@@ -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).



Should I submit a v3 patch series which only includes patches 1-9? (That 
is only the avpriv_find_start_code() changes, since 10-13 were 
cbs_mpeg2.c and separate but related.)

Regards,
Scott