| Message ID | 1525415541-11801-1-git-send-email-kjeyapal@akamai.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, May 4, 2018 at 9:32 AM, Karthick J <kjeyapal@akamai.com> wrote: > From: Karthick Jeyapal <kjeyapal@akamai.com> > > Right now segment file format is chosen to be either mp4 or webm based on the codec format. > This patch makes that choice configurable by the user, instead of being decided by the muxer. > --- > doc/muxers.texi | 8 ++++++++ > libavformat/dashenc.c | 48 ++++++++++++++++++++++-------------------------- > 2 files changed, 30 insertions(+), 26 deletions(-) > Hi, Sorry for getting to this so late, been busy on various things (as usual). Thanks for prodding me. > diff --git a/doc/muxers.texi b/doc/muxers.texi > index 6f03bba..2429f8e 100644 > --- a/doc/muxers.texi > +++ b/doc/muxers.texi > @@ -282,6 +282,14 @@ corrects that index value. > Typically this logic is needed in live streaming use cases. The network bandwidth > fluctuations are common during long run streaming. Each fluctuation can cause > the segment indexes fall behind the expected real time position. > + > +@item dash_segment_type @var{dash_segment_type} > +Possible values: > +@item mp4 > +If this flag is set, the dash segment files will be in in ISOBMFF format. This is the default format. > + > +@item webm > +If this flag is set, the dash segment files will be in in WebM format. > @end table > > @anchor{framecrc} > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index 1dd6333..412f074 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -48,6 +48,11 @@ > #include "vpcc.h" > #include "dash.h" > > +typedef enum { > + SEGMENT_TYPE_MP4, > + SEGMENT_TYPE_WEBM, > +} SegmentType; > + Ah yes, an enum :) I really like the checks being equality/inequality now. I've seen things like SEGMENT_TYPE_NB used for the stopper so that in the AVOption you can then set the maximum to *_NB - 1 instead of then having to change it if it ever gets anything added to it. Maybe consider making something like the `codecs[]` array for formats and make the thing in DASHContext as a char pointer, so that you can just point the string pointer to its value in init() instead of doing a run-time strncpy. This does remove the "dynamicness" of the per-stream selection, which possibly should be mentioned. But at least personally I think this is what people actually wanted with WebM vs ISOBMFF DASH selection ;) , as in not having surprises between streams. Otherwise this patch generally looks alright, leaving just the segment file name part not automatical just yet :) (I feel like we need to have separate options for the general template and the extension). Best regards, Jan
On 5/12/18 8:47 PM, Jan Ekström wrote: > On Fri, May 4, 2018 at 9:32 AM, Karthick J <kjeyapal@akamai.com> wrote: >> From: Karthick Jeyapal <kjeyapal@akamai.com> >> >> Right now segment file format is chosen to be either mp4 or webm based on the codec format. >> This patch makes that choice configurable by the user, instead of being decided by the muxer. >> --- >> doc/muxers.texi | 8 ++++++++ >> libavformat/dashenc.c | 48 ++++++++++++++++++++++-------------------------- >> 2 files changed, 30 insertions(+), 26 deletions(-) >> > > Hi, > > Sorry for getting to this so late, been busy on various things (as > usual). Thanks for prodding me. Thanks for your reply. > >> diff --git a/doc/muxers.texi b/doc/muxers.texi >> index 6f03bba..2429f8e 100644 >> --- a/doc/muxers.texi >> +++ b/doc/muxers.texi >> @@ -282,6 +282,14 @@ corrects that index value. >> Typically this logic is needed in live streaming use cases. The network bandwidth >> fluctuations are common during long run streaming. Each fluctuation can cause >> the segment indexes fall behind the expected real time position. >> + >> +@item dash_segment_type @var{dash_segment_type} >> +Possible values: >> +@item mp4 >> +If this flag is set, the dash segment files will be in in ISOBMFF format. This is the default format. >> + >> +@item webm >> +If this flag is set, the dash segment files will be in in WebM format. >> @end table >> >> @anchor{framecrc} >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index 1dd6333..412f074 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -48,6 +48,11 @@ >> #include "vpcc.h" >> #include "dash.h" >> >> +typedef enum { >> + SEGMENT_TYPE_MP4, >> + SEGMENT_TYPE_WEBM, >> +} SegmentType; >> + > I agree with all your comments below. Please find the newer patch in http://ffmpeg.org/pipermail/ffmpeg-devel/2018-May/229998.html which tries to address most of them. > Ah yes, an enum :) I really like the checks being equality/inequality > now. I've seen things like SEGMENT_TYPE_NB used for the stopper so > that in the AVOption you can then set the maximum to *_NB - 1 instead > of then having to change it if it ever gets anything added to it. > > Maybe consider making something like the `codecs[]` array for formats > and make the thing in DASHContext as a char pointer, so that you can > just point the string pointer to its value in init() instead of doing > a run-time strncpy. > > This does remove the "dynamicness" of the per-stream selection, which > possibly should be mentioned. But at least personally I think this is > what people actually wanted with WebM vs ISOBMFF DASH selection ;) , > as in not having surprises between streams. > > Otherwise this patch generally looks alright, leaving just the segment > file name part not automatical just yet :) (I feel like we need to > have separate options for the general template and the extension). > > Best regards, > Jan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/doc/muxers.texi b/doc/muxers.texi index 6f03bba..2429f8e 100644 --- a/doc/muxers.texi +++ b/doc/muxers.texi @@ -282,6 +282,14 @@ corrects that index value. Typically this logic is needed in live streaming use cases. The network bandwidth fluctuations are common during long run streaming. Each fluctuation can cause the segment indexes fall behind the expected real time position. + +@item dash_segment_type @var{dash_segment_type} +Possible values: +@item mp4 +If this flag is set, the dash segment files will be in in ISOBMFF format. This is the default format. + +@item webm +If this flag is set, the dash segment files will be in in WebM format. @end table @anchor{framecrc} diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index 1dd6333..412f074 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -48,6 +48,11 @@ #include "vpcc.h" #include "dash.h" +typedef enum { + SEGMENT_TYPE_MP4, + SEGMENT_TYPE_WEBM, +} SegmentType; + typedef struct Segment { char file[1024]; int64_t start_pos; @@ -69,7 +74,6 @@ typedef struct OutputStream { AVFormatContext *ctx; int ctx_inited, as_idx; AVIOContext *out; - char format_name[8]; int packets_written; char initfile[1024]; int64_t init_start_pos, pos; @@ -125,6 +129,8 @@ typedef struct DASHContext { int streaming; int64_t timeout; int index_correction; + SegmentType segment_type; + char format_name[8]; } DASHContext; static struct codec_string { @@ -581,13 +587,13 @@ static int write_adaptation_set(AVFormatContext *s, AVIOContext *out, int as_ind if (as->media_type == AVMEDIA_TYPE_VIDEO) { AVStream *st = s->streams[i]; avio_printf(out, "\t\t\t<Representation id=\"%d\" mimeType=\"video/%s\" codecs=\"%s\"%s width=\"%d\" height=\"%d\"", - i, os->format_name, os->codec_str, bandwidth_str, s->streams[i]->codecpar->width, s->streams[i]->codecpar->height); + i, c->format_name, os->codec_str, bandwidth_str, s->streams[i]->codecpar->width, s->streams[i]->codecpar->height); if (st->avg_frame_rate.num) avio_printf(out, " frameRate=\"%d/%d\"", st->avg_frame_rate.num, st->avg_frame_rate.den); avio_printf(out, ">\n"); } else { avio_printf(out, "\t\t\t<Representation id=\"%d\" mimeType=\"audio/%s\" codecs=\"%s\"%s audioSamplingRate=\"%d\">\n", - i, os->format_name, os->codec_str, bandwidth_str, s->streams[i]->codecpar->sample_rate); + i, c->format_name, os->codec_str, bandwidth_str, s->streams[i]->codecpar->sample_rate); avio_printf(out, "\t\t\t\t<AudioChannelConfiguration schemeIdUri=\"urn:mpeg:dash:23003:3:audio_channel_configuration:2011\" value=\"%d\" />\n", s->streams[i]->codecpar->channels); } @@ -959,27 +965,14 @@ static int dash_init(AVFormatContext *s) if (!ctx) return AVERROR(ENOMEM); - // choose muxer based on codec: webm for VP8 and opus, mp4 otherwise - // note: os->format_name is also used as part of the mimetype of the - // representation, e.g. video/<format_name> - if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_VP8 || - s->streams[i]->codecpar->codec_id == AV_CODEC_ID_OPUS || - s->streams[i]->codecpar->codec_id == AV_CODEC_ID_VORBIS) { - snprintf(os->format_name, sizeof(os->format_name), "webm"); - - if (s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) { - av_log(s, AV_LOG_ERROR, - "WebM support in dashenc is experimental and has not " - "been validated. For testing purposes, make sure " - "to add -strict experimental and override " - "-init_seg_name and -media_seg_name to end with " - "the extension 'webm'.\n"); - return AVERROR(EINVAL); - } + if (c->segment_type == SEGMENT_TYPE_WEBM) { + snprintf(c->format_name, sizeof(c->format_name), "webm"); + } else if (c->segment_type == SEGMENT_TYPE_MP4) { + snprintf(c->format_name, sizeof(c->format_name), "mp4"); } else { - snprintf(os->format_name, sizeof(os->format_name), "mp4"); + return AVERROR_MUXER_NOT_FOUND; } - ctx->oformat = av_guess_format(os->format_name, NULL, NULL); + ctx->oformat = av_guess_format(c->format_name, NULL, NULL); if (!ctx->oformat) return AVERROR_MUXER_NOT_FOUND; os->ctx = ctx; @@ -1017,7 +1010,7 @@ static int dash_init(AVFormatContext *s) av_dict_free(&opts); os->init_start_pos = 0; - if (!strcmp(os->format_name, "mp4")) { + if (c->segment_type == SEGMENT_TYPE_MP4) { if (c->streaming) av_dict_set(&opts, "movflags", "frag_every_frame+dash+delay_moov", 0); else @@ -1082,7 +1075,7 @@ static int dash_write_header(AVFormatContext *s) // Flush init segment // Only for WebM segment, since for mp4 delay_moov is set and // the init segment is thus flushed after the first packets. - if (strcmp(os->format_name, "mp4") && + if (c->segment_type == SEGMENT_TYPE_WEBM && (ret = flush_init_segment(s, os)) < 0) return ret; } @@ -1253,7 +1246,7 @@ static int dash_flush(AVFormatContext *s, int final, int stream) } if (!c->single_file) { - if (!strcmp(os->format_name, "mp4") && !os->written_len) + if (c->segment_type == SEGMENT_TYPE_MP4 && !os->written_len) write_styp(os->ctx->pb); } else { snprintf(os->full_path, sizeof(os->full_path), "%s%s", c->dirname, os->initfile); @@ -1443,7 +1436,7 @@ static int dash_write_packet(AVFormatContext *s, AVPacket *pkt) } //write out the data immediately in streaming mode - if (c->streaming && !strcmp(os->format_name, "mp4")) { + if (c->streaming && c->segment_type == SEGMENT_TYPE_MP4) { int len = 0; uint8_t *buf = NULL; if (!os->written_len) @@ -1538,6 +1531,9 @@ static const AVOption options[] = { { "streaming", "Enable/Disable streaming mode of output. Each frame will be moof fragment", OFFSET(streaming), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, E }, { "timeout", "set timeout for socket I/O operations", OFFSET(timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1, INT_MAX, .flags = E }, { "index_correction", "Enable/Disable segment index correction logic", OFFSET(index_correction), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, E }, + { "dash_segment_type", "set dash segment files type", OFFSET(segment_type), AV_OPT_TYPE_INT, {.i64 = SEGMENT_TYPE_MP4 }, 0, SEGMENT_TYPE_WEBM, E, "segment_type"}, + { "mp4", "make segment file in ISOBMFF format", 0, AV_OPT_TYPE_CONST, {.i64 = SEGMENT_TYPE_MP4 }, 0, UINT_MAX, E, "segment_type"}, + { "webm", "make segment file in WebM format", 0, AV_OPT_TYPE_CONST, {.i64 = SEGMENT_TYPE_WEBM }, 0, UINT_MAX, E, "segment_type"}, { NULL }, };
From: Karthick Jeyapal <kjeyapal@akamai.com> Right now segment file format is chosen to be either mp4 or webm based on the codec format. This patch makes that choice configurable by the user, instead of being decided by the muxer. --- doc/muxers.texi | 8 ++++++++ libavformat/dashenc.c | 48 ++++++++++++++++++++++-------------------------- 2 files changed, 30 insertions(+), 26 deletions(-)