Thanks for the thorough review. I incorporated all your feedback, and it did clean up my code significantly, especially the format-tag population method, which I've now renamed to dash_read_tmpl_id().
To answer some of your questions, the '$$' identifier logic was missing for no real good reason, so I added that to my commit. I also agree with you that the dash-format-string for the template identifiers, when provided, should take the form of '%0[width]d' only and not '%d'. Technically '%d' still works with dash.js, but there's really never any point in having a '%d' since things end up looking the same even without it. On Mon, Nov 24, 2014 at 11:17 AM, Martin Storsjö <[email protected]> wrote: > On Mon, 24 Nov 2014, Bryan Huh wrote: > > This allows one to specify templated segment names for init-segments, >> media-segments, and for the base-url in the case of single-file. >> --- >> libavformat/dashenc.c | 165 ++++++++++++++++++++++++++++++ >> +++++++++++++++----- >> 1 file changed, 151 insertions(+), 14 deletions(-) >> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index 0654661..1f8e4a3 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -38,6 +38,15 @@ >> #include "os_support.h" >> #include "url.h" >> >> +// See ISO/IEC 23009-1:2014 5.3.9.4.4 >> +typedef enum { >> + DASH_TMPL_ID_UNDEFINED = -1, >> + DASH_TMPL_ID_REP_ID, >> + DASH_TMPL_ID_NUMBER, >> + DASH_TMPL_ID_BANDWIDTH, >> + DASH_TMPL_ID_TIME, >> +} DASHTmplId; >> + >> > > It seems you're missing handling of the $$ escape pattern? > > > typedef struct Segment { >> char file[1024]; >> int64_t start_pos; >> @@ -59,6 +68,7 @@ typedef struct OutputStream { >> int nb_segments, segments_size, segment_index; >> Segment **segments; >> int64_t first_dts, start_dts, end_dts; >> + int bit_rate; >> char bandwidth_str[64]; >> >> char codec_str[100]; >> @@ -72,13 +82,15 @@ typedef struct DASHContext { >> int remove_at_exit; >> int use_template; >> int use_timeline; >> - int single_file; >> OutputStream *streams; >> int has_video, has_audio; >> int last_duration; >> int total_duration; >> char availability_start_time[100]; >> char dirname[1024]; >> + const char *init_seg_name; >> + const char *media_seg_name; >> + const char *single_file_name; >> } DASHContext; >> >> static int dash_write(void *opaque, uint8_t *buf, int buf_size) >> @@ -192,7 +204,7 @@ static void output_segment_list(OutputStream *os, >> AVIOContext *out, DASHContext >> avio_printf(out, "\t\t\t\t<SegmentTemplate timescale=\"%d\" ", >> timescale); >> if (!c->use_timeline) >> avio_printf(out, "duration=\"%d\" ", c->last_duration); >> - avio_printf(out, >> "initialization=\"init-stream$RepresentationID$.m4s\" >> media=\"chunk-stream$RepresentationID$-$Number%%05d$.m4s\" >> startNumber=\"%d\">\n", c->use_timeline ? start_number : 1); >> + avio_printf(out, "initialization=\"%s\" media=\"%s\" >> startNumber=\"%d\">\n", c->init_seg_name, c->media_seg_name, >> c->use_timeline ? start_number : 1); >> if (c->use_timeline) { >> avio_printf(out, "\t\t\t\t\t<SegmentTimeline>\n"); >> for (i = start_index; i < os->nb_segments; ) { >> @@ -212,7 +224,7 @@ static void output_segment_list(OutputStream *os, >> AVIOContext *out, DASHContext >> avio_printf(out, "\t\t\t\t\t</SegmentTimeline>\n"); >> } >> avio_printf(out, "\t\t\t\t</SegmentTemplate>\n"); >> - } else if (c->single_file) { >> + } else if (c->single_file_name) { >> avio_printf(out, "\t\t\t\t<BaseURL>%s</BaseURL>\n", >> os->initfile); >> avio_printf(out, "\t\t\t\t<SegmentList timescale=\"%d\" >> duration=\"%d\" startNumber=\"%d\">\n", AV_TIME_BASE, c->last_duration, >> start_number); >> avio_printf(out, "\t\t\t\t\t<Initialization >> range=\"%"PRId64"-%"PRId64"\" />\n", os->init_start_pos, os->init_start_pos >> + os->init_range_length - 1); >> @@ -235,6 +247,128 @@ static void output_segment_list(OutputStream *os, >> AVIOContext *out, DASHContext >> } >> } >> >> +static DASHTmplId dash_read_format_tag(const char *identifier, char* >> format_tag, const char **ptr) { >> > > nit: char *format_tag > > + const char *next_ptr; >> + DASHTmplId id_type = DASH_TMPL_ID_UNDEFINED; >> + >> + if (av_strstart(identifier, "$RepresentationID$", &next_ptr)) { >> + id_type = DASH_TMPL_ID_REP_ID; >> + // default to basic format, as $RepresentationID$ identifiers >> + // are not allowed to have custom format-tags. >> + av_strlcpy(format_tag, "%d", 3); >> > > Instead of hardcoding the 3 here, perhaps you could pass in the size of > the format_tag buffer to have it all a bit cleaner. Right now this is just > as (un)safe as plain strcpy except a bit more obscured. We do know that the > caller will have a big enough buffer, but the code would be cleaner (fewer > magic numbers) to use a parameter for it. > > + if (ptr) >> + *ptr = next_ptr; >> > > I think you could simplify this a little by assuming ptr always is > non-null - it's (currently at least) only used within this file so we know > who will be calling it. > > + } else { // the following identifiers may have an explicit format_tag >> + if (av_strstart(identifier, "$Number", &next_ptr)) >> + id_type = DASH_TMPL_ID_NUMBER; >> + else if (av_strstart(identifier, "$Bandwidth", &next_ptr)) >> + id_type = DASH_TMPL_ID_BANDWIDTH; >> + else if (av_strstart(identifier, "$Time", &next_ptr)) >> + id_type = DASH_TMPL_ID_TIME; >> + else >> + id_type = DASH_TMPL_ID_UNDEFINED; >> + >> + // next parse the format-tag: >> + // (next_ptr now points at the first '%' at the beginning of the >> format-tag) >> + if (id_type != DASH_TMPL_ID_UNDEFINED) { >> + if (next_ptr[0] == '$') { // no format-tag >> + if (id_type == DASH_TMPL_ID_TIME) >> + av_strlcpy(format_tag, "%lld", 5); >> + else >> + av_strlcpy(format_tag, "%d", 3); >> > > The "if (time) %lld else %d" pattern is repeated a few times here - you > could perhaps store the lld vs d string in a variable, straightening out > the rest of this a little > > + if (ptr) >> + *ptr = &next_ptr[1]; >> + } else if (av_strstart(next_ptr, "%d$", &next_ptr)) { >> + if (id_type == DASH_TMPL_ID_TIME) >> + av_strlcpy(format_tag, "%lld", 5); >> + else >> + av_strlcpy(format_tag, "%d", 3); >> + if (ptr) >> + *ptr = next_ptr; >> > > Technically, is this even allowed by the spec? I don't mind keeping it > though. As far as I read it, the only valid format tag is %0[width]d, not > %d in itself. > > + } else { >> + const char *width_ptr; >> + // only tolerate single-digit width-field (a.k.a up to >> 9-digit width) >> + if (av_strstart(next_ptr, "%0", &width_ptr) && >> > > nit: Unintentional double space before the && > > > + av_isdigit(width_ptr[0]) && >> + av_strstart(&width_ptr[1], "d$", &next_ptr)) { >> + if (id_type == DASH_TMPL_ID_TIME) { >> + char format[7] = { '%', '0', width_ptr[0], 'l', >> 'l', 'd', '\0' }; >> + av_strlcpy(format_tag, format, 7); >> + } else { >> + char format[5] = { '%', '0', width_ptr[0], 'd', >> '\0' }; >> + av_strlcpy(format_tag, format, 5); >> + } >> + if (ptr) >> + *ptr = next_ptr; >> + } else { >> + av_log(NULL, AV_LOG_WARNING, "Failed to parse >> format-tag beginning with %s. Expected either a " >> + "closing '$' character >> or a format-string like '%%d' or '%%0[width]d', " >> + "where width must be a >> single digit\n", next_ptr); >> + id_type = DASH_TMPL_ID_UNDEFINED; >> + } >> + } >> + } >> + } >> + return id_type; >> +} >> + >> +static void dash_fill_tmpl_params(char *dst, size_t buffer_size, >> + const char *template, int rep_id, >> + int number, int bit_rate, >> + int64_t time) { >> > > nit: Misaligned the later lines > > + int dst_pos = 0; >> + const char *t_cur = template; >> + while (dst_pos < buffer_size-1 && strlen(t_cur) > 0) { >> > > nit: Add spaces around the - in buffer_size-1 > > Instead of strlen(t_cur) > 0 you could also use *t_cur, which doesn't > require counting through the whole string, when all you want to know is > whether it is nonempty. > > + // copy over everything up to the first '$' character >> + const char *t_next = strchr(t_cur, '$'); >> + if (t_next) { >> + int num_copy_bytes = FFMIN(t_next - t_cur, buffer_size - >> dst_pos - 1); >> + av_strlcpy(&dst[dst_pos], t_cur, num_copy_bytes + 1); >> + // advance >> + dst_pos += num_copy_bytes; >> + t_cur = t_next; >> + } else { // no more DASH identifiers to substitute - just copy >> the rest over and break >> + av_strlcpy(&dst[dst_pos], t_cur, buffer_size - dst_pos); >> + break; >> + } >> + >> + if (dst_pos >= buffer_size - 1 || strlen(t_cur) == 0) >> + break; >> > > Similarly you could use !*t_cur here instead of strlen > > + >> + // t_cur is now pointing to a '$' character >> + char format_tag[7]; // May be "%d", "%0Xd", or "%0Xlld" (for >> $Time$), where X is in [0-9] >> + int n; >> + DASHTmplId id_type = dash_read_format_tag(t_cur, format_tag, >> &t_next); >> > > We avoid mixed statements and declarations in our codebase, even though > it's allowed by C99, so please declare the variables at the start of the > current scope > > + switch (id_type) { >> + case (DASH_TMPL_ID_REP_ID): >> > > nit: No need for parentheses around the enums > > + n = snprintf(&dst[dst_pos], buffer_size - dst_pos, >> format_tag, rep_id); >> + break; >> + case (DASH_TMPL_ID_NUMBER): >> + n = snprintf(&dst[dst_pos], buffer_size - dst_pos, >> format_tag, number); >> + break; >> + case (DASH_TMPL_ID_BANDWIDTH): >> + n = snprintf(&dst[dst_pos], buffer_size - dst_pos, >> format_tag, bit_rate); >> + break; >> + case (DASH_TMPL_ID_TIME): >> + n = snprintf(&dst[dst_pos], buffer_size - dst_pos, >> format_tag, time); >> + break; >> + case (DASH_TMPL_ID_UNDEFINED): >> + // copy over one byte and advance >> + dst[dst_pos] = t_cur[0]; >> + n = 1; >> + t_next = &t_cur[1]; >> > > I'm a bit hesitant here - in all other cases, the output buffer is always > null-terminated, except here. If the while-loop condtition turns out false > after this loop round, you would return with a non-terminated buffer. > > + break; >> + } >> + // t_next points just past the processed identifier >> + // n is either the number of bytes that were attempted to be >> written to dst >> + // (may have failed to write all because buffer-size). >> > > nit: buffer_size > > > + >> + // advance >> + dst_pos += FFMIN(n, buffer_size - dst_pos - 1); >> + t_cur = t_next; >> + } >> +} >> + >> static char *xmlescape(const char *str) { >> int outlen = strlen(str)*3/2 + 6; >> char *out = av_realloc(NULL, outlen + 1); >> @@ -403,7 +537,7 @@ static int dash_write_header(AVFormatContext *s) >> char *ptr; >> char basename[1024]; >> >> - if (c->single_file) >> + if (c->single_file_name) >> c->use_template = 0; >> >> av_strlcpy(c->dirname, s->filename, sizeof(c->dirname)); >> @@ -439,9 +573,10 @@ static int dash_write_header(AVFormatContext *s) >> AVDictionary *opts = NULL; >> char filename[1024]; >> >> - if (s->streams[i]->codec->bit_rate) { >> + os->bit_rate = s->streams[i]->codec->bit_rate >> + if (os->bit_rate) { >> snprintf(os->bandwidth_str, sizeof(os->bandwidth_str), >> - " bandwidth=\"%d\"", s->streams[i]->codec->bit_ >> rate); >> + " bandwidth=\"%d\"", os->bit_rate); >> } else { >> int level = s->strict_std_compliance >= FF_COMPLIANCE_STRICT ? >> AV_LOG_ERROR : AV_LOG_WARNING; >> @@ -476,10 +611,10 @@ static int dash_write_header(AVFormatContext *s) >> goto fail; >> } >> >> - if (c->single_file) >> - snprintf(os->initfile, sizeof(os->initfile), >> "%s-stream%d.m4s", basename, i); >> + if (c->single_file_name) >> + dash_fill_tmpl_params(os->initfile, sizeof(os->initfile), >> c->single_file_name, i, 0, os->bit_rate, 0); >> else >> - snprintf(os->initfile, sizeof(os->initfile), >> "init-stream%d.m4s", i); >> + dash_fill_tmpl_params(os->initfile, sizeof(os->initfile), >> c->init_seg_name, i, 0, os->bit_rate, 0); >> snprintf(filename, sizeof(filename), "%s%s", c->dirname, >> os->initfile); >> ret = ffurl_open(&os->out, filename, AVIO_FLAG_WRITE, >> &s->interrupt_callback, NULL); >> if (ret < 0) >> @@ -494,7 +629,7 @@ static int dash_write_header(AVFormatContext *s) >> avio_flush(ctx->pb); >> av_dict_free(&opts); >> >> - if (c->single_file) { >> + if (c->single_file_name) { >> os->init_range_length = avio_tell(ctx->pb); >> } else { >> ffurl_close(os->out); >> @@ -623,8 +758,8 @@ static int dash_flush(AVFormatContext *s, int final, >> int stream) >> continue; >> } >> >> - if (!c->single_file) { >> - snprintf(filename, sizeof(filename), >> "chunk-stream%d-%05d.m4s", i, os->segment_index); >> + if (!c->single_file_name) { >> + dash_fill_tmpl_params(filename, sizeof(filename), >> c->media_seg_name, i, os->segment_index, os->bit_rate, os->start_dts); >> snprintf(full_path, sizeof(full_path), "%s%s", c->dirname, >> filename); >> snprintf(temp_path, sizeof(temp_path), "%s.tmp", full_path); >> ret = ffurl_open(&os->out, temp_path, AVIO_FLAG_WRITE, >> &s->interrupt_callback, NULL); >> @@ -637,7 +772,7 @@ static int dash_flush(AVFormatContext *s, int final, >> int stream) >> os->packets_written = 0; >> >> range_length = avio_tell(os->ctx->pb) - start_pos; >> - if (c->single_file) { >> + if (c->single_file_name) { >> find_index_range(s, c->dirname, os->initfile, start_pos, >> &index_length); >> } else { >> ffurl_close(os->out); >> @@ -768,7 +903,9 @@ static const AVOption options[] = { >> { "remove_at_exit", "remove all segments when finished", >> OFFSET(remove_at_exit), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, E }, >> { "use_template", "Use SegmentTemplate instead of SegmentList", >> OFFSET(use_template), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, E }, >> { "use_timeline", "Use SegmentTimeline in SegmentTemplate", >> OFFSET(use_timeline), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, E }, >> - { "single_file", "Store all segments in one file, accessed using >> byte ranges", OFFSET(single_file), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, E }, >> + { "init_seg_name", "DASH-templated name to used for the >> initialization segment", OFFSET(init_seg_name), AV_OPT_TYPE_STRING, {.str = >> "init-stream$RepresentationID$.m4s"}, 0, 0, E }, >> + { "media_seg_name", "DASH-templated name to used for the media >> segments", OFFSET(media_seg_name), AV_OPT_TYPE_STRING, {.str = >> "chunk-stream$RepresentationID$-$Number%05d$.m4s"}, 0, 0, E }, >> + { "single_file_name", "DASH-templated name to be used for baseURL. >> Implies storing all segments in one file, accessed using byte ranges", >> OFFSET(single_file_name), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E }, >> { NULL }, >> > > I would kinda like to keep the single_file option - it's useful if one > wants to use that option but don't care about what the actual file name is > and don't want to try to come up with a usable one (e.g. if you pick a bad > name which doesn't contain any identifier, you'd have all tracks using the > same file name). > > Other than these minor details, it looks quite good to me! > > // Martin > _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
