Message ID | 20200527150722.611671-1-przemyslaw.sobala@gmail.com |
---|---|
State | Accepted |
Commit | 2a9ffd89fcb09bd69b2130da039ad2caba79cf33 |
Headers | show |
Series | [FFmpeg-devel] avformat/dashenc: use AVCodecContext timebase when computing missing bitrate | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On 5/27/20 8:37 PM, Przemysław Sobala wrote: > --- > 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; > if (bitrate >= 0) > os->bit_rate = bitrate; > } LGTM. Thanks.
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
On Mon, Jun 1, 2020 at 10:06 AM Anton Khirnov <anton@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. -- Przemysław Sobala
On 6/1/20 5:24 PM, Przemysław Sobala wrote: > On Mon, Jun 1, 2020 at 10:06 AM Anton Khirnov <anton@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 > > -- > Przemysław Sobala > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__ffmpeg.org_mailman_listinfo_ffmpeg-2Ddevel&d=DwIGaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=xOoesbz-6ff1GPXp5Lg4jf1ZG99yp4a1qhxVn_YOwRU&m=d5_uQoLoKY0i_-ZFJQGxnQcD0feA0He0ST4ojg4---4&s=dstVnsDMFsrakFvnEaO5zT4dmLntnO4RBfr7YTHwI_4&e= > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal@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@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. -- Regards Przemysław Sobala
On Tue, Jun 2, 2020 at 10:19 AM Przemysław Sobala < przemyslaw.sobala@gmail.com> wrote: > On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal@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@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? -- pozdrawiam Przemysław Sobala
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@gmail.com> wrote: > >> On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal@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@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 :) > > -- > pozdrawiam > Przemysław Sobala > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__ffmpeg.org_mailman_listinfo_ffmpeg-2Ddevel&d=DwIGaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=xOoesbz-6ff1GPXp5Lg4jf1ZG99yp4a1qhxVn_YOwRU&m=q7hq8J8ltTngyTvXBudpQq8HN4Ure-eQJZiCG85-TgI&s=pM0LIY83Tgww_6eGlIuJe7lzdcfZcKoqyH2rMDz396E&e= > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, Jun 4, 2020 at 11:24 AM Jeyapal, Karthick <kjeyapal@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@gmail.com> wrote: > > > >> On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal@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@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. -- regards Przemysław Sobala
On Thu, Jun 4, 2020 at 3:13 PM Przemysław Sobala < przemyslaw.sobala@gmail.com> wrote: > On Thu, Jun 4, 2020 at 11:24 AM Jeyapal, Karthick <kjeyapal@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@gmail.com> wrote: >> > >> >> On Mon, Jun 1, 2020 at 3:30 PM Jeyapal, Karthick <kjeyapal@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@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
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; if (bitrate >= 0) os->bit_rate = bitrate; }