[FFmpeg-devel,4/5] lavf/dashdec: improve memory handling

- Fixes a couple leaks
- Adds an av_freep equivalent for libxml buffers
- Avoids some redundant copying
---
 libavformat/dashdec.c | 44 +++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

rcombs:
> - Fixes a couple leaks
> - Adds an av_freep equivalent for libxml buffers
> - Avoids some redundant copying
> ---
>  libavformat/dashdec.c | 44 +++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index c94ce2caca..378c892b87 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -147,9 +147,6 @@ typedef struct DASHContext {
>      uint64_t period_duration;
>      uint64_t period_start;
>  
> -    /* AdaptationSet Attribute */
> -    char *adaptationset_lang;
> -
>      int is_live;
>      AVIOInterruptCB *interrupt_callback;
>      char *allowed_extensions;
> @@ -162,6 +159,15 @@ typedef struct DASHContext {
>  
>  } DASHContext;
>  
> +static void xml_freep(void* arg)
> +{
> +    void *val;
> +
> +    memcpy(&val, arg, sizeof(val));
> +    memcpy(arg, &(void *){ NULL }, sizeof(val));
> +    xmlFree(val);
> +}
> +

libxml2 allows to use custom memory allocation functions. Have you tried
to just use our allocation functions which would allow to use av_freep
directly?

>  static int ishttp(char *url)
>  {
>      const char *proto_name = avio_find_protocol_name(url);
> @@ -362,6 +368,8 @@ static void free_representation(struct representation *pls)
>          avformat_close_input(&pls->ctx);
>      }
>  
> +    xml_freep(&pls->lang);
> +
>      av_freep(&pls->url_template);
>      av_freep(&pls);
>  }
> @@ -878,15 +886,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>              ret = AVERROR(ENOMEM);
>              goto end;
>          }
> -        if (c->adaptationset_lang) {
> -            rep->lang = av_strdup(c->adaptationset_lang);
> -            if (!rep->lang) {
> -                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
> -                av_freep(&rep);
> -                ret = AVERROR(ENOMEM);
> -                goto end;
> -            }
> -        }
> +
> +        rep->lang = xmlGetProp(adaptationset_node, "lang");
> +
>          rep->parent = s;
>          representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate");
>          representation_baseurl_node = find_child_node_by_name(representation_node, "BaseURL");
> @@ -965,7 +967,8 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>                  xmlFree(startnumber_val);
>              }
>              if (adaptationset_supplementalproperty_node) {
> -                if (!av_strcasecmp(xmlGetProp(adaptationset_supplementalproperty_node,"schemeIdUri"), "http://dashif.org/guidelines/last-segment-number")) {
> +                char *schemeIdUri = xmlGetProp(adaptationset_supplementalproperty_node, "schemeIdUri");
> +                if (!av_strcasecmp(schemeIdUri, "http://dashif.org/guidelines/last-segment-number")) {
>                      val = xmlGetProp(adaptationset_supplementalproperty_node,"value");
>                      if (!val) {
>                          av_log(s, AV_LOG_ERROR, "Missing value attribute in adaptationset_supplementalproperty_node\n");
> @@ -974,6 +977,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>                          xmlFree(val);
>                      }
>                  }
> +                xmlFree(schemeIdUri);
>              }
>  
>              fragment_timeline_node = find_child_node_by_name(representation_segmenttemplate_node, "SegmentTimeline");
> @@ -1120,13 +1124,10 @@ end:
>  
>  static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptationset_node)
>  {
> -    DASHContext *c = s->priv_data;
> -
>      if (!adaptationset_node) {
>          av_log(s, AV_LOG_WARNING, "Cannot get AdaptationSet\n");
>          return AVERROR(EINVAL);
>      }
> -    c->adaptationset_lang = xmlGetProp(adaptationset_node, "lang");
>  
>      return 0;
>  }
> @@ -1139,7 +1140,6 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>                                          xmlNodePtr period_segmentlist_node)
>  {
>      int ret = 0;
> -    DASHContext *c = s->priv_data;
>      xmlNodePtr fragment_template_node = NULL;
>      xmlNodePtr content_component_node = NULL;
>      xmlNodePtr adaptationset_baseurl_node = NULL;
> @@ -1182,7 +1182,6 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>      }
>  
>  err:
> -    av_freep(&c->adaptationset_lang);
>      return ret;
>  }
>  
> @@ -2157,6 +2156,10 @@ static int dash_read_header(AVFormatContext *s)
>                  av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0);
>              if (rep->id[0])
>                  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);
> +                xml_freep(&rep->lang);
> +            }
>          }
>          for (i = 0; i < c->n_audios; i++) {
>              rep = c->audios[i];
> @@ -2168,7 +2171,7 @@ static int dash_read_header(AVFormatContext *s)
>                  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);
> +                xml_freep(&rep->lang);
>              }
>          }
>          for (i = 0; i < c->n_subtitles; i++) {
> @@ -2179,7 +2182,7 @@ static int dash_read_header(AVFormatContext *s)
>                  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);
> +                xml_freep(&rep->lang);
>              }
>          }
>      }
> @@ -2282,6 +2285,7 @@ static int dash_close(AVFormatContext *s)
>      DASHContext *c = s->priv_data;
>      free_audio_list(c);
>      free_video_list(c);
> +    free_subtitle_list(c);
>      av_dict_free(&c->avio_opts);
>      av_freep(&c->base_url);
>      return 0;
>

