[FFmpeg-devel] [PATCH] avformat/dashenc: use AVCodecContext timebase when computing missing bitrate

[Przemysław Sobala]

On Thu, Jun 4, 2020 at 3:13 PM Przemysław Sobala <
przemyslaw.sobala at gmail.com> wrote:

> On Thu, Jun 4, 2020 at 11:24 AM Jeyapal, Karthick <kjeyapal at akamai.com>
> wrote:
>
>>
>> On 6/4/20 1:29 PM, Przemysław Sobala wrote:
>> > On Tue, Jun 2, 2020 at 10:19 AM Przemysław Sobala <
>> > przemyslaw.sobala at gmail.com> wrote:
>> >
>> >> On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal at akamai.com>
>> >> wrote:
>> >>
>> >>>
>> >>> On 6/1/20 5:24 PM, Przemysław Sobala wrote:
>> >>>> On Mon, Jun 1, 2020 at 10:06 AM Anton Khirnov <anton at khirnov.net>
>> >>> wrote:
>> >>>>
>> >>>>> Quoting Przemysław Sobala (2020-05-27 17:07:22)
>> >>>>>> ---
>> >>>>>>  libavformat/dashenc.c | 2 +-
>> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>>
>> >>>>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> >>>>>> index 0cf0df50ef..00a37b175d 100644
>> >>>>>> --- a/libavformat/dashenc.c
>> >>>>>> +++ b/libavformat/dashenc.c
>> >>>>>> @@ -1959,7 +1959,7 @@ static int dash_flush(AVFormatContext *s, int
>> >>>>> final, int stream)
>> >>>>>>
>> >>>>>>          if (!os->bit_rate) {
>> >>>>>>              // calculate average bitrate of first segment
>> >>>>>> -            int64_t bitrate = (int64_t) range_length * 8 *
>> >>> AV_TIME_BASE
>> >>>>> / duration;
>> >>>>>> +            int64_t bitrate = (int64_t) range_length * 8 *
>> >>>>> (c->use_timeline ? os->ctx->streams[0]->time_base.den :
>> AV_TIME_BASE) /
>> >>>>> duration;
>> >>>>>
>> >>>>> That does not look like an AVCodecContext
>> >>>>>
>> >>>>
>> >>>> Of course not. time_base is AVStream's field. I don't know why I
>> wrote
>> >>>> AVCodecContext... Please amend that commit message if possible.
>> >>> Amended and Pushed!
>> >>>
>> >>> Thanks,
>> >>> Karthick
>> >>>
>> >>>
>> >> Thanks.
>> >> What do you think about computing an average bitrate for all segments,
>> not
>> >> only the first one (in case of a static - not dynamic - DASH
>> manifest), if
>> >> one would not want to specify bitrate while encoding using x264 CRF
>> rate
>> >> control? I could prepare such a patch that, if bitrate is not
>> specified,
>> >> it'd be computed at the end, for static manifest, for all segments.
>> It'd be
>> >> more accurate comparing to the first segment's bitrate.
>> >>
>> >
>> > Any comments about that?
>> Any patch that fixes/improves the current behavior is always welcome :)
>>
>
> I was thinking about something like (diff against release/4.2 branch):
>
>
> $ git diff origin/release/4.2 -- libavformat/dashenc.c
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index f0e45da89ad..60250b6d33a 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -88,6 +88,7 @@ typedef struct OutputStream {
>      int64_t first_pts, start_pts, max_pts;
>      int64_t last_dts, last_pts;
>      int bit_rate;
> +    double average_bit_rate;
>      SegmentType segment_type;  /* segment type selected for this
> particular stream */
>      const char *format_name;
>      const char *extension_name;
> @@ -775,6 +776,9 @@ static int write_adaptation_set(AVFormatContext *s,
> AVIOContext *out, int as_ind
>          if (os->bit_rate > 0)
>              snprintf(bandwidth_str, sizeof(bandwidth_str), "
> bandwidth=\"%d\"",
>                       os->bit_rate);
> +        else if (final)
> +            snprintf(bandwidth_str, sizeof(bandwidth_str), "
> bandwidth=\"%d\"",
> +                     (int) os->average_bit_rate);
>
>          if (as->media_type == AVMEDIA_TYPE_VIDEO) {
>              AVStream *st = s->streams[i];
> @@ -1618,12 +1622,10 @@ static int dash_flush(AVFormatContext *s, int
> final, int stream)
>          os->total_pkt_size = 0;
>
>          if (!os->bit_rate) {
> -            // calculate average bitrate of first segment
> -            int64_t bitrate = (int64_t) range_length * 8 * AV_TIME_BASE /
> av_rescale_q(os->max_pts - os->start_pts,
> -
>               st->time_base,
> -
>               AV_TIME_BASE_Q);
> -            if (bitrate >= 0)
> -                os->bit_rate = bitrate;
> +            // calculate average bitrate
> +            int64_t duration = av_rescale_q(os->max_pts - os->start_pts,
> st->time_base, AV_TIME_BASE_Q);
> +            int64_t segment_bitrate = (int64_t) range_length * 8 *
> AV_TIME_BASE / duration;
> +            os->average_bit_rate = (os->average_bit_rate *
> (os->segment_index - 1) + segment_bitrate) / os->segment_index;
>          }
>          add_segment(os, os->filename, os->start_pts, os->max_pts -
> os->start_pts, os->pos, range_length, index_length, next_exp_index);
>          av_log(s, AV_LOG_VERBOSE, "Representation %d media segment %d
> written to: %s\n", i, os->segment_index, os->full_path);
>
>
> This patch allows to compute the average bitrate for all segments and
> write it inside a static manifest.
> Unfortunately, for dynamic manifest, the bitrate, if not specified by
> client, is not computed at all, and I have no idea how to handle that.
> Previously, bitrate for dynamic manifest, if not specified by client, was
> computed only for the first segment. I could introduce another field
> "first_segment_bitrate" in struct OutputStream and fill the manifest with
> that value, as long as it's not a static one, to preserve that behavior.
>

Again, any comments?

--
regards
Przemysław Sobala