diff mbox series

[FFmpeg-devel,v3] avformat/dashdec: fix memleak for commit commit e134c203

Message ID 20200324044144.51979-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel,v3] avformat/dashdec: fix memleak for commit commit e134c203 | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Liu Steven March 24, 2020, 4:41 a.m. UTC 
These member will be used for get more correct information of the MPD

Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
Suggested-by: Nicolas George <george@nsup.org>
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 214 insertions(+), 30 deletions(-)

Comments

Nicolas George March 26, 2020, 3:46 p.m. UTC | #1 
Steven Liu (12020-03-24):
> These member will be used for get more correct information of the MPD
> 
> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> Suggested-by: Nicolas George <george@nsup.org>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 214 insertions(+), 30 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 5bbe5d3985..b5bb8e674c 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -108,6 +108,7 @@ struct representation {
>      int64_t cur_seg_offset;
>      int64_t cur_seg_size;
>      struct fragment *cur_seg;
> +    char *lang;
>  
>      /* Currently active Media Initialization Section */
>      struct fragment *init_section;
> @@ -122,19 +123,15 @@ struct representation {
>  typedef struct DASHContext {
>      const AVClass *class;
>      char *base_url;
> -    char *adaptionset_contenttype_val;
> -    char *adaptionset_par_val;
> -    char *adaptionset_lang_val;
> -    char *adaptionset_minbw_val;
> -    char *adaptionset_maxbw_val;
> -    char *adaptionset_minwidth_val;
> -    char *adaptionset_maxwidth_val;
> -    char *adaptionset_minheight_val;
> -    char *adaptionset_maxheight_val;
> -    char *adaptionset_minframerate_val;
> -    char *adaptionset_maxframerate_val;
> -    char *adaptionset_segmentalignment_val;
> -    char *adaptionset_bitstreamswitching_val;
> +    char *adaptionset_lang;
> +    uint64_t adaptionset_minbw;
> +    uint64_t adaptionset_maxbw;
> +    uint64_t adaptionset_minwidth;
> +    uint64_t adaptionset_maxwidth;
> +    uint64_t adaptionset_minheight;
> +    uint64_t adaptionset_maxheight;
> +    AVRational adaptionset_minframerate;
> +    AVRational adaptionset_maxframerate;
>  
>      int n_videos;
>      struct representation **videos;
> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>  
>  } DASHContext;
>  

> +static int get_ratio_from_string(AVRational *dst, char *src)

const char *

> +{

> +    char *p = src;

Not useful.

> +    char *end = NULL;
> +    char *end_ptr = NULL;
> +    int num, den;
> +

> +    num = (int)strtol(p, &end_ptr, 10);

Possible integer overflow.

> +    if (errno == ERANGE || end_ptr == src) {
> +        return AVERROR_INVALIDDATA;
> +    }

Unnecessary braces.

> +
> +    if (end_ptr[0] == '\0') {
> +        dst->den = 1;
> +        dst->num = num;
> +        return 0;
> +    }
> +
> +    if (end_ptr[0] != '/')
> +        return AVERROR_INVALIDDATA;

> +    p = end_ptr + 1;

Not useful.

> +    den = (int)strtol(p, &end, 10);

Possible integer overflow.

> +    if (errno == ERANGE || end == src) {

> +        dst->den = 1;

Why?

> +        return AVERROR_INVALIDDATA;
> +    }
> +    dst->den = den;
> +
> +    return 0;
> +}
> +

> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)

The return type does not work.

> +{
> +    char *end_ptr = NULL;
> +    char *val = xmlGetProp(node, key);
> +    int ret = 0;
> +
> +    if (val) {

> +        ret = strtoull(val, &end_ptr, 10);

Can't store unsigned long long into int.

> +        if (errno == ERANGE) {
> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
> +            xmlFree(val);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        if (end_ptr == val || end_ptr[0] != '\0') {
> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
> +            "not a valid number: %s\n", key, val);
> +            xmlFree(val);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        *dst = ret;
> +        xmlFree(val);
> +    }
> +    return ret;
> +}
> +
> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
> +{
> +    char *val = xmlGetProp(node, key);
> +    int ret = 0;
> +    AVRational rate;
> +
> +    if (val) {
> +        ret = get_ratio_from_string(&rate, val);
> +        if (ret < 0)
> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
> +        xmlFree(val);
> +    }
> +    member->num = rate.num;
> +    member->den = rate.den;
> +    return ret;
> +}
> +
>  static int ishttp(char *url)
>  {
>      const char *proto_name = avio_find_protocol_name(url);
> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>              ret = AVERROR(ENOMEM);
>              goto end;
>          }
> +        if (c->adaptionset_lang) {
> +            rep->lang = av_strdup(c->adaptionset_lang);
> +            if (!rep->lang) {
> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
> +                av_freep(&rep);
> +                ret = AVERROR(ENOMEM);
> +                goto end;
> +            }
> +        }
>          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");
> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>          }
>  
>          if (rep) {
> +            uint64_t width = 0;
> +            uint64_t height = 0;
> +
>              if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>                  rep->fragment_timescale = 1;
>              rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;

> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +
> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
> +            if (ret < 0)
> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
> +                "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +
> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
> +            if (ret < 0)
> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
> +                "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +

Looks unrelated.

>              strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>              rep->framerate = av_make_q(0, 0);
>              if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>                  if (ret < 0)
> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
> +                if (c->adaptionset_maxframerate.num > 0) {
> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                        free_representation(rep);
> +                        goto end;
> +                    }
> +                }
> +                if (c->adaptionset_minframerate.num > 0) {
> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                        free_representation(rep);
> +                        goto end;
> +                    }
> +                }

This is changing the behavior. I don't know if it is in the desirable
direction.

>              }
>  
>              switch (type) {
> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>      subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>  
>  end:
> +
>      if (rep_id_val)
>          xmlFree(rep_id_val);
>      if (rep_bandwidth_val)
> @@ -1116,6 +1242,62 @@ end:
>      return ret;
>  }
>  
> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
> +{
> +    int ret = 0;
> +    DASHContext *c = s->priv_data;
> +
> +    if (!adaptionset_node) {
> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
> +        return AVERROR(EINVAL);
> +    }
> +    c->adaptionset_minframerate = av_make_q(0, 0);
> +    c->adaptionset_maxframerate = av_make_q(0, 0);
> +    c->adaptionset_maxbw = UINT64_MAX;
> +    c->adaptionset_minbw = 0;
> +    c->adaptionset_minwidth = 0;
> +    c->adaptionset_maxwidth = UINT64_MAX;
> +    c->adaptionset_minheight = 0;
> +    c->adaptionset_maxheight = UINT64_MAX;
> +
> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
> +

> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
> +    if (ret  < 0)
> +        c->adaptionset_minbw = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
> +    if (ret <= 0)
> +        c->adaptionset_maxbw = UINT64_MAX;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
> +    if (ret < 0)
> +        c->adaptionset_minwidth = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
> +    if (ret <= 0)
> +        c->adaptionset_maxwidth = UINT64_MAX;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
> +    if (ret < 0)
> +        c->adaptionset_minheight = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
> +    if (ret <= 0)
> +        c->adaptionset_maxheight = UINT64_MAX;
> +
> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
> +    if (ret < 0) {
> +        c->adaptionset_minframerate = av_make_q(0, 0);
> +    }
> +
> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
> +    if (ret < 0)
> +        c->adaptionset_maxframerate = av_make_q(0, 0);

If you don't actually use the return code, then have the function use a
default value directly, that will make the code more compact.

> +
> +    return ret;
> +}
> +
>  static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>                                          xmlNodePtr adaptionset_node,
>                                          xmlNodePtr mpd_baseurl_node,
> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>      xmlNodePtr adaptionset_segmentlist_node = NULL;
>      xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>      xmlNodePtr node = NULL;
> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
> +
> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
> +    if (ret < 0)
> +        return ret;
>  
>      node = xmlFirstElementChild(adaptionset_node);
>      while (node) {
> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>                                                  adaptionset_segmentlist_node,
>                                                  adaptionset_supplementalproperty_node);
>              if (ret < 0) {
> -                return ret;
> +                goto end;
>              }
>          }
>          node = xmlNextElementSibling(node);
>      }
> -    return 0;
> +
> +end:
> +    av_freep(&c->adaptionset_lang);
> +    return ret;
>  }
>  
>  static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
> +                av_freep(&rep->lang);
> +            }
>          }
>          for (i = 0; i < c->n_subtitles; i++) {
>              rep = c->subtitles[i];
> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>              rep->assoc_stream = s->streams[rep->stream_index];
>              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, "lang", rep->lang, 0);
> +                av_freep(&rep->lang);
> +            }
>          }
>      }
>  