> On Jun 11, 2020, at 10:19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> 
> rcombs:
>> - Fixes a couple leaks
>> - Adds an av_freep equivalent for libxml buffers
>> - Avoids some redundant copying
>> ---
>> libavformat/dashdec.c | 44 +++++++++++++++++++++++--------------------
>> 1 file changed, 24 insertions(+), 20 deletions(-)
>> 
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index c94ce2caca..378c892b87 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -147,9 +147,6 @@ typedef struct DASHContext {
>>     uint64_t period_duration;
>>     uint64_t period_start;
>> 
>> -    /* AdaptationSet Attribute */
>> -    char *adaptationset_lang;
>> -
>>     int is_live;
>>     AVIOInterruptCB *interrupt_callback;
>>     char *allowed_extensions;
>> @@ -162,6 +159,15 @@ typedef struct DASHContext {
>> 
>> } DASHContext;
>> 
>> +static void xml_freep(void* arg)
>> +{
>> +    void *val;
>> +
>> +    memcpy(&val, arg, sizeof(val));
>> +    memcpy(arg, &(void *){ NULL }, sizeof(val));
>> +    xmlFree(val);
>> +}
>> +
> 
> libxml2 allows to use custom memory allocation functions. Have you tried
> to just use our allocation functions which would allow to use av_freep
> directly?

This would work in theory, but it's not library-safe: if any other library (or the main application) also overrode the libxml2 allocation functions, then this could fail (and if some other code relied on that functionality, we could break that). I'd rather not rely on global state in a dependency lib like that if not absolutely necessary.

> 
>> static int ishttp(char *url)
>> {
>>     const char *proto_name = avio_find_protocol_name(url);
>> @@ -362,6 +368,8 @@ static void free_representation(struct representation *pls)
>>         avformat_close_input(&pls->ctx);
>>     }
>> 
>> +    xml_freep(&pls->lang);
>> +
>>     av_freep(&pls->url_template);
>>     av_freep(&pls);
>> }
>> @@ -878,15 +886,9 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>             ret = AVERROR(ENOMEM);
>>             goto end;
>>         }
>> -        if (c->adaptationset_lang) {
>> -            rep->lang = av_strdup(c->adaptationset_lang);
>> -            if (!rep->lang) {
>> -                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>> -                av_freep(&rep);
>> -                ret = AVERROR(ENOMEM);
>> -                goto end;
>> -            }
>> -        }
>> +
>> +        rep->lang = xmlGetProp(adaptationset_node, "lang");
>> +
>>         rep->parent = s;
>>         representation_segmenttemplate_node = find_child_node_by_name(representation_node, "SegmentTemplate");
>>         representation_baseurl_node = find_child_node_by_name(representation_node, "BaseURL");
>> @@ -965,7 +967,8 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>                 xmlFree(startnumber_val);
>>             }
>>             if (adaptationset_supplementalproperty_node) {
>> -                if (!av_strcasecmp(xmlGetProp(adaptationset_supplementalproperty_node,"schemeIdUri"), "http://dashif.org/guidelines/last-segment-number")) {
>> +                char *schemeIdUri = xmlGetProp(adaptationset_supplementalproperty_node, "schemeIdUri");
>> +                if (!av_strcasecmp(schemeIdUri, "http://dashif.org/guidelines/last-segment-number")) {
>>                     val = xmlGetProp(adaptationset_supplementalproperty_node,"value");
>>                     if (!val) {
>>                         av_log(s, AV_LOG_ERROR, "Missing value attribute in adaptationset_supplementalproperty_node\n");
>> @@ -974,6 +977,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>                         xmlFree(val);
>>                     }
>>                 }
>> +                xmlFree(schemeIdUri);
>>             }
>> 
>>             fragment_timeline_node = find_child_node_by_name(representation_segmenttemplate_node, "SegmentTimeline");
>> @@ -1120,13 +1124,10 @@ end:
>> 
>> static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptationset_node)
>> {
>> -    DASHContext *c = s->priv_data;
>> -
>>     if (!adaptationset_node) {
>>         av_log(s, AV_LOG_WARNING, "Cannot get AdaptationSet\n");
>>         return AVERROR(EINVAL);
>>     }
>> -    c->adaptationset_lang = xmlGetProp(adaptationset_node, "lang");
>> 
>>     return 0;
>> }
>> @@ -1139,7 +1140,6 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>                                         xmlNodePtr period_segmentlist_node)
>> {
>>     int ret = 0;
>> -    DASHContext *c = s->priv_data;
>>     xmlNodePtr fragment_template_node = NULL;
>>     xmlNodePtr content_component_node = NULL;
>>     xmlNodePtr adaptationset_baseurl_node = NULL;
>> @@ -1182,7 +1182,6 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>     }
>> 
>> err:
>> -    av_freep(&c->adaptationset_lang);
>>     return ret;
>> }
>> 
>> @@ -2157,6 +2156,10 @@ static int dash_read_header(AVFormatContext *s)
>>                 av_dict_set_int(&rep->assoc_stream->metadata, "variant_bitrate", rep->bandwidth, 0);
>>             if (rep->id[0])
>>                 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);
>> +                xml_freep(&rep->lang);
>> +            }
>>         }
>>         for (i = 0; i < c->n_audios; i++) {
>>             rep = c->audios[i];
>> @@ -2168,7 +2171,7 @@ static int dash_read_header(AVFormatContext *s)
>>                 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);
>> +                xml_freep(&rep->lang);
>>             }
>>         }
>>         for (i = 0; i < c->n_subtitles; i++) {
>> @@ -2179,7 +2182,7 @@ static int dash_read_header(AVFormatContext *s)
>>                 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);
>> +                xml_freep(&rep->lang);
>>             }
>>         }
>>     }
>> @@ -2282,6 +2285,7 @@ static int dash_close(AVFormatContext *s)
>>     DASHContext *c = s->priv_data;
>>     free_audio_list(c);
>>     free_video_list(c);
>> +    free_subtitle_list(c);
>>     av_dict_free(&c->avio_opts);
>>     av_freep(&c->base_url);
>>     return 0;
>> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".