diff mbox series

[FFmpeg-devel,1/2] avformat/dashdec: Remove limit on length of id

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
Related show

Checks

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

Commit Message

Andreas Rheinhardt March 2, 2021, 2:53 p.m. UTC
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(-)

Comments

James Almer March 2, 2021, 3:10 p.m. UTC | #1
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 mbox series

Patch

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);