Regards,
Liu Steven March 27, 2020, 3:02 a.m. UTC | #2 
> 2020年3月26日 下午11:46，Nicolas George <george@nsup.org> 写道：
> 
> Steven Liu (12020-03-24):
>> These member will be used for get more correct information of the MPD
>> 
>> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> Suggested-by: Nicolas George <george@nsup.org>
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>> libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 214 insertions(+), 30 deletions(-)
>> 
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index 5bbe5d3985..b5bb8e674c 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -108,6 +108,7 @@ struct representation {
>>     int64_t cur_seg_offset;
>>     int64_t cur_seg_size;
>>     struct fragment *cur_seg;
>> +    char *lang;
>> 
>>     /* Currently active Media Initialization Section */
>>     struct fragment *init_section;
>> @@ -122,19 +123,15 @@ struct representation {
>> typedef struct DASHContext {
>>     const AVClass *class;
>>     char *base_url;
>> -    char *adaptionset_contenttype_val;
>> -    char *adaptionset_par_val;
>> -    char *adaptionset_lang_val;
>> -    char *adaptionset_minbw_val;
>> -    char *adaptionset_maxbw_val;
>> -    char *adaptionset_minwidth_val;
>> -    char *adaptionset_maxwidth_val;
>> -    char *adaptionset_minheight_val;
>> -    char *adaptionset_maxheight_val;
>> -    char *adaptionset_minframerate_val;
>> -    char *adaptionset_maxframerate_val;
>> -    char *adaptionset_segmentalignment_val;
>> -    char *adaptionset_bitstreamswitching_val;
>> +    char *adaptionset_lang;
>> +    uint64_t adaptionset_minbw;
>> +    uint64_t adaptionset_maxbw;
>> +    uint64_t adaptionset_minwidth;
>> +    uint64_t adaptionset_maxwidth;
>> +    uint64_t adaptionset_minheight;
>> +    uint64_t adaptionset_maxheight;
>> +    AVRational adaptionset_minframerate;
>> +    AVRational adaptionset_maxframerate;
>> 
>>     int n_videos;
>>     struct representation **videos;
>> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>> 
>> } DASHContext;
>> 
> 
>> +static int get_ratio_from_string(AVRational *dst, char *src)
> 
> const char *
Ok, will modify it.
> 
>> +{
> 
>> +    char *p = src;
> 
> Not useful.
> 
>> +    char *end = NULL;
>> +    char *end_ptr = NULL;
>> +    int num, den;
>> +
> 
>> +    num = (int)strtol(p, &end_ptr, 10);
> 
> Possible integer overflow.
Ok, will modify it . I should add range of the value check. 
> 
>> +    if (errno == ERANGE || end_ptr == src) {
>> +        return AVERROR_INVALIDDATA;
>> +    }
> 
> Unnecessary braces.
Ok, will modify it.
> 
>> +
>> +    if (end_ptr[0] == '\0') {
>> +        dst->den = 1;
>> +        dst->num = num;
>> +        return 0;
>> +    }
>> +
>> +    if (end_ptr[0] != '/')
>> +        return AVERROR_INVALIDDATA;
> 
>> +    p = end_ptr + 1;
> 
> Not useful.
Do you mean I should use end_ptr++ ?
> 
>> +    den = (int)strtol(p, &end, 10);
> 
> Possible integer overflow.
Ok, will modify it . I should add range of the value check. 

> 
>> +    if (errno == ERANGE || end == src) {
> 
>> +        dst->den = 1;
> 
> Why?
For example:
32000/goodmorning
This is wrong framerate, and the Denominator is init to 0, so set it 1 here, because n/0 is incorrect.
> 
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +    dst->den = den;
>> +
>> +    return 0;
>> +}
>> +
> 
>> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
> 
> The return type does not work.
Ok, will modify it.
> 
>> +{
>> +    char *end_ptr = NULL;
>> +    char *val = xmlGetProp(node, key);
>> +    int ret = 0;
>> +
>> +    if (val) {
> 
>> +        ret = strtoull(val, &end_ptr, 10);
> 
> Can't store unsigned long long into int.
Ok, will modify it.
> 
>> +        if (errno == ERANGE) {
>> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
>> +            xmlFree(val);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        if (end_ptr == val || end_ptr[0] != '\0') {
>> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
>> +            "not a valid number: %s\n", key, val);
>> +            xmlFree(val);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        *dst = ret;
>> +        xmlFree(val);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
>> +{
>> +    char *val = xmlGetProp(node, key);
>> +    int ret = 0;
>> +    AVRational rate;
>> +
>> +    if (val) {
>> +        ret = get_ratio_from_string(&rate, val);
>> +        if (ret < 0)
>> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
>> +        xmlFree(val);
>> +    }
>> +    member->num = rate.num;
>> +    member->den = rate.den;
>> +    return ret;
>> +}
>> +
>> static int ishttp(char *url)
>> {
>>     const char *proto_name = avio_find_protocol_name(url);
>> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>             ret = AVERROR(ENOMEM);
>>             goto end;
>>         }
>> +        if (c->adaptionset_lang) {
>> +            rep->lang = av_strdup(c->adaptionset_lang);
>> +            if (!rep->lang) {
>> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>> +                av_freep(&rep);
>> +                ret = AVERROR(ENOMEM);
>> +                goto end;
>> +            }
>> +        }
>>         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");
>> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>         }
>> 
>>         if (rep) {
>> +            uint64_t width = 0;
>> +            uint64_t height = 0;
>> +
>>             if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>>                 rep->fragment_timescale = 1;
>>             rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
> 
>> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
>> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
>> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
>> +            if (ret < 0)
>> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
>> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
>> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
>> +                "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
>> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
>> +            if (ret < 0)
>> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
>> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
>> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
>> +                "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
> 
> Looks unrelated.
if (max or min) Width and the (max or min) Height is present in the Adaptationset, and the width or height is not in the range of the (max or min)Width or Height, the stream will be ignored.
> 
>>             strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>>             rep->framerate = av_make_q(0, 0);
>>             if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
>> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
>> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>>                 if (ret < 0)
>> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>> +                if (c->adaptionset_maxframerate.num > 0) {
>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                        free_representation(rep);
>> +                        goto end;
>> +                    }
>> +                }
>> +                if (c->adaptionset_minframerate.num > 0) {
>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                        free_representation(rep);
>> +                        goto end;
>> +                    }
>> +                }
> 
> This is changing the behavior. I don't know if it is in the desirable
> direction.
This workflow will ignore the wrong framerate stream in the mpd, because the frame is higher than the maxframerate or lower than the minframerate.
> 
>>             }
>> 
>>             switch (type) {
>> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>     subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>> 
>> end:
>> +
>>     if (rep_id_val)
>>         xmlFree(rep_id_val);
>>     if (rep_bandwidth_val)
>> @@ -1116,6 +1242,62 @@ end:
>>     return ret;
>> }
>> 
>> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
>> +{
>> +    int ret = 0;
>> +    DASHContext *c = s->priv_data;
>> +
>> +    if (!adaptionset_node) {
>> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +    c->adaptionset_minframerate = av_make_q(0, 0);
>> +    c->adaptionset_maxframerate = av_make_q(0, 0);
>> +    c->adaptionset_maxbw = UINT64_MAX;
>> +    c->adaptionset_minbw = 0;
>> +    c->adaptionset_minwidth = 0;
>> +    c->adaptionset_maxwidth = UINT64_MAX;
>> +    c->adaptionset_minheight = 0;
>> +    c->adaptionset_maxheight = UINT64_MAX;
>> +
>> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
>> +
> 
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
>> +    if (ret  < 0)
>> +        c->adaptionset_minbw = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxbw = UINT64_MAX;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
>> +    if (ret < 0)
>> +        c->adaptionset_minwidth = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxwidth = UINT64_MAX;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
>> +    if (ret < 0)
>> +        c->adaptionset_minheight = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxheight = UINT64_MAX;
>> +
>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
>> +    if (ret < 0) {
>> +        c->adaptionset_minframerate = av_make_q(0, 0);
>> +    }
>> +
>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
>> +    if (ret < 0)
>> +        c->adaptionset_maxframerate = av_make_q(0, 0);
> 
> If you don't actually use the return code, then have the function use a
> default value directly, that will make the code more compact.
for example,
if (dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate”) < 0)
     c->adaptionset_maxframerate = av_make_q(0, 0);

Do you mean do it like this way?

> 
>> +
>> +    return ret;
>> +}
>> +
>> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>                                         xmlNodePtr adaptionset_node,
>>                                         xmlNodePtr mpd_baseurl_node,
>> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>     xmlNodePtr adaptionset_segmentlist_node = NULL;
>>     xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>>     xmlNodePtr node = NULL;
>> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
>> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
>> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
>> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
>> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
>> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
>> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
>> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
>> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
>> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
>> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
>> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
>> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
>> +
>> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
>> +    if (ret < 0)
>> +        return ret;
>> 
>>     node = xmlFirstElementChild(adaptionset_node);
>>     while (node) {
>> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>                                                 adaptionset_segmentlist_node,
>>                                                 adaptionset_supplementalproperty_node);
>>             if (ret < 0) {
>> -                return ret;
>> +                goto end;
>>             }
>>         }
>>         node = xmlNextElementSibling(node);
>>     }
>> -    return 0;
>> +
>> +end:
>> +    av_freep(&c->adaptionset_lang);
>> +    return ret;
>> }
>> 
>> static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
>> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
>> +                av_freep(&rep->lang);
>> +            }
>>         }
>>         for (i = 0; i < c->n_subtitles; i++) {
>>             rep = c->subtitles[i];
>> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>>             rep->assoc_stream = s->streams[rep->stream_index];
>>             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, "lang", rep->lang, 0);
>> +                av_freep(&rep->lang);
>> +            }
>>         }
>>     }
>> 
> 

