| Message ID | 20210302145351.514945-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9a38e6ff9745ee07ce82beee9dd2259b4ce2b73b |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/dashdec: Remove limit on length of id | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 3/2/2021 11:53 AM, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > You like this more? Yes, otherwise as there's no defined upper limit we'll just be increasing the fixed array every now and then when new manifest files show up with increasingly verbose ids. > (It seems to me that the earlier code would have use a NULL pointer for > %s in av_log if there is no id.) Since the field is mandatory, chances are it never happened because no valid manifest would be missing it. > > libavformat/dashdec.c | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 6b43fd6278..947b42f816 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -82,7 +82,7 @@ struct representation { > AVFormatContext *ctx; > int stream_index; > > - char id[32]; > + char *id; > char *lang; > int bandwidth; Unrelated to this patch, but bandwidth is an unsigned int. https://github.com/Dash-Industry-Forum/DASH/blob/master/mpdvalidator/schemas/DASH-MPD.xsd#L329 > AVRational framerate; > @@ -361,6 +361,7 @@ static void free_representation(struct representation *pls) > > av_freep(&pls->url_template); > av_freep(&pls->lang); > + av_freep(&pls->id); > av_freep(&pls); > } > > @@ -842,7 +843,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > char *val = NULL; > xmlNodePtr baseurl_nodes[4]; > xmlNodePtr representation_node = node; > - char *rep_id_val, *rep_bandwidth_val; > + char *rep_bandwidth_val; > enum AVMediaType type = AVMEDIA_TYPE_UNKNOWN; > > // try get information from representation > @@ -876,8 +877,14 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate"); > representation_baseurl_node = find_child_node_by_name(representation_node, "BaseURL"); > representation_segmentlist_node = find_child_node_by_name(representation_node, "SegmentList"); > - rep_id_val = xmlGetProp(representation_node, "id"); > rep_bandwidth_val = xmlGetProp(representation_node, "bandwidth"); > + val = xmlGetProp(representation_node, "id"); > + if (val) { > + rep->id = av_strdup(val); > + xmlFree(val); > + if (!rep->id) > + goto enomem; > + } > > baseurl_nodes[0] = mpd_baseurl_node; > baseurl_nodes[1] = period_baseurl_node; > @@ -886,7 +893,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > > ret = resolve_content_path(s, url, &c->max_url_size, baseurl_nodes, 4); > c->max_url_size = aligned(c->max_url_size > - + (rep_id_val ? strlen(rep_id_val) : 0) > + + (rep->id ? strlen(rep->id) : 0) > + (rep_bandwidth_val ? strlen(rep_bandwidth_val) : 0)); > if (ret == AVERROR(ENOMEM) || ret == 0) > goto free; > @@ -906,7 +913,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > goto enomem; > } > c->max_url_size = aligned(c->max_url_size + strlen(val)); > - rep->init_section->url = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, val); > + rep->init_section->url = get_content_url(baseurl_nodes, 4, > + c->max_url_size, rep->id, > + rep_bandwidth_val, val); > xmlFree(val); > if (!rep->init_section->url) > goto enomem; > @@ -915,7 +924,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > val = get_val_from_nodes_tab(fragment_templates_tab, 4, "media"); > if (val) { > c->max_url_size = aligned(c->max_url_size + strlen(val)); > - rep->url_template = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, val); > + rep->url_template = get_content_url(baseurl_nodes, 4, > + c->max_url_size, rep->id, > + rep_bandwidth_val, val); > xmlFree(val); > } > val = get_val_from_nodes_tab(fragment_templates_tab, 4, "presentationTimeOffset"); > @@ -980,7 +991,8 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > av_free(seg); > goto free; > } > - seg->url = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, NULL); > + seg->url = get_content_url(baseurl_nodes, 4, c->max_url_size, > + rep->id, rep_bandwidth_val, NULL); > if (!seg->url) > goto enomem; > seg->size = -1; > @@ -1014,8 +1026,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > fragmenturl_node = xmlFirstElementChild(representation_segmentlist_node); > while (fragmenturl_node) { > ret = parse_manifest_segmenturlnode(s, rep, fragmenturl_node, > - baseurl_nodes, > - rep_id_val, > + baseurl_nodes, rep->id, > rep_bandwidth_val); > if (ret < 0) > goto free; > @@ -1035,15 +1046,14 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > } > } > } else { > - av_log(s, AV_LOG_ERROR, "Unknown format of Representation node id[%s] \n", rep_id_val); > + av_log(s, AV_LOG_ERROR, "Unknown format of Representation node id '%s' \n", > + rep->id ? rep->id : ""); > goto free; > } > > if (rep->fragment_duration > 0 && !rep->fragment_timescale) > rep->fragment_timescale = 1; > rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0; > - if (rep_id_val) > - av_strlcpy(rep->id, rep_id_val, sizeof(rep->id)); > rep->framerate = av_make_q(0, 0); > if (type == AVMEDIA_TYPE_VIDEO) { > char *rep_framerate_val = xmlGetProp(representation_node, "frameRate"); > @@ -1070,8 +1080,6 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, > goto free; > > end: > - if (rep_id_val) > - xmlFree(rep_id_val); > if (rep_bandwidth_val) > xmlFree(rep_bandwidth_val); > > @@ -2129,7 +2137,7 @@ static int dash_read_header(AVFormatContext *s) > rep->assoc_stream = s->streams[rep->stream_index]; > if (rep->bandwidth > 0) > av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0); > - if (rep->id[0]) > + if (rep->id) > av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); > } > for (i = 0; i < c->n_audios; i++) { > @@ -2138,7 +2146,7 @@ static int dash_read_header(AVFormatContext *s) > rep->assoc_stream = s->streams[rep->stream_index]; > if (rep->bandwidth > 0) > av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0); > - if (rep->id[0]) > + if (rep->id) > av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); > if (rep->lang) { > av_dict_set(&rep->assoc_stream->metadata, "language", rep->lang, 0); > @@ -2149,7 +2157,7 @@ static int dash_read_header(AVFormatContext *s) > rep = c->subtitles[i]; > av_program_add_stream_index(s, 0, rep->stream_index); > rep->assoc_stream = s->streams[rep->stream_index]; > - if (rep->id[0]) > + if (rep->id) > av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); nit: You could use AV_DICT_DONT_STRDUP_VAL in all three of these and set rep->id to NULL afterwards, but it's probably a pointless micro optimization when parsing a small manifest file. Should be ok either way. > if (rep->lang) { > av_dict_set(&rep->assoc_stream->metadata, "language", rep->lang, 0); >
diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index 6b43fd6278..947b42f816 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -82,7 +82,7 @@ struct representation { AVFormatContext *ctx; int stream_index; - char id[32]; + char *id; char *lang; int bandwidth; AVRational framerate; @@ -361,6 +361,7 @@ static void free_representation(struct representation *pls) av_freep(&pls->url_template); av_freep(&pls->lang); + av_freep(&pls->id); av_freep(&pls); } @@ -842,7 +843,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, char *val = NULL; xmlNodePtr baseurl_nodes[4]; xmlNodePtr representation_node = node; - char *rep_id_val, *rep_bandwidth_val; + char *rep_bandwidth_val; enum AVMediaType type = AVMEDIA_TYPE_UNKNOWN; // try get information from representation @@ -876,8 +877,14 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate"); representation_baseurl_node = find_child_node_by_name(representation_node, "BaseURL"); representation_segmentlist_node = find_child_node_by_name(representation_node, "SegmentList"); - rep_id_val = xmlGetProp(representation_node, "id"); rep_bandwidth_val = xmlGetProp(representation_node, "bandwidth"); + val = xmlGetProp(representation_node, "id"); + if (val) { + rep->id = av_strdup(val); + xmlFree(val); + if (!rep->id) + goto enomem; + } baseurl_nodes[0] = mpd_baseurl_node; baseurl_nodes[1] = period_baseurl_node; @@ -886,7 +893,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, ret = resolve_content_path(s, url, &c->max_url_size, baseurl_nodes, 4); c->max_url_size = aligned(c->max_url_size - + (rep_id_val ? strlen(rep_id_val) : 0) + + (rep->id ? strlen(rep->id) : 0) + (rep_bandwidth_val ? strlen(rep_bandwidth_val) : 0)); if (ret == AVERROR(ENOMEM) || ret == 0) goto free; @@ -906,7 +913,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, goto enomem; } c->max_url_size = aligned(c->max_url_size + strlen(val)); - rep->init_section->url = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, val); + rep->init_section->url = get_content_url(baseurl_nodes, 4, + c->max_url_size, rep->id, + rep_bandwidth_val, val); xmlFree(val); if (!rep->init_section->url) goto enomem; @@ -915,7 +924,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, val = get_val_from_nodes_tab(fragment_templates_tab, 4, "media"); if (val) { c->max_url_size = aligned(c->max_url_size + strlen(val)); - rep->url_template = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, val); + rep->url_template = get_content_url(baseurl_nodes, 4, + c->max_url_size, rep->id, + rep_bandwidth_val, val); xmlFree(val); } val = get_val_from_nodes_tab(fragment_templates_tab, 4, "presentationTimeOffset"); @@ -980,7 +991,8 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, av_free(seg); goto free; } - seg->url = get_content_url(baseurl_nodes, 4, c->max_url_size, rep_id_val, rep_bandwidth_val, NULL); + seg->url = get_content_url(baseurl_nodes, 4, c->max_url_size, + rep->id, rep_bandwidth_val, NULL); if (!seg->url) goto enomem; seg->size = -1; @@ -1014,8 +1026,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, fragmenturl_node = xmlFirstElementChild(representation_segmentlist_node); while (fragmenturl_node) { ret = parse_manifest_segmenturlnode(s, rep, fragmenturl_node, - baseurl_nodes, - rep_id_val, + baseurl_nodes, rep->id, rep_bandwidth_val); if (ret < 0) goto free; @@ -1035,15 +1046,14 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, } } } else { - av_log(s, AV_LOG_ERROR, "Unknown format of Representation node id[%s] \n", rep_id_val); + av_log(s, AV_LOG_ERROR, "Unknown format of Representation node id '%s' \n", + rep->id ? rep->id : ""); goto free; } if (rep->fragment_duration > 0 && !rep->fragment_timescale) rep->fragment_timescale = 1; rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0; - if (rep_id_val) - av_strlcpy(rep->id, rep_id_val, sizeof(rep->id)); rep->framerate = av_make_q(0, 0); if (type == AVMEDIA_TYPE_VIDEO) { char *rep_framerate_val = xmlGetProp(representation_node, "frameRate"); @@ -1070,8 +1080,6 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url, goto free; end: - if (rep_id_val) - xmlFree(rep_id_val); if (rep_bandwidth_val) xmlFree(rep_bandwidth_val); @@ -2129,7 +2137,7 @@ static int dash_read_header(AVFormatContext *s) rep->assoc_stream = s->streams[rep->stream_index]; if (rep->bandwidth > 0) av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0); - if (rep->id[0]) + if (rep->id) av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); } for (i = 0; i < c->n_audios; i++) { @@ -2138,7 +2146,7 @@ static int dash_read_header(AVFormatContext *s) rep->assoc_stream = s->streams[rep->stream_index]; if (rep->bandwidth > 0) av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0); - if (rep->id[0]) + if (rep->id) av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); if (rep->lang) { av_dict_set(&rep->assoc_stream->metadata, "language", rep->lang, 0); @@ -2149,7 +2157,7 @@ static int dash_read_header(AVFormatContext *s) rep = c->subtitles[i]; av_program_add_stream_index(s, 0, rep->stream_index); rep->assoc_stream = s->streams[rep->stream_index]; - if (rep->id[0]) + if (rep->id) av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0); if (rep->lang) { av_dict_set(&rep->assoc_stream->metadata, "language", rep->lang, 0);
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- You like this more? (It seems to me that the earlier code would have use a NULL pointer for %s in av_log if there is no id.) libavformat/dashdec.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)