[FFmpeg-devel] [PATCH 4/7] avcodec/cbs_mpeg2: Simplify splitting fragment

[Scott Theisen]

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 at 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