Thanks

Steven Liu
Andreas Rheinhardt March 28, 2020, 1:37 a.m. UTC | #3 
Steven Liu:
> These member will be used for get more correct information of the MPD
> 
> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> Suggested-by: Nicolas George <george@nsup.org>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 214 insertions(+), 30 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 5bbe5d3985..b5bb8e674c 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -108,6 +108,7 @@ struct representation {
>      int64_t cur_seg_offset;
>      int64_t cur_seg_size;
>      struct fragment *cur_seg;
> +    char *lang;
>  
>      /* Currently active Media Initialization Section */
>      struct fragment *init_section;
> @@ -122,19 +123,15 @@ struct representation {
>  typedef struct DASHContext {
>      const AVClass *class;
>      char *base_url;
> -    char *adaptionset_contenttype_val;
> -    char *adaptionset_par_val;
> -    char *adaptionset_lang_val;
> -    char *adaptionset_minbw_val;
> -    char *adaptionset_maxbw_val;
> -    char *adaptionset_minwidth_val;
> -    char *adaptionset_maxwidth_val;
> -    char *adaptionset_minheight_val;
> -    char *adaptionset_maxheight_val;
> -    char *adaptionset_minframerate_val;
> -    char *adaptionset_maxframerate_val;
> -    char *adaptionset_segmentalignment_val;
> -    char *adaptionset_bitstreamswitching_val;
> +    char *adaptionset_lang;
> +    uint64_t adaptionset_minbw;
> +    uint64_t adaptionset_maxbw;
> +    uint64_t adaptionset_minwidth;
> +    uint64_t adaptionset_maxwidth;
> +    uint64_t adaptionset_minheight;
> +    uint64_t adaptionset_maxheight;
> +    AVRational adaptionset_minframerate;
> +    AVRational adaptionset_maxframerate;
>  
>      int n_videos;
>      struct representation **videos;
> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>  
>  } DASHContext;
>  
> +static int get_ratio_from_string(AVRational *dst, char *src)
> +{
> +    char *p = src;
> +    char *end = NULL;
> +    char *end_ptr = NULL;
> +    int num, den;
> +
> +    num = (int)strtol(p, &end_ptr, 10);
> +    if (errno == ERANGE || end_ptr == src) {
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (end_ptr[0] == '\0') {
> +        dst->den = 1;
> +        dst->num = num;
> +        return 0;
> +    }
> +
> +    if (end_ptr[0] != '/')
> +        return AVERROR_INVALIDDATA;
> +    p = end_ptr + 1;
> +    den = (int)strtol(p, &end, 10);
> +    if (errno == ERANGE || end == src) {
> +        dst->den = 1;
> +        return AVERROR_INVALIDDATA;
> +    }
> +    dst->den = den;
> +
> +    return 0;
> +}
> +
> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
> +{
> +    char *end_ptr = NULL;
> +    char *val = xmlGetProp(node, key);
> +    int ret = 0;
> +
> +    if (val) {
> +        ret = strtoull(val, &end_ptr, 10);
> +        if (errno == ERANGE) {
> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
> +            xmlFree(val);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        if (end_ptr == val || end_ptr[0] != '\0') {
> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
> +            "not a valid number: %s\n", key, val);
> +            xmlFree(val);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        *dst = ret;
> +        xmlFree(val);
> +    }
> +    return ret;
> +}
> +
> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
> +{
> +    char *val = xmlGetProp(node, key);
> +    int ret = 0;
> +    AVRational rate;
> +
> +    if (val) {
> +        ret = get_ratio_from_string(&rate, val);
> +        if (ret < 0)
> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
> +        xmlFree(val);
> +    }
> +    member->num = rate.num;
> +    member->den = rate.den;
> +    return ret;
> +}
> +
>  static int ishttp(char *url)
>  {
>      const char *proto_name = avio_find_protocol_name(url);
> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>              ret = AVERROR(ENOMEM);
>              goto end;
>          }
> +        if (c->adaptionset_lang) {
> +            rep->lang = av_strdup(c->adaptionset_lang);
> +            if (!rep->lang) {
> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
> +                av_freep(&rep);
> +                ret = AVERROR(ENOMEM);
> +                goto end;
> +            }
> +        }
>          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");
> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>          }
>  
>          if (rep) {
> +            uint64_t width = 0;
> +            uint64_t height = 0;
> +
>              if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>                  rep->fragment_timescale = 1;
>              rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +
> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
> +            if (ret < 0)
> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
> +                "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +
> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
> +            if (ret < 0)
> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
> +                "will ignore this representation.\n", rep_id_val);
> +                free_representation(rep);
> +                goto end;
> +            }
> +
>              strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>              rep->framerate = av_make_q(0, 0);
>              if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>                  if (ret < 0)
> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
> +                if (c->adaptionset_maxframerate.num > 0) {
> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                        free_representation(rep);
> +                        goto end;
> +                    }
> +                }
> +                if (c->adaptionset_minframerate.num > 0) {
> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
> +                        "will ignore this representation.\n", rep_id_val);
> +                        free_representation(rep);
> +                        goto end;
> +                    }
> +                }
>              }
>  
>              switch (type) {
> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>      subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>  
>  end:
> +
>      if (rep_id_val)
>          xmlFree(rep_id_val);
>      if (rep_bandwidth_val)
> @@ -1116,6 +1242,62 @@ end:
>      return ret;
>  }
>  
> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
> +{
> +    int ret = 0;
> +    DASHContext *c = s->priv_data;
> +
> +    if (!adaptionset_node) {
> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
> +        return AVERROR(EINVAL);
> +    }
> +    c->adaptionset_minframerate = av_make_q(0, 0);
> +    c->adaptionset_maxframerate = av_make_q(0, 0);
> +    c->adaptionset_maxbw = UINT64_MAX;
> +    c->adaptionset_minbw = 0;
> +    c->adaptionset_minwidth = 0;
> +    c->adaptionset_maxwidth = UINT64_MAX;
> +    c->adaptionset_minheight = 0;
> +    c->adaptionset_maxheight = UINT64_MAX;
> +
> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
> +    if (ret  < 0)
> +        c->adaptionset_minbw = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
> +    if (ret <= 0)
> +        c->adaptionset_maxbw = UINT64_MAX;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
> +    if (ret < 0)
> +        c->adaptionset_minwidth = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
> +    if (ret <= 0)
> +        c->adaptionset_maxwidth = UINT64_MAX;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
> +    if (ret < 0)
> +        c->adaptionset_minheight = 0;
> +
> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
> +    if (ret <= 0)
> +        c->adaptionset_maxheight = UINT64_MAX;
> +
> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
> +    if (ret < 0) {
> +        c->adaptionset_minframerate = av_make_q(0, 0);
> +    }
> +
> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
> +    if (ret < 0)
> +        c->adaptionset_maxframerate = av_make_q(0, 0);
> +
> +    return ret;
> +}
> +
>  static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>                                          xmlNodePtr adaptionset_node,
>                                          xmlNodePtr mpd_baseurl_node,
> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>      xmlNodePtr adaptionset_segmentlist_node = NULL;
>      xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>      xmlNodePtr node = NULL;
> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
> +
> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
> +    if (ret < 0)
> +        return ret;
>  
>      node = xmlFirstElementChild(adaptionset_node);
>      while (node) {
> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>                                                  adaptionset_segmentlist_node,
>                                                  adaptionset_supplementalproperty_node);
>              if (ret < 0) {
> -                return ret;
> +                goto end;
>              }
>          }
>          node = xmlNextElementSibling(node);
>      }
> -    return 0;
> +
> +end:
> +    av_freep(&c->adaptionset_lang);
> +    return ret;
>  }
>  
>  static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
> +                av_freep(&rep->lang);
> +            }
>          }
>          for (i = 0; i < c->n_subtitles; i++) {
>              rep = c->subtitles[i];
> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>              rep->assoc_stream = s->streams[rep->stream_index];
>              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, "lang", rep->lang, 0);
> +                av_freep(&rep->lang);
> +            }
>          }
>      }
>  
> 
The commit title says that this commit is about fixing a memleak (it btw
has one "commit" too much in it), whereas most of this patch is about
adding new functionality. Which makes me wonder how you intend to fix
this in the old releases? Backport everything?
(This problem would of course not exist if the new functionality would
be split from fixing the memleak.)

