On 9/28/18 11:33 AM, Amit Kale wrote:
> If this flag is set, BANDWIDTH value in a master playlist entry will be set
> to the peak segment bandwidth. This patch also delays freeing of hls stream
> data, so that it's available for bandwidth calculation.
This is a very useful feature. Thanks for contributing back to ffmpeg.
I think the patch and its logic is overall good. Comments for some minor issues
inlined below.
>
> Signed-off-by: Amit Kale<am...@hotstar.com>
> ---
> doc/muxers.texi | 4 ++++
> libavformat/hlsenc.c | 29 +++++++++++++++++++++--------
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> Index: ffmpeg/doc/muxers.texi
> ===================================================================
> --- ffmpeg.orig/doc/muxers.texi
> +++ ffmpeg/doc/muxers.texi
> @@ -789,6 +789,10 @@ subdirectories.
> Possible values:
>
> @table @samp
> +@item peak_segment_bw
I think abbreviations are not allowed/recommended in command line options of
ffmpeg.
Maybe better to rename it to peak_segment_bandwidth or peak _bandwidth.
> +If this flag is set, BANDWIDTH value in a master playlist entry will be
> +set to the peak segment bandwidth.
> +
> @item single_file
> If this flag is set, the muxer will store all segments in a single MPEG-TS
> file, and will use byte ranges in the playlist. HLS playlists generated with
> Index: ffmpeg/libavformat/hlsenc.c
> ===================================================================
> --- ffmpeg.orig/libavformat/hlsenc.c
> +++ ffmpeg/libavformat/hlsenc.c
> @@ -99,6 +99,7 @@ typedef enum HLSFlags {
> HLS_TEMP_FILE = (1 << 11),
> HLS_PERIODIC_REKEY = (1 << 12),
> HLS_INDEPENDENT_SEGMENTS = (1 << 13),
> + HLS_PEAK_SEGMENT_BW = (1 << 14),
Looks like you haven't used "git format-patch" to generate this patch. Due to
this the spacing looks odd and it is not possible to apply this patch directly.
Could you please resend this patch generated from "git format-patch"
> } HLSFlags;
>
> typedef enum {
> @@ -1174,7 +1175,8 @@ static int64_t get_stream_bit_rate(AVStr
> }
>
> static int create_master_playlist(AVFormatContext *s,
> - VariantStream * const input_vs)
> + VariantStream * const input_vs,
> + int last)
> {
> HLSContext *hls = s->priv_data;
> VariantStream *vs, *temp_vs;
> @@ -1191,13 +1193,16 @@ static int create_master_playlist(AVForm
> for (i = 0; i < hls->nb_varstreams; i++)
> if (!hls->var_streams[i].m3u8_created)
> return 0;
> - } else {
> + } else if (!last) {
> /* Keep publishing the master playlist at the configured rate */
> if (&hls->var_streams[0] != input_vs || !hls->master_publish_rate ||
> input_vs->number % hls->master_publish_rate)
> return 0;
> }
>
> + if (hls->flags & HLS_PEAK_SEGMENT_BW && hls->version < 7)
> + hls->version = 7;
> +
It is better to move this logic to "hls_window" function, where version setting
logic is present.
Otherwise version setting logic gets cluttered.
> set_http_options(s, &options, hls);
>
> ret = hlsenc_io_open(s, &hls->m3u8_out, hls->master_m3u8_url, &options);
> @@ -1298,11 +1303,21 @@ static int create_master_playlist(AVForm
> }
>
> bandwidth = 0;
> - if (vid_st)
> - bandwidth += get_stream_bit_rate(vid_st);
> - if (aud_st)
> - bandwidth += get_stream_bit_rate(aud_st);
> - bandwidth += bandwidth / 10;
> + if (last && hls->flags & HLS_PEAK_SEGMENT_BW) {
> + HLSSegment *hs = vs->segments;
> + while (hs) {
> + int64_t segment_bandwidth = hs->size * 8 / hs->duration;
> + if (bandwidth < segment_bandwidth)
> + bandwidth = segment_bandwidth;
> + hs = hs->next;
> + }
> + } else {
> + if (vid_st)
> + bandwidth += get_stream_bit_rate(vid_st);
> + if (aud_st)
> + bandwidth += get_stream_bit_rate(aud_st);
> + bandwidth += bandwidth / 10;
> + }
>
> ccgroup = NULL;
> if (vid_st && vs->ccgroup) {
> @@ -1443,7 +1458,7 @@ fail:
> ff_rename(temp_filename, vs->m3u8_name, s);
>
> if (ret >= 0 && hls->master_pl_name)
> - if (create_master_playlist(s, vs) < 0)
> + if (create_master_playlist(s, vs, last) < 0)
> av_log(s, AV_LOG_WARNING, "Master playlist creation failed\n");
>
> return ret;
> @@ -2421,10 +2436,13 @@ failed:
> av_freep(&vs->vtt_m3u8_name);
> avformat_free_context(vtt_oc);
> }
> + av_free(old_filename);
>
> + }
> + for (i = 0; i < hls->nb_varstreams; i++) {
> + vs = &hls->var_streams[i];
> hls_free_segments(vs->segments);
> hls_free_segments(vs->old_segments);
> - av_free(old_filename);
> av_freep(&vs->m3u8_name);
> av_freep(&vs->streams);
> av_freep(&vs->agroup);
> @@ -2819,6 +2837,7 @@ static const AVOption options[] = {
> {"fmp4", "make segment file to fragment mp4 files in m3u8", 0,
> AV_OPT_TYPE_CONST, {.i64 = SEGMENT_TYPE_FMP4 }, 0, UINT_MAX, E,
> "segment_type"},
> {"hls_fmp4_init_filename", "set fragment mp4 file init filename",
> OFFSET(fmp4_init_filename), AV_OPT_TYPE_STRING, {.str = "init.mp4"},
> 0, 0, E},
> {"hls_flags", "set flags affecting HLS playlist and media file
> generation", OFFSET(flags), AV_OPT_TYPE_FLAGS, {.i64 = 0 }, 0, UINT_MAX, E,
> "flags"},
> + {"peak_segment_bw", "sets bandwidth in master play list to peak
> segment bandwidth", 0, AV_OPT_TYPE_CONST, {.i64 = HLS_PEAK_SEGMENT_BW }, 0,
> UINT_MAX, E, "flags"},
> {"single_file", "generate a single media file indexed with byte
> ranges", 0, AV_OPT_TYPE_CONST, {.i64 = HLS_SINGLE_FILE }, 0, UINT_MAX, E,
> "flags"},
> {"temp_file", "write segment to temporary file and rename when
> complete", 0, AV_OPT_TYPE_CONST, {.i64 = HLS_TEMP_FILE }, 0, UINT_MAX, E,
> "flags"},
> {"delete_segments", "delete segment files that are no longer part of the
> playlist", 0, AV_OPT_TYPE_CONST, {.i64 = HLS_DELETE_SEGMENTS }, 0, UINT_MAX,
> E, "flags"},
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
