diff mbox series

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

Message ID 20200920170629.26504-2-jeebjp@gmail.com
State Accepted
Headers show
Series avformat/movenc: btrt box support | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Sept. 20, 2020, 5:06 p.m. UTC 
This can now be re-utilized in other places.
---
 libavformat/movenc.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Martin Storsjö Sept. 21, 2020, 10:08 a.m. UTC | #1 
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
diff mbox series

Patch

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 = \
+        (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,
+    };
+}
+
 static int mov_write_esds_tag(AVIOContext *pb, MOVTrack *track) // Basic
 {
-    AVCPBProperties *props;
+    struct mpeg4_bit_rate_values bit_rates = calculate_mpeg4_bit_rates(track);
     int64_t pos = avio_tell(pb);
     int decoder_specific_info_len = track->vos_len ? 5 + track->vos_len : 0;
-    unsigned avg_bitrate;
 
     avio_wb32(pb, 0); // size
     ffio_wfourcc(pb, "esds");
@@ -669,14 +693,9 @@  static int mov_write_esds_tag(AVIOContext *pb, MOVTrack *track) // Basic
     else
         avio_w8(pb, 0x11); // flags (= Visualstream)
 
-    props = (AVCPBProperties*)av_stream_get_side_data(track->st, AV_PKT_DATA_CPB_PROPERTIES,
-                                                      NULL);
-
-    avio_wb24(pb, props ? props->buffer_size / 8 : 0); // Buffersize DB
-
-    avg_bitrate = compute_avg_bitrate(track);
-    avio_wb32(pb, props ? FFMAX3(props->max_bitrate, props->avg_bitrate, avg_bitrate) : FFMAX(track->par->bit_rate, avg_bitrate)); // maxbitrate (FIXME should be max rate in any 1 sec window)
-    avio_wb32(pb, avg_bitrate);
+    avio_wb24(pb, bit_rates.buffer_size); // Buffersize DB
+    avio_wb32(pb, bit_rates.max_bit_rate); // maxbitrate
+    avio_wb32(pb, bit_rates.avg_bit_rate);
 
     if (track->vos_len) {
         // DecoderSpecific info descriptor