- Andreas
Liu Steven March 28, 2020, 1:47 a.m. UTC | #4 
> 2020年3月28日 上午9:37，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
> 
> Steven Liu:
>> These member will be used for get more correct information of the MPD
>> 
>> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> Suggested-by: Nicolas George <george@nsup.org>
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>> libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 214 insertions(+), 30 deletions(-)
>> 
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index 5bbe5d3985..b5bb8e674c 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -108,6 +108,7 @@ struct representation {
>>     int64_t cur_seg_offset;
>>     int64_t cur_seg_size;
>>     struct fragment *cur_seg;
>> +    char *lang;
>> 
>>     /* Currently active Media Initialization Section */
>>     struct fragment *init_section;
>> @@ -122,19 +123,15 @@ struct representation {
>> typedef struct DASHContext {
>>     const AVClass *class;
>>     char *base_url;
>> -    char *adaptionset_contenttype_val;
>> -    char *adaptionset_par_val;
>> -    char *adaptionset_lang_val;
>> -    char *adaptionset_minbw_val;
>> -    char *adaptionset_maxbw_val;
>> -    char *adaptionset_minwidth_val;
>> -    char *adaptionset_maxwidth_val;
>> -    char *adaptionset_minheight_val;
>> -    char *adaptionset_maxheight_val;
>> -    char *adaptionset_minframerate_val;
>> -    char *adaptionset_maxframerate_val;
>> -    char *adaptionset_segmentalignment_val;
>> -    char *adaptionset_bitstreamswitching_val;
>> +    char *adaptionset_lang;
>> +    uint64_t adaptionset_minbw;
>> +    uint64_t adaptionset_maxbw;
>> +    uint64_t adaptionset_minwidth;
>> +    uint64_t adaptionset_maxwidth;
>> +    uint64_t adaptionset_minheight;
>> +    uint64_t adaptionset_maxheight;
>> +    AVRational adaptionset_minframerate;
>> +    AVRational adaptionset_maxframerate;
>> 
>>     int n_videos;
>>     struct representation **videos;
>> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>> 
>> } DASHContext;
>> 
>> +static int get_ratio_from_string(AVRational *dst, char *src)
>> +{
>> +    char *p = src;
>> +    char *end = NULL;
>> +    char *end_ptr = NULL;
>> +    int num, den;
>> +
>> +    num = (int)strtol(p, &end_ptr, 10);
>> +    if (errno == ERANGE || end_ptr == src) {
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    if (end_ptr[0] == '\0') {
>> +        dst->den = 1;
>> +        dst->num = num;
>> +        return 0;
>> +    }
>> +
>> +    if (end_ptr[0] != '/')
>> +        return AVERROR_INVALIDDATA;
>> +    p = end_ptr + 1;
>> +    den = (int)strtol(p, &end, 10);
>> +    if (errno == ERANGE || end == src) {
>> +        dst->den = 1;
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +    dst->den = den;
>> +
>> +    return 0;
>> +}
>> +
>> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
>> +{
>> +    char *end_ptr = NULL;
>> +    char *val = xmlGetProp(node, key);
>> +    int ret = 0;
>> +
>> +    if (val) {
>> +        ret = strtoull(val, &end_ptr, 10);
>> +        if (errno == ERANGE) {
>> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
>> +            xmlFree(val);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        if (end_ptr == val || end_ptr[0] != '\0') {
>> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
>> +            "not a valid number: %s\n", key, val);
>> +            xmlFree(val);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        *dst = ret;
>> +        xmlFree(val);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
>> +{
>> +    char *val = xmlGetProp(node, key);
>> +    int ret = 0;
>> +    AVRational rate;
>> +
>> +    if (val) {
>> +        ret = get_ratio_from_string(&rate, val);
>> +        if (ret < 0)
>> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
>> +        xmlFree(val);
>> +    }
>> +    member->num = rate.num;
>> +    member->den = rate.den;
>> +    return ret;
>> +}
>> +
>> static int ishttp(char *url)
>> {
>>     const char *proto_name = avio_find_protocol_name(url);
>> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>             ret = AVERROR(ENOMEM);
>>             goto end;
>>         }
>> +        if (c->adaptionset_lang) {
>> +            rep->lang = av_strdup(c->adaptionset_lang);
>> +            if (!rep->lang) {
>> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>> +                av_freep(&rep);
>> +                ret = AVERROR(ENOMEM);
>> +                goto end;
>> +            }
>> +        }
>>         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");
>> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>         }
>> 
>>         if (rep) {
>> +            uint64_t width = 0;
>> +            uint64_t height = 0;
>> +
>>             if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>>                 rep->fragment_timescale = 1;
>>             rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
>> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
>> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
>> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
>> +            if (ret < 0)
>> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
>> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
>> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
>> +                "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
>> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
>> +            if (ret < 0)
>> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
>> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
>> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
>> +                "will ignore this representation.\n", rep_id_val);
>> +                free_representation(rep);
>> +                goto end;
>> +            }
>> +
>>             strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>>             rep->framerate = av_make_q(0, 0);
>>             if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
>> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
>> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>>                 if (ret < 0)
>> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>> +                if (c->adaptionset_maxframerate.num > 0) {
>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                        free_representation(rep);
>> +                        goto end;
>> +                    }
>> +                }
>> +                if (c->adaptionset_minframerate.num > 0) {
>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>> +                        "will ignore this representation.\n", rep_id_val);
>> +                        free_representation(rep);
>> +                        goto end;
>> +                    }
>> +                }
>>             }
>> 
>>             switch (type) {
>> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>     subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>> 
>> end:
>> +
>>     if (rep_id_val)
>>         xmlFree(rep_id_val);
>>     if (rep_bandwidth_val)
>> @@ -1116,6 +1242,62 @@ end:
>>     return ret;
>> }
>> 
>> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
>> +{
>> +    int ret = 0;
>> +    DASHContext *c = s->priv_data;
>> +
>> +    if (!adaptionset_node) {
>> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +    c->adaptionset_minframerate = av_make_q(0, 0);
>> +    c->adaptionset_maxframerate = av_make_q(0, 0);
>> +    c->adaptionset_maxbw = UINT64_MAX;
>> +    c->adaptionset_minbw = 0;
>> +    c->adaptionset_minwidth = 0;
>> +    c->adaptionset_maxwidth = UINT64_MAX;
>> +    c->adaptionset_minheight = 0;
>> +    c->adaptionset_maxheight = UINT64_MAX;
>> +
>> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
>> +    if (ret  < 0)
>> +        c->adaptionset_minbw = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxbw = UINT64_MAX;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
>> +    if (ret < 0)
>> +        c->adaptionset_minwidth = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxwidth = UINT64_MAX;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
>> +    if (ret < 0)
>> +        c->adaptionset_minheight = 0;
>> +
>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
>> +    if (ret <= 0)
>> +        c->adaptionset_maxheight = UINT64_MAX;
>> +
>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
>> +    if (ret < 0) {
>> +        c->adaptionset_minframerate = av_make_q(0, 0);
>> +    }
>> +
>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
>> +    if (ret < 0)
>> +        c->adaptionset_maxframerate = av_make_q(0, 0);
>> +
>> +    return ret;
>> +}
>> +
>> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>                                         xmlNodePtr adaptionset_node,
>>                                         xmlNodePtr mpd_baseurl_node,
>> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>     xmlNodePtr adaptionset_segmentlist_node = NULL;
>>     xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>>     xmlNodePtr node = NULL;
>> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
>> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
>> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
>> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
>> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
>> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
>> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
>> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
>> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
>> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
>> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
>> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
>> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
>> +
>> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
>> +    if (ret < 0)
>> +        return ret;
>> 
>>     node = xmlFirstElementChild(adaptionset_node);
>>     while (node) {
>> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>                                                 adaptionset_segmentlist_node,
>>                                                 adaptionset_supplementalproperty_node);
>>             if (ret < 0) {
>> -                return ret;
>> +                goto end;
>>             }
>>         }
>>         node = xmlNextElementSibling(node);
>>     }
>> -    return 0;
>> +
>> +end:
>> +    av_freep(&c->adaptionset_lang);
>> +    return ret;
>> }
>> 
>> static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
>> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
>> +                av_freep(&rep->lang);
>> +            }
>>         }
>>         for (i = 0; i < c->n_subtitles; i++) {
>>             rep = c->subtitles[i];
>> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>>             rep->assoc_stream = s->streams[rep->stream_index];
>>             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, "lang", rep->lang, 0);
>> +                av_freep(&rep->lang);
>> +            }
>>         }
>>     }
>> 
>> 
> The commit title says that this commit is about fixing a memleak (it btw
> has one "commit" too much in it), whereas most of this patch is about
> adding new functionality. Which makes me wonder how you intend to fix
> this in the old releases? Backport everything?
> (This problem would of course not exist if the new functionality would
> be split from fixing the memleak.)
What about modify to: avformat/dashdec: refine functionally for commit e134c203
> 
> - Andreas
> _______________________________________________
> 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".

Thanks

