diff mbox series

[FFmpeg-devel,2/2] avformat/dashdec: Don't needlessly strdup metadata

Message ID 20210302145351.514945-2-andreas.rheinhardt@gmail.com
State Accepted
Commit ea3953ad406e96e95c533ef850c6395da6a7dd86
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>
---
Using separate lists for audio, video and subtitles leads to lots of
code duplication in this demuxer; the only place where one gains a bit
is in refresh_manifest (which is already buggy in itself because it
leaks upon error...). Is it really worth it?

 libavformat/dashdec.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

James Almer March 2, 2021, 3:12 p.m. UTC | #1
On 3/2/2021 11:53 AM, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Using separate lists for audio, video and subtitles leads to lots of
> code duplication in this demuxer; the only place where one gains a bit
> is in refresh_manifest (which is already buggy in itself because it
> leaks upon error...). Is it really worth it?
> 
>   libavformat/dashdec.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 947b42f816..6f3f28dcc7 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -2039,6 +2039,14 @@ static int copy_init_section(struct representation *rep_dest, struct representat
>   
>   static int dash_close(AVFormatContext *s);
>   
> +static void move_metadata(AVStream *st, const char *key, char **value)
> +{
> +    if (*value) {
> +        av_dict_set(&st->metadata, key, *value, AV_DICT_DONT_STRDUP_VAL);
> +        *value = NULL;
> +    }
> +}
> +
>   static int dash_read_header(AVFormatContext *s)
>   {
>       DASHContext *c = s->priv_data;
> @@ -2137,8 +2145,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)
> -            av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0);
> +        move_metadata(rep->assoc_stream, "id", &rep->id);
>       }
>       for (i = 0; i < c->n_audios; i++) {
>           rep = c->audios[i];
> @@ -2146,23 +2153,15 @@ 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)
> -            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);
> -            av_freep(&rep->lang);
> -        }
> +        move_metadata(rep->assoc_stream, "id", &rep->id);
> +        move_metadata(rep->assoc_stream, "language", &rep->lang);
>       }
>       for (i = 0; i < c->n_subtitles; i++) {
>           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)
> -            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);
> -            av_freep(&rep->lang);
> -        }
> +        move_metadata(rep->assoc_stream, "id", &rep->id);

Well, forget what i said at the end of my review for the previous patch :p

> +        move_metadata(rep->assoc_stream, "language", &rep->lang);
>       }
>   
>       return 0;
>
diff mbox series

Patch

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 947b42f816..6f3f28dcc7 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -2039,6 +2039,14 @@  static int copy_init_section(struct representation *rep_dest, struct representat
 
 static int dash_close(AVFormatContext *s);
 
+static void move_metadata(AVStream *st, const char *key, char **value)
+{
+    if (*value) {
+        av_dict_set(&st->metadata, key, *value, AV_DICT_DONT_STRDUP_VAL);
+        *value = NULL;
+    }
+}
+
 static int dash_read_header(AVFormatContext *s)
 {
     DASHContext *c = s->priv_data;
@@ -2137,8 +2145,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)
-            av_dict_set(&rep->assoc_stream->metadata, "id", rep->id, 0);
+        move_metadata(rep->assoc_stream, "id", &rep->id);
     }
     for (i = 0; i < c->n_audios; i++) {
         rep = c->audios[i];
@@ -2146,23 +2153,15 @@  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)
-            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);
-            av_freep(&rep->lang);
-        }
+        move_metadata(rep->assoc_stream, "id", &rep->id);
+        move_metadata(rep->assoc_stream, "language", &rep->lang);
     }
     for (i = 0; i < c->n_subtitles; i++) {
         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)
-            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);
-            av_freep(&rep->lang);
-        }
+        move_metadata(rep->assoc_stream, "id", &rep->id);
+        move_metadata(rep->assoc_stream, "language", &rep->lang);
     }
 
     return 0;