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

[Andreas Rheinhardt]

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