Steven Liu
Andreas Rheinhardt March 28, 2020, 2:15 a.m. UTC | #5 
Steven Liu:
> 
> 
>> 2020年3月28日 上午9:37，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
>>
>> Steven Liu:
>>> These member will be used for get more correct information of the MPD
>>>
>>> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> Suggested-by: Nicolas George <george@nsup.org>
>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>> ---
>>> libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 214 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>>> index 5bbe5d3985..b5bb8e674c 100644
>>> --- a/libavformat/dashdec.c
>>> +++ b/libavformat/dashdec.c
>>> @@ -108,6 +108,7 @@ struct representation {
>>>     int64_t cur_seg_offset;
>>>     int64_t cur_seg_size;
>>>     struct fragment *cur_seg;
>>> +    char *lang;
>>>
>>>     /* Currently active Media Initialization Section */
>>>     struct fragment *init_section;
>>> @@ -122,19 +123,15 @@ struct representation {
>>> typedef struct DASHContext {
>>>     const AVClass *class;
>>>     char *base_url;
>>> -    char *adaptionset_contenttype_val;
>>> -    char *adaptionset_par_val;
>>> -    char *adaptionset_lang_val;
>>> -    char *adaptionset_minbw_val;
>>> -    char *adaptionset_maxbw_val;
>>> -    char *adaptionset_minwidth_val;
>>> -    char *adaptionset_maxwidth_val;
>>> -    char *adaptionset_minheight_val;
>>> -    char *adaptionset_maxheight_val;
>>> -    char *adaptionset_minframerate_val;
>>> -    char *adaptionset_maxframerate_val;
>>> -    char *adaptionset_segmentalignment_val;
>>> -    char *adaptionset_bitstreamswitching_val;
>>> +    char *adaptionset_lang;
>>> +    uint64_t adaptionset_minbw;
>>> +    uint64_t adaptionset_maxbw;
>>> +    uint64_t adaptionset_minwidth;
>>> +    uint64_t adaptionset_maxwidth;
>>> +    uint64_t adaptionset_minheight;
>>> +    uint64_t adaptionset_maxheight;
>>> +    AVRational adaptionset_minframerate;
>>> +    AVRational adaptionset_maxframerate;
>>>
>>>     int n_videos;
>>>     struct representation **videos;
>>> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>>>
>>> } DASHContext;
>>>
>>> +static int get_ratio_from_string(AVRational *dst, char *src)
>>> +{
>>> +    char *p = src;
>>> +    char *end = NULL;
>>> +    char *end_ptr = NULL;
>>> +    int num, den;
>>> +
>>> +    num = (int)strtol(p, &end_ptr, 10);
>>> +    if (errno == ERANGE || end_ptr == src) {
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>> +    if (end_ptr[0] == '\0') {
>>> +        dst->den = 1;
>>> +        dst->num = num;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (end_ptr[0] != '/')
>>> +        return AVERROR_INVALIDDATA;
>>> +    p = end_ptr + 1;
>>> +    den = (int)strtol(p, &end, 10);
>>> +    if (errno == ERANGE || end == src) {
>>> +        dst->den = 1;
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +    dst->den = den;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
>>> +{
>>> +    char *end_ptr = NULL;
>>> +    char *val = xmlGetProp(node, key);
>>> +    int ret = 0;
>>> +
>>> +    if (val) {
>>> +        ret = strtoull(val, &end_ptr, 10);
>>> +        if (errno == ERANGE) {
>>> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
>>> +            xmlFree(val);
>>> +            return AVERROR_INVALIDDATA;
>>> +        }
>>> +        if (end_ptr == val || end_ptr[0] != '\0') {
>>> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
>>> +            "not a valid number: %s\n", key, val);
>>> +            xmlFree(val);
>>> +            return AVERROR_INVALIDDATA;
>>> +        }
>>> +        *dst = ret;
>>> +        xmlFree(val);
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
>>> +{
>>> +    char *val = xmlGetProp(node, key);
>>> +    int ret = 0;
>>> +    AVRational rate;
>>> +
>>> +    if (val) {
>>> +        ret = get_ratio_from_string(&rate, val);
>>> +        if (ret < 0)
>>> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
>>> +        xmlFree(val);
>>> +    }
>>> +    member->num = rate.num;
>>> +    member->den = rate.den;
>>> +    return ret;
>>> +}
>>> +
>>> static int ishttp(char *url)
>>> {
>>>     const char *proto_name = avio_find_protocol_name(url);
>>> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>             ret = AVERROR(ENOMEM);
>>>             goto end;
>>>         }
>>> +        if (c->adaptionset_lang) {
>>> +            rep->lang = av_strdup(c->adaptionset_lang);
>>> +            if (!rep->lang) {
>>> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>>> +                av_freep(&rep);
>>> +                ret = AVERROR(ENOMEM);
>>> +                goto end;
>>> +            }
>>> +        }
>>>         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");
>>> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>         }
>>>
>>>         if (rep) {
>>> +            uint64_t width = 0;
>>> +            uint64_t height = 0;
>>> +
>>>             if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>>>                 rep->fragment_timescale = 1;
>>>             rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
>>> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
>>> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
>>> +                        "will ignore this representation.\n", rep_id_val);
>>> +                free_representation(rep);
>>> +                goto end;
>>> +            }
>>> +
>>> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
>>> +            if (ret < 0)
>>> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
>>> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
>>> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
>>> +                "will ignore this representation.\n", rep_id_val);
>>> +                free_representation(rep);
>>> +                goto end;
>>> +            }
>>> +
>>> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
>>> +            if (ret < 0)
>>> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
>>> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
>>> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
>>> +                "will ignore this representation.\n", rep_id_val);
>>> +                free_representation(rep);
>>> +                goto end;
>>> +            }
>>> +
>>>             strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>>>             rep->framerate = av_make_q(0, 0);
>>>             if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
>>> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
>>> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>>>                 if (ret < 0)
>>> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>>> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>>> +                if (c->adaptionset_maxframerate.num > 0) {
>>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
>>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>>> +                        "will ignore this representation.\n", rep_id_val);
>>> +                        free_representation(rep);
>>> +                        goto end;
>>> +                    }
>>> +                }
>>> +                if (c->adaptionset_minframerate.num > 0) {
>>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
>>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>>> +                        "will ignore this representation.\n", rep_id_val);
>>> +                        free_representation(rep);
>>> +                        goto end;
>>> +                    }
>>> +                }
>>>             }
>>>
>>>             switch (type) {
>>> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>     subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>>>
>>> end:
>>> +
>>>     if (rep_id_val)
>>>         xmlFree(rep_id_val);
>>>     if (rep_bandwidth_val)
>>> @@ -1116,6 +1242,62 @@ end:
>>>     return ret;
>>> }
>>>
>>> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
>>> +{
>>> +    int ret = 0;
>>> +    DASHContext *c = s->priv_data;
>>> +
>>> +    if (!adaptionset_node) {
>>> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
>>> +        return AVERROR(EINVAL);
>>> +    }
>>> +    c->adaptionset_minframerate = av_make_q(0, 0);
>>> +    c->adaptionset_maxframerate = av_make_q(0, 0);
>>> +    c->adaptionset_maxbw = UINT64_MAX;
>>> +    c->adaptionset_minbw = 0;
>>> +    c->adaptionset_minwidth = 0;
>>> +    c->adaptionset_maxwidth = UINT64_MAX;
>>> +    c->adaptionset_minheight = 0;
>>> +    c->adaptionset_maxheight = UINT64_MAX;
>>> +
>>> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
>>> +    if (ret  < 0)
>>> +        c->adaptionset_minbw = 0;
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
>>> +    if (ret <= 0)
>>> +        c->adaptionset_maxbw = UINT64_MAX;
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
>>> +    if (ret < 0)
>>> +        c->adaptionset_minwidth = 0;
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
>>> +    if (ret <= 0)
>>> +        c->adaptionset_maxwidth = UINT64_MAX;
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
>>> +    if (ret < 0)
>>> +        c->adaptionset_minheight = 0;
>>> +
>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
>>> +    if (ret <= 0)
>>> +        c->adaptionset_maxheight = UINT64_MAX;
>>> +
>>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
>>> +    if (ret < 0) {
>>> +        c->adaptionset_minframerate = av_make_q(0, 0);
>>> +    }
>>> +
>>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
>>> +    if (ret < 0)
>>> +        c->adaptionset_maxframerate = av_make_q(0, 0);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>                                         xmlNodePtr adaptionset_node,
>>>                                         xmlNodePtr mpd_baseurl_node,
>>> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>     xmlNodePtr adaptionset_segmentlist_node = NULL;
>>>     xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>>>     xmlNodePtr node = NULL;
>>> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
>>> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
>>> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
>>> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
>>> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
>>> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
>>> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
>>> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
>>> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
>>> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
>>> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
>>> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
>>> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
>>> +
>>> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
>>> +    if (ret < 0)
>>> +        return ret;
>>>
>>>     node = xmlFirstElementChild(adaptionset_node);
>>>     while (node) {
>>> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>                                                 adaptionset_segmentlist_node,
>>>                                                 adaptionset_supplementalproperty_node);
>>>             if (ret < 0) {
>>> -                return ret;
>>> +                goto end;
>>>             }
>>>         }
>>>         node = xmlNextElementSibling(node);
>>>     }
>>> -    return 0;
>>> +
>>> +end:
>>> +    av_freep(&c->adaptionset_lang);
>>> +    return ret;
>>> }
>>>
>>> static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
>>> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
>>> +                av_freep(&rep->lang);
>>> +            }
>>>         }
>>>         for (i = 0; i < c->n_subtitles; i++) {
>>>             rep = c->subtitles[i];
>>> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>>>             rep->assoc_stream = s->streams[rep->stream_index];
>>>             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, "lang", rep->lang, 0);
>>> +                av_freep(&rep->lang);
>>> +            }
>>>         }
>>>     }
>>>
>>>
>> The commit title says that this commit is about fixing a memleak (it btw
>> has one "commit" too much in it), whereas most of this patch is about
>> adding new functionality. Which makes me wonder how you intend to fix
>> this in the old releases? Backport everything?
>> (This problem would of course not exist if the new functionality would
>> be split from fixing the memleak.)
> What about modify to: avformat/dashdec: refine functionally for commit e134c203

