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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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;
>  }

_______________________________________________
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".