[FFmpeg-devel] [PATCH 1/5] avformat/movenc: split MPEG-4 bit rate value calculation

[Martin Storsjö]

On Sun, 20 Sep 2020, Jan Ekström wrote:

> This can now be re-utilized in other places.
> ---
> libavformat/movenc.c | 39 +++++++++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 12471c7d68..33331962f2 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -633,12 +633,36 @@ static unsigned compute_avg_bitrate(MOVTrack *track)
>     return size * 8 * track->timescale / track->track_duration;
> }
> 
> +struct mpeg4_bit_rate_values {
> +    uint32_t buffer_size;  ///< Size of the decoding buffer for the elementary stream in bytes.
> +    uint32_t max_bit_rate; ///< Maximum rate in bits/second over any window of one second.
> +    uint32_t avg_bit_rate; ///< Average rate in bits/second over the entire presentation.
> +};
> +
> +static struct mpeg4_bit_rate_values calculate_mpeg4_bit_rates(MOVTrack *track)
> +{
> +    AVCPBProperties *props = \

There's a few stray backslashes here - that look like remnants from trying 
to do this function as a plain macro

> +        (AVCPBProperties*)av_stream_get_side_data(track->st,
> +                                                  AV_PKT_DATA_CPB_PROPERTIES,
> +                                                  NULL);
> +    unsigned avg_bit_rate = compute_avg_bitrate(track);
> +
> +
> +    return (struct mpeg4_bit_rate_values){
> +        .buffer_size = props ? props->buffer_size / 8 : 0,
> +        // (FIXME should be max rate in any 1 sec window)
> +        .max_bit_rate = props ? \
> +            FFMAX3(props->max_bitrate, props->avg_bitrate, avg_bit_rate) : \
> +            FFMAX(track->par->bit_rate, avg_bit_rate),
> +        .avg_bit_rate = avg_bit_rate,
> +    };

I find this form pretty terrible for readability. As this is a proper 
function and not just a macro, would it be possible to just make this more 
sequential code, like:

     struct mpeg4_bit_rate_values ret;
     ret.avg_bit_rate = avg_bit_rate;
     ...

And one could avoid the awkward duplication in the FFMAX()/FFMAX3() by 
doing e.g. like this:
     ret.max_bit_rate = FFMAX(track->par_bit_rate, avg_bit_rate);
     if (props)
         ret.max_bit_rate = FFMAX(ret.max_bit_rate, props->max_bitrate);

// Martin