This will not make it apparent from the commit title that it fixes a
memleak; and the reference to the old commit seems wrong, too: You
actually do not even build your new functionality on top of e134c203,
you are essentially reverting said commit in this commit and start anew.
Also e134c203 didn't add any functionality to refine.

And changing the title won't solve the backporting issue, of course.

- Andreas
Liu Steven March 28, 2020, 3:45 a.m. UTC | #6 
> 2020年3月28日 上午10:15，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
> 
> Steven Liu:
>> 
>> 
>>> 2020年3月28日 上午9:37，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
>>> 
>>> Steven Liu:
>>>> These member will be used for get more correct information of the MPD
>>>> 
>>>> Suggested-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> Suggested-by: Nicolas George <george@nsup.org>
>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>>> ---
>>>> libavformat/dashdec.c | 244 ++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 214 insertions(+), 30 deletions(-)
>>>> 
>>>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>>>> index 5bbe5d3985..b5bb8e674c 100644
>>>> --- a/libavformat/dashdec.c
>>>> +++ b/libavformat/dashdec.c
>>>> @@ -108,6 +108,7 @@ struct representation {
>>>>    int64_t cur_seg_offset;
>>>>    int64_t cur_seg_size;
>>>>    struct fragment *cur_seg;
>>>> +    char *lang;
>>>> 
>>>>    /* Currently active Media Initialization Section */
>>>>    struct fragment *init_section;
>>>> @@ -122,19 +123,15 @@ struct representation {
>>>> typedef struct DASHContext {
>>>>    const AVClass *class;
>>>>    char *base_url;
>>>> -    char *adaptionset_contenttype_val;
>>>> -    char *adaptionset_par_val;
>>>> -    char *adaptionset_lang_val;
>>>> -    char *adaptionset_minbw_val;
>>>> -    char *adaptionset_maxbw_val;
>>>> -    char *adaptionset_minwidth_val;
>>>> -    char *adaptionset_maxwidth_val;
>>>> -    char *adaptionset_minheight_val;
>>>> -    char *adaptionset_maxheight_val;
>>>> -    char *adaptionset_minframerate_val;
>>>> -    char *adaptionset_maxframerate_val;
>>>> -    char *adaptionset_segmentalignment_val;
>>>> -    char *adaptionset_bitstreamswitching_val;
>>>> +    char *adaptionset_lang;
>>>> +    uint64_t adaptionset_minbw;
>>>> +    uint64_t adaptionset_maxbw;
>>>> +    uint64_t adaptionset_minwidth;
>>>> +    uint64_t adaptionset_maxwidth;
>>>> +    uint64_t adaptionset_minheight;
>>>> +    uint64_t adaptionset_maxheight;
>>>> +    AVRational adaptionset_minframerate;
>>>> +    AVRational adaptionset_maxframerate;
>>>> 
>>>>    int n_videos;
>>>>    struct representation **videos;
>>>> @@ -169,6 +166,79 @@ typedef struct DASHContext {
>>>> 
>>>> } DASHContext;
>>>> 
>>>> +static int get_ratio_from_string(AVRational *dst, char *src)
>>>> +{
>>>> +    char *p = src;
>>>> +    char *end = NULL;
>>>> +    char *end_ptr = NULL;
>>>> +    int num, den;
>>>> +
>>>> +    num = (int)strtol(p, &end_ptr, 10);
>>>> +    if (errno == ERANGE || end_ptr == src) {
>>>> +        return AVERROR_INVALIDDATA;
>>>> +    }
>>>> +
>>>> +    if (end_ptr[0] == '\0') {
>>>> +        dst->den = 1;
>>>> +        dst->num = num;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (end_ptr[0] != '/')
>>>> +        return AVERROR_INVALIDDATA;
>>>> +    p = end_ptr + 1;
>>>> +    den = (int)strtol(p, &end, 10);
>>>> +    if (errno == ERANGE || end == src) {
>>>> +        dst->den = 1;
>>>> +        return AVERROR_INVALIDDATA;
>>>> +    }
>>>> +    dst->den = den;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
>>>> +{
>>>> +    char *end_ptr = NULL;
>>>> +    char *val = xmlGetProp(node, key);
>>>> +    int ret = 0;
>>>> +
>>>> +    if (val) {
>>>> +        ret = strtoull(val, &end_ptr, 10);
>>>> +        if (errno == ERANGE) {
>>>> +            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
>>>> +            xmlFree(val);
>>>> +            return AVERROR_INVALIDDATA;
>>>> +        }
>>>> +        if (end_ptr == val || end_ptr[0] != '\0') {
>>>> +            av_log(s, AV_LOG_ERROR, "The %s field value is "
>>>> +            "not a valid number: %s\n", key, val);
>>>> +            xmlFree(val);
>>>> +            return AVERROR_INVALIDDATA;
>>>> +        }
>>>> +        *dst = ret;
>>>> +        xmlFree(val);
>>>> +    }
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
>>>> +{
>>>> +    char *val = xmlGetProp(node, key);
>>>> +    int ret = 0;
>>>> +    AVRational rate;
>>>> +
>>>> +    if (val) {
>>>> +        ret = get_ratio_from_string(&rate, val);
>>>> +        if (ret < 0)
>>>> +            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
>>>> +        xmlFree(val);
>>>> +    }
>>>> +    member->num = rate.num;
>>>> +    member->den = rate.den;
>>>> +    return ret;
>>>> +}
>>>> +
>>>> static int ishttp(char *url)
>>>> {
>>>>    const char *proto_name = avio_find_protocol_name(url);
>>>> @@ -885,6 +955,15 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>>            ret = AVERROR(ENOMEM);
>>>>            goto end;
>>>>        }
>>>> +        if (c->adaptionset_lang) {
>>>> +            rep->lang = av_strdup(c->adaptionset_lang);
>>>> +            if (!rep->lang) {
>>>> +                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
>>>> +                av_freep(&rep);
>>>> +                ret = AVERROR(ENOMEM);
>>>> +                goto end;
>>>> +            }
>>>> +        }
>>>>        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");
>>>> @@ -1070,15 +1149,61 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>>        }
>>>> 
>>>>        if (rep) {
>>>> +            uint64_t width = 0;
>>>> +            uint64_t height = 0;
>>>> +
>>>>            if (rep->fragment_duration > 0 && !rep->fragment_timescale)
>>>>                rep->fragment_timescale = 1;
>>>>            rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
>>>> +            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
>>>> +                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
>>>> +                        "will ignore this representation.\n", rep_id_val);
>>>> +                free_representation(rep);
>>>> +                goto end;
>>>> +            }
>>>> +
>>>> +            ret = dash_prop_get_int64(s, representation_node, &width, "width");
>>>> +            if (ret < 0)
>>>> +                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
>>>> +            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
>>>> +                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
>>>> +                "will ignore this representation.\n", rep_id_val);
>>>> +                free_representation(rep);
>>>> +                goto end;
>>>> +            }
>>>> +
>>>> +            ret = dash_prop_get_int64(s, representation_node, &height, "height");
>>>> +            if (ret < 0)
>>>> +                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
>>>> +            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
>>>> +                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
>>>> +                "will ignore this representation.\n", rep_id_val);
>>>> +                free_representation(rep);
>>>> +                goto end;
>>>> +            }
>>>> +
>>>>            strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
>>>>            rep->framerate = av_make_q(0, 0);
>>>>            if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
>>>> -                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
>>>> +                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
>>>>                if (ret < 0)
>>>> -                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>>>> +                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
>>>> +                if (c->adaptionset_maxframerate.num > 0) {
>>>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
>>>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>>>> +                        "will ignore this representation.\n", rep_id_val);
>>>> +                        free_representation(rep);
>>>> +                        goto end;
>>>> +                    }
>>>> +                }
>>>> +                if (c->adaptionset_minframerate.num > 0) {
>>>> +                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
>>>> +                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
>>>> +                        "will ignore this representation.\n", rep_id_val);
>>>> +                        free_representation(rep);
>>>> +                        goto end;
>>>> +                    }
>>>> +                }
>>>>            }
>>>> 
>>>>            switch (type) {
>>>> @@ -1106,6 +1231,7 @@ static int parse_manifest_representation(AVFormatContext *s, const char *url,
>>>>    subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
>>>> 
>>>> end:
>>>> +
>>>>    if (rep_id_val)
>>>>        xmlFree(rep_id_val);
>>>>    if (rep_bandwidth_val)
>>>> @@ -1116,6 +1242,62 @@ end:
>>>>    return ret;
>>>> }
>>>> 
>>>> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
>>>> +{
>>>> +    int ret = 0;
>>>> +    DASHContext *c = s->priv_data;
>>>> +
>>>> +    if (!adaptionset_node) {
>>>> +        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
>>>> +        return AVERROR(EINVAL);
>>>> +    }
>>>> +    c->adaptionset_minframerate = av_make_q(0, 0);
>>>> +    c->adaptionset_maxframerate = av_make_q(0, 0);
>>>> +    c->adaptionset_maxbw = UINT64_MAX;
>>>> +    c->adaptionset_minbw = 0;
>>>> +    c->adaptionset_minwidth = 0;
>>>> +    c->adaptionset_maxwidth = UINT64_MAX;
>>>> +    c->adaptionset_minheight = 0;
>>>> +    c->adaptionset_maxheight = UINT64_MAX;
>>>> +
>>>> +    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
>>>> +    if (ret  < 0)
>>>> +        c->adaptionset_minbw = 0;
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
>>>> +    if (ret <= 0)
>>>> +        c->adaptionset_maxbw = UINT64_MAX;
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
>>>> +    if (ret < 0)
>>>> +        c->adaptionset_minwidth = 0;
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
>>>> +    if (ret <= 0)
>>>> +        c->adaptionset_maxwidth = UINT64_MAX;
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
>>>> +    if (ret < 0)
>>>> +        c->adaptionset_minheight = 0;
>>>> +
>>>> +    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
>>>> +    if (ret <= 0)
>>>> +        c->adaptionset_maxheight = UINT64_MAX;
>>>> +
>>>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
>>>> +    if (ret < 0) {
>>>> +        c->adaptionset_minframerate = av_make_q(0, 0);
>>>> +    }
>>>> +
>>>> +    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
>>>> +    if (ret < 0)
>>>> +        c->adaptionset_maxframerate = av_make_q(0, 0);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>>                                        xmlNodePtr adaptionset_node,
>>>>                                        xmlNodePtr mpd_baseurl_node,
>>>> @@ -1131,19 +1313,10 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>>    xmlNodePtr adaptionset_segmentlist_node = NULL;
>>>>    xmlNodePtr adaptionset_supplementalproperty_node = NULL;
>>>>    xmlNodePtr node = NULL;
>>>> -    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
>>>> -    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
>>>> -    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
>>>> -    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
>>>> -    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
>>>> -    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
>>>> -    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
>>>> -    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
>>>> -    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
>>>> -    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
>>>> -    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
>>>> -    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
>>>> -    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
>>>> +
>>>> +    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> 
>>>>    node = xmlFirstElementChild(adaptionset_node);
>>>>    while (node) {
>>>> @@ -1170,12 +1343,15 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
>>>>                                                adaptionset_segmentlist_node,
>>>>                                                adaptionset_supplementalproperty_node);
>>>>            if (ret < 0) {
>>>> -                return ret;
>>>> +                goto end;
>>>>            }
>>>>        }
>>>>        node = xmlNextElementSibling(node);
>>>>    }
>>>> -    return 0;
>>>> +
>>>> +end:
>>>> +    av_freep(&c->adaptionset_lang);
>>>> +    return ret;
>>>> }
>>>> 
>>>> static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
>>>> @@ -2148,6 +2324,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, "lang", rep->lang, 0);
>>>> +                av_freep(&rep->lang);
>>>> +            }
>>>>        }
>>>>        for (i = 0; i < c->n_subtitles; i++) {
>>>>            rep = c->subtitles[i];
>>>> @@ -2155,6 +2335,10 @@ static int dash_read_header(AVFormatContext *s)
>>>>            rep->assoc_stream = s->streams[rep->stream_index];
>>>>            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, "lang", rep->lang, 0);
>>>> +                av_freep(&rep->lang);
>>>> +            }
>>>>        }
>>>>    }
>>>> 
>>>> 
>>> The commit title says that this commit is about fixing a memleak (it btw
>>> has one "commit" too much in it), whereas most of this patch is about
>>> adding new functionality. Which makes me wonder how you intend to fix
>>> this in the old releases? Backport everything?
>>> (This problem would of course not exist if the new functionality would
>>> be split from fixing the memleak.)
>> What about modify to: avformat/dashdec: refine functionally for commit e134c203
> 
> This will not make it apparent from the commit title that it fixes a
> memleak; and the reference to the old commit seems wrong, too: You
> actually do not even build your new functionality on top of e134c203,
> you are essentially reverting said commit in this commit and start anew.
> Also e134c203 didn't add any functionality to refine.
> 
> And changing the title won't solve the backporting issue, of course.
Ok, will add some comment about this commit,
For example:
	refine adaptationset attribute process function, and fix some memleak of old commit, 
	Whatever, this just don’t want remove these adaptationset attribute by Andreas, 
	because these attribute should be used to check representation usable.

Thanks

Steven Liu
Nicolas George March 28, 2020, 11:54 a.m. UTC | #7 
Andreas Rheinhardt (12020-03-28):
> The commit title says that this commit is about fixing a memleak (it btw

That is my concern too.

> has one "commit" too much in it), whereas most of this patch is about
> adding new functionality. Which makes me wonder how you intend to fix
> this in the old releases? Backport everything?
> (This problem would of course not exist if the new functionality would
> be split from fixing the memleak.)

The leak exists in a release? Then it settles things: revert the
original commit, period.

Then, Steven, you can continue this very patch, you just need to rebase
it on top of the revert, it will actually be simpler.

Regards,
Andreas Rheinhardt March 28, 2020, 11:43 p.m. UTC | #8 
Nicolas George:
> Andreas Rheinhardt (12020-03-28):
>> The commit title says that this commit is about fixing a memleak (it btw
> 
> That is my concern too.
> 
>> has one "commit" too much in it), whereas most of this patch is about
>> adding new functionality. Which makes me wonder how you intend to fix
>> this in the old releases? Backport everything?
>> (This problem would of course not exist if the new functionality would
>> be split from fixing the memleak.)
> 
> The leak exists in a release? 

It is in 4.1 and 4.2.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 5bbe5d3985..b5bb8e674c 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -108,6 +108,7 @@  struct representation {
     int64_t cur_seg_offset;
     int64_t cur_seg_size;
     struct fragment *cur_seg;
+    char *lang;
 
     /* Currently active Media Initialization Section */
     struct fragment *init_section;
@@ -122,19 +123,15 @@  struct representation {
 typedef struct DASHContext {
     const AVClass *class;
     char *base_url;
-    char *adaptionset_contenttype_val;
-    char *adaptionset_par_val;
-    char *adaptionset_lang_val;
-    char *adaptionset_minbw_val;
-    char *adaptionset_maxbw_val;
-    char *adaptionset_minwidth_val;
-    char *adaptionset_maxwidth_val;
-    char *adaptionset_minheight_val;
-    char *adaptionset_maxheight_val;
-    char *adaptionset_minframerate_val;
-    char *adaptionset_maxframerate_val;
-    char *adaptionset_segmentalignment_val;
-    char *adaptionset_bitstreamswitching_val;
+    char *adaptionset_lang;
+    uint64_t adaptionset_minbw;
+    uint64_t adaptionset_maxbw;
+    uint64_t adaptionset_minwidth;
+    uint64_t adaptionset_maxwidth;
+    uint64_t adaptionset_minheight;
+    uint64_t adaptionset_maxheight;
+    AVRational adaptionset_minframerate;
+    AVRational adaptionset_maxframerate;
 
     int n_videos;
     struct representation **videos;
@@ -169,6 +166,79 @@  typedef struct DASHContext {
 
 } DASHContext;
 
+static int get_ratio_from_string(AVRational *dst, char *src)
+{
+    char *p = src;
+    char *end = NULL;
+    char *end_ptr = NULL;
+    int num, den;
+
+    num = (int)strtol(p, &end_ptr, 10);
+    if (errno == ERANGE || end_ptr == src) {
+        return AVERROR_INVALIDDATA;
+    }
+
+    if (end_ptr[0] == '\0') {
+        dst->den = 1;
+        dst->num = num;
+        return 0;
+    }
+
+    if (end_ptr[0] != '/')
+        return AVERROR_INVALIDDATA;
+    p = end_ptr + 1;
+    den = (int)strtol(p, &end, 10);
+    if (errno == ERANGE || end == src) {
+        dst->den = 1;
+        return AVERROR_INVALIDDATA;
+    }
+    dst->den = den;
+
+    return 0;
+}
+
+static uint64_t dash_prop_get_int64(AVFormatContext *s, xmlNodePtr node, uint64_t *dst, const char *key)
+{
+    char *end_ptr = NULL;
+    char *val = xmlGetProp(node, key);
+    int ret = 0;
+
+    if (val) {
+        ret = strtoull(val, &end_ptr, 10);
+        if (errno == ERANGE) {
+            av_log(s, AV_LOG_WARNING, "overflow/underflow when get value of %s\n", key);
+            xmlFree(val);
+            return AVERROR_INVALIDDATA;
+        }
+        if (end_ptr == val || end_ptr[0] != '\0') {
+            av_log(s, AV_LOG_ERROR, "The %s field value is "
+            "not a valid number: %s\n", key, val);
+            xmlFree(val);
+            return AVERROR_INVALIDDATA;
+        }
+        *dst = ret;
+        xmlFree(val);
+    }
+    return ret;
+}
+
+static int dash_get_prop_ratio(AVFormatContext *s, xmlNodePtr node, AVRational *member, const char *key)
+{
+    char *val = xmlGetProp(node, key);
+    int ret = 0;
+    AVRational rate;
+
+    if (val) {
+        ret = get_ratio_from_string(&rate, val);
+        if (ret < 0)
+            av_log(s, AV_LOG_WARNING, "Ignoring invalid %s frame rate '%s'\n", key, val);
+        xmlFree(val);
+    }
+    member->num = rate.num;
+    member->den = rate.den;
+    return ret;
+}
+
 static int ishttp(char *url)
 {
     const char *proto_name = avio_find_protocol_name(url);
@@ -885,6 +955,15 @@  static int parse_manifest_representation(AVFormatContext *s, const char *url,
             ret = AVERROR(ENOMEM);
             goto end;
         }
+        if (c->adaptionset_lang) {
+            rep->lang = av_strdup(c->adaptionset_lang);
+            if (!rep->lang) {
+                av_log(s, AV_LOG_ERROR, "alloc language memory failure\n");
+                av_freep(&rep);
+                ret = AVERROR(ENOMEM);
+                goto end;
+            }
+        }
         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");
@@ -1070,15 +1149,61 @@  static int parse_manifest_representation(AVFormatContext *s, const char *url,
         }
 
         if (rep) {
+            uint64_t width = 0;
+            uint64_t height = 0;
+
             if (rep->fragment_duration > 0 && !rep->fragment_timescale)
                 rep->fragment_timescale = 1;
             rep->bandwidth = rep_bandwidth_val ? atoi(rep_bandwidth_val) : 0;
+            if (rep_bandwidth_val && (rep->bandwidth > c->adaptionset_maxbw || rep->bandwidth < c->adaptionset_minbw)) {
+                av_log(s, AV_LOG_WARNING, "The bandwidth of representation %s is incorrect, "
+                        "will ignore this representation.\n", rep_id_val);
+                free_representation(rep);
+                goto end;
+            }
+
+            ret = dash_prop_get_int64(s, representation_node, &width, "width");
+            if (ret < 0)
+                av_log(s, AV_LOG_WARNING, "Ignoring the width of representation %s is incorrect.\n", rep_id_val);
+            if (width > c->adaptionset_maxwidth || width < c->adaptionset_minwidth) {
+                av_log(s, AV_LOG_WARNING, "Ignoring width of representation %s is incorrect, "
+                "will ignore this representation.\n", rep_id_val);
+                free_representation(rep);
+                goto end;
+            }
+
+            ret = dash_prop_get_int64(s, representation_node, &height, "height");
+            if (ret < 0)
+                av_log(s, AV_LOG_WARNING, "Ignoring the height of representation %s is incorrect.\n", rep_id_val);
+            if (height > c->adaptionset_maxheight || height < c->adaptionset_minheight) {
+                av_log(s, AV_LOG_WARNING, "Ignoring height of representation %s is incorrect, "
+                "will ignore this representation.\n", rep_id_val);
+                free_representation(rep);
+                goto end;
+            }
+
             strncpy(rep->id, rep_id_val ? rep_id_val : "", sizeof(rep->id));
             rep->framerate = av_make_q(0, 0);
             if (type == AVMEDIA_TYPE_VIDEO && rep_framerate_val) {
-                ret = av_parse_video_rate(&rep->framerate, rep_framerate_val);
+                ret = get_ratio_from_string(&rep->framerate, rep_framerate_val);
                 if (ret < 0)
-                    av_log(s, AV_LOG_VERBOSE, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
+                    av_log(s, AV_LOG_WARNING, "Ignoring invalid frame rate '%s'\n", rep_framerate_val);
+                if (c->adaptionset_maxframerate.num > 0) {
+                    if (av_cmp_q(rep->framerate, c->adaptionset_maxframerate) == 1) {
+                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
+                        "will ignore this representation.\n", rep_id_val);
+                        free_representation(rep);
+                        goto end;
+                    }
+                }
+                if (c->adaptionset_minframerate.num > 0) {
+                    if (av_cmp_q(rep->framerate, c->adaptionset_minframerate) == -1) {
+                        av_log(s, AV_LOG_WARNING, "Ignoring frame rate of representation %s incorrect, "
+                        "will ignore this representation.\n", rep_id_val);
+                        free_representation(rep);
+                        goto end;
+                    }
+                }
             }
 
             switch (type) {
@@ -1106,6 +1231,7 @@  static int parse_manifest_representation(AVFormatContext *s, const char *url,
     subtitle_rep_idx += type == AVMEDIA_TYPE_SUBTITLE;
 
 end:
+
     if (rep_id_val)
         xmlFree(rep_id_val);
     if (rep_bandwidth_val)
@@ -1116,6 +1242,62 @@  end:
     return ret;
 }
 
+static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node)
+{
+    int ret = 0;
+    DASHContext *c = s->priv_data;
+
+    if (!adaptionset_node) {
+        av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n");
+        return AVERROR(EINVAL);
+    }
+    c->adaptionset_minframerate = av_make_q(0, 0);
+    c->adaptionset_maxframerate = av_make_q(0, 0);
+    c->adaptionset_maxbw = UINT64_MAX;
+    c->adaptionset_minbw = 0;
+    c->adaptionset_minwidth = 0;
+    c->adaptionset_maxwidth = UINT64_MAX;
+    c->adaptionset_minheight = 0;
+    c->adaptionset_maxheight = UINT64_MAX;
+
+    c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang");
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minbw, "minBandwidth");
+    if (ret  < 0)
+        c->adaptionset_minbw = 0;
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxbw, "maxBandwidth");
+    if (ret <= 0)
+        c->adaptionset_maxbw = UINT64_MAX;
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minwidth, "minWidth");
+    if (ret < 0)
+        c->adaptionset_minwidth = 0;
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxwidth, "maxWidth");
+    if (ret <= 0)
+        c->adaptionset_maxwidth = UINT64_MAX;
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_minheight, "minHeight");
+    if (ret < 0)
+        c->adaptionset_minheight = 0;
+
+    ret = dash_prop_get_int64(s, adaptionset_node, &c->adaptionset_maxheight, "maxHeight");
+    if (ret <= 0)
+        c->adaptionset_maxheight = UINT64_MAX;
+
+    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_minframerate, "minFrameRate");
+    if (ret < 0) {
+        c->adaptionset_minframerate = av_make_q(0, 0);
+    }
+
+    ret = dash_get_prop_ratio(s, adaptionset_node, &c->adaptionset_maxframerate, "maxFrameRate");
+    if (ret < 0)
+        c->adaptionset_maxframerate = av_make_q(0, 0);
+
+    return ret;
+}
+
 static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
                                         xmlNodePtr adaptionset_node,
                                         xmlNodePtr mpd_baseurl_node,
@@ -1131,19 +1313,10 @@  static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
     xmlNodePtr adaptionset_segmentlist_node = NULL;
     xmlNodePtr adaptionset_supplementalproperty_node = NULL;
     xmlNodePtr node = NULL;
-    c->adaptionset_contenttype_val = xmlGetProp(adaptionset_node, "contentType");
-    c->adaptionset_par_val = xmlGetProp(adaptionset_node, "par");
-    c->adaptionset_lang_val = xmlGetProp(adaptionset_node, "lang");
-    c->adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth");
-    c->adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth");
-    c->adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth");
-    c->adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth");
-    c->adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight");
-    c->adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight");
-    c->adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate");
-    c->adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate");
-    c->adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment");
-    c->adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching");
+
+    ret = parse_manifest_adaptationset_attr(s, adaptionset_node);
+    if (ret < 0)
+        return ret;
 
     node = xmlFirstElementChild(adaptionset_node);
     while (node) {
@@ -1170,12 +1343,15 @@  static int parse_manifest_adaptationset(AVFormatContext *s, const char *url,
                                                 adaptionset_segmentlist_node,
                                                 adaptionset_supplementalproperty_node);
             if (ret < 0) {
-                return ret;
+                goto end;
             }
         }
         node = xmlNextElementSibling(node);
     }
-    return 0;
+
+end:
+    av_freep(&c->adaptionset_lang);
+    return ret;
 }
 
 static int parse_programinformation(AVFormatContext *s, xmlNodePtr node)
@@ -2148,6 +2324,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, "lang", rep->lang, 0);
+                av_freep(&rep->lang);
+            }
         }
         for (i = 0; i < c->n_subtitles; i++) {
             rep = c->subtitles[i];
@@ -2155,6 +2335,10 @@  static int dash_read_header(AVFormatContext *s)
             rep->assoc_stream = s->streams[rep->stream_index];
             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, "lang", rep->lang, 0);
+                av_freep(&rep->lang);
+            }
         }
     }