| Message ID | 20200316164701.81967-1-lq@chinaffmpeg.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avformat/dashdec: fix memleak for commit commit e134c203 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Hello, before the actual review I want to tell you that I actually agree with Nicholas. I don't see the point of already parsing stuff that is not used yet, especially if it involves allocations and checks. Steven Liu: > These member will be used for get more correct information of the MPD > > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavformat/dashdec.c | 134 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 107 insertions(+), 27 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 5bbe5d3985..27d44c2633 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -122,19 +122,17 @@ 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; > + int64_t adaptionset_minbw; > + int64_t adaptionset_maxbw; > + int64_t adaptionset_minwidth; > + int64_t adaptionset_maxwidth; > + int64_t adaptionset_minheight; > + int64_t adaptionset_maxheight; > + AVRational adaptionset_minframerate; > + AVRational adaptionset_maxframerate; > + int adaptionset_segmentalignment; > + int adaptionset_bitstreamswitching; > > int n_videos; > struct representation **videos; > @@ -1116,6 +1114,90 @@ end: > return ret; > } > > +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node) > +{ > + int ret = 0; > + DASHContext *c = s->priv_data; > + char *adaptionset_minbw_val = NULL; > + char *adaptionset_maxbw_val = NULL; > + char *adaptionset_minwidth_val = NULL; > + char *adaptionset_maxwidth_val = NULL; > + char *adaptionset_minheight_val = NULL; > + char *adaptionset_maxheight_val = NULL; > + char *adaptionset_minframerate_val = NULL; > + char *adaptionset_maxframerate_val = NULL; > + char *adaptionset_segmentalignment_val = NULL; > + char *adaptionset_bitstreamswitching_val = NULL; How about using one pointer for all this stuff? It would still be clear what is currently parsed because the right-hand side (e.g. "xmlGetProp(adaptionset_node, "minHeight");") contains it; furthermore, it allows to make overlong lines shorter. > + > + if (!adaptionset_node) { > + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n"); > + return AVERROR(EINVAL); > + } > + > + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang"); Is it certain that parse_manifest_adaptionset() (and therefore parse_manifest_adaptionset_attr() as well) gets only executed once (even for spec-incompliant input) although it is called inside a loop in parse_manifest()? If it gets executed more than once, the string from earlier runs would be overwritten above and therefore leak. And is there actually a way to distinguish the "node doesn't exist" case from allocation failure? > + > + adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth"); > + if (adaptionset_minbw_val) { > + c->adaptionset_minbw = (int64_t) strtoll(adaptionset_minbw_val, NULL, 10); You don't check for errors here: Check errno as well as the endposition. Furthermore, is there a reason you are using a signed type for what appears to be a fundamentally unsigned type. (How weird that libxml2 doesn't seem to provide a way to parse numbers without an allocation.) > + xmlFree(adaptionset_minbw_val); > + } > + adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth"); > + if (adaptionset_maxbw_val) { > + c->adaptionset_maxbw = (int64_t) strtoll(adaptionset_maxbw_val, NULL, 10); > + xmlFree(adaptionset_maxbw_val); > + } > + adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth"); > + if (adaptionset_minwidth_val) { > + c->adaptionset_minwidth = (int64_t) strtoll(adaptionset_minwidth_val, NULL, 10); > + xmlFree(adaptionset_minwidth_val); > + } > + adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth"); > + if (adaptionset_maxwidth_val) { > + c->adaptionset_maxwidth = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); > + xmlFree(adaptionset_maxwidth_val); > + } > + adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight"); > + if (adaptionset_minheight_val) { > + c->adaptionset_minheight = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); > + xmlFree(adaptionset_minheight_val); > + } > + adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight"); > + if (adaptionset_maxheight_val) { > + c->adaptionset_maxheight = (int64_t) strtoll(adaptionset_maxheight_val, NULL, 10); > + xmlFree(adaptionset_maxheight_val); > + } > + adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate"); > + if (adaptionset_minframerate_val) { > + ret = av_parse_video_rate(&c->adaptionset_minframerate, adaptionset_minframerate_val); > + xmlFree(adaptionset_minframerate_val); > + if (ret < 0) > + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid min frame rate '%s'\n", adaptionset_minframerate_val); AV_LOG_VERBOSE is clearly wrong. If you ignore the error, it should be AV_LOG_WARNING; if not AV_LOG_ERROR. You are doing something between these two: It might be that ret gets overwritten or it might be returned. If it gets returned, you are skipping parse_manifest_representation() by simply returning the error to parse_manifest() which then ignores it. > + } > + adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate"); > + if (adaptionset_maxframerate_val) { > + ret = av_parse_video_rate(&c->adaptionset_maxframerate, adaptionset_maxframerate_val); > + xmlFree(adaptionset_maxframerate_val); > + if (ret < 0) > + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid max frame rate '%s'\n", adaptionset_maxframerate_val); > + } > + adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment"); > + if (adaptionset_segmentalignment_val) { > + c->adaptionset_segmentalignment = 0; > + if (!av_strcasecmp(adaptionset_segmentalignment_val, "true")) > + c->adaptionset_segmentalignment = 1; > + xmlFree(adaptionset_segmentalignment_val); > + } > + adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching"); > + if (adaptionset_bitstreamswitching_val) { > + c->adaptionset_bitstreamswitching = 0; > + if (!av_strcasecmp(adaptionset_bitstreamswitching_val, "true")) > + c->adaptionset_bitstreamswitching = 1; > + xmlFree(adaptionset_bitstreamswitching_val); > + } > + > + return ret; > +} > + > static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > xmlNodePtr adaptionset_node, > xmlNodePtr mpd_baseurl_node, > @@ -1124,26 +1206,16 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > xmlNodePtr period_segmentlist_node) > { > int ret = 0; > - DASHContext *c = s->priv_data; > xmlNodePtr fragment_template_node = NULL; > xmlNodePtr content_component_node = NULL; > xmlNodePtr adaptionset_baseurl_node = NULL; > 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) { > @@ -2148,6 +2220,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 (c->adaptionset_lang) { > + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); > + xmlFree(c->adaptionset_lang); This is very dangerous: You are freeing c->adaptionset_lang, but you are not resetting the pointer to NULL. This leads to use-after-frees. Furthermore, I haven't read the relevant documents, but I can't believe that there can't be multiple different languages for audio and subtitles (that is after all the main purpose of having different audio and subtitle streams in the first place!). It would make more sense to have one language per representation as this seems to correspond to the streams in FFmpeg. Additionally, you are ony freeing c->adaptionset_lang if there is an audio or subtitle stream; a video-only stream would still leak if it had a defined language. And it would also leak if an error happens so that we do not reach the lines above (e.g. if av_new_program() fails). > + } > } > for (i = 0; i < c->n_subtitles; i++) { > rep = c->subtitles[i]; > @@ -2155,6 +2231,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 (c->adaptionset_lang) { > + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); > + xmlFree(c->adaptionset_lang); > + } > } > } > > A final note: libxml2 allows to use custom allocator, free and strdup functions. How about using our functions for this so that we don't need to worry about e.g. xmlFree(NULL). And we could use av_freep(). - Andreas PS: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/257908.html
Thanks Steven Liu > 2020年3月19日 上午12:48，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Hello, > > before the actual review I want to tell you that I actually agree with > Nicholas. I don't see the point of already parsing stuff that is not > used yet, especially if it involves allocations and checks. Yes, I agreed with all of you, but I think we should try to "full support all the attributes parse" what writes in the sepcification, And try to use all the attribute. Because those attribute in the specification is not useless and have some reason, I’m not member of the specification author, > > Steven Liu: >> These member will be used for get more correct information of the MPD >> >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >> --- >> libavformat/dashdec.c | 134 +++++++++++++++++++++++++++++++++--------- >> 1 file changed, 107 insertions(+), 27 deletions(-) >> >> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c >> index 5bbe5d3985..27d44c2633 100644 >> --- a/libavformat/dashdec.c >> +++ b/libavformat/dashdec.c >> @@ -122,19 +122,17 @@ 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; >> + int64_t adaptionset_minbw; >> + int64_t adaptionset_maxbw; >> + int64_t adaptionset_minwidth; >> + int64_t adaptionset_maxwidth; >> + int64_t adaptionset_minheight; >> + int64_t adaptionset_maxheight; >> + AVRational adaptionset_minframerate; >> + AVRational adaptionset_maxframerate; >> + int adaptionset_segmentalignment; >> + int adaptionset_bitstreamswitching; >> >> int n_videos; >> struct representation **videos; >> @@ -1116,6 +1114,90 @@ end: >> return ret; >> } >> >> +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node) >> +{ >> + int ret = 0; >> + DASHContext *c = s->priv_data; >> + char *adaptionset_minbw_val = NULL; >> + char *adaptionset_maxbw_val = NULL; >> + char *adaptionset_minwidth_val = NULL; >> + char *adaptionset_maxwidth_val = NULL; >> + char *adaptionset_minheight_val = NULL; >> + char *adaptionset_maxheight_val = NULL; >> + char *adaptionset_minframerate_val = NULL; >> + char *adaptionset_maxframerate_val = NULL; >> + char *adaptionset_segmentalignment_val = NULL; >> + char *adaptionset_bitstreamswitching_val = NULL; > > How about using one pointer for all this stuff? It would still be clear > what is currently parsed because the right-hand side (e.g. > "xmlGetProp(adaptionset_node, "minHeight");") contains it; furthermore, > it allows to make overlong lines shorter. This will update in version 2. > >> + >> + if (!adaptionset_node) { >> + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n"); >> + return AVERROR(EINVAL); >> + } >> + >> + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang"); > > Is it certain that parse_manifest_adaptionset() (and therefore > parse_manifest_adaptionset_attr() as well) gets only executed once (even > for spec-incompliant input) although it is called inside a loop in > parse_manifest()? If it gets executed more than once, the string from > earlier runs would be overwritten above and therefore leak. > > And is there actually a way to distinguish the "node doesn't exist" case > from allocation failure? This will update in version 2, add the lang into representation->lang for every stream. > >> + >> + adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth"); >> + if (adaptionset_minbw_val) { >> + c->adaptionset_minbw = (int64_t) strtoll(adaptionset_minbw_val, NULL, 10); > > You don't check for errors here: Check errno as well as the endposition. > Furthermore, is there a reason you are using a signed type for what > appears to be a fundamentally unsigned type. Will update in version > > (How weird that libxml2 doesn't seem to provide a way to parse numbers > without an allocation.) > >> + xmlFree(adaptionset_minbw_val); >> + } >> + adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth"); >> + if (adaptionset_maxbw_val) { >> + c->adaptionset_maxbw = (int64_t) strtoll(adaptionset_maxbw_val, NULL, 10); >> + xmlFree(adaptionset_maxbw_val); >> + } >> + adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth"); >> + if (adaptionset_minwidth_val) { >> + c->adaptionset_minwidth = (int64_t) strtoll(adaptionset_minwidth_val, NULL, 10); >> + xmlFree(adaptionset_minwidth_val); >> + } >> + adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth"); >> + if (adaptionset_maxwidth_val) { >> + c->adaptionset_maxwidth = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); >> + xmlFree(adaptionset_maxwidth_val); >> + } >> + adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight"); >> + if (adaptionset_minheight_val) { >> + c->adaptionset_minheight = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); >> + xmlFree(adaptionset_minheight_val); >> + } >> + adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight"); >> + if (adaptionset_maxheight_val) { >> + c->adaptionset_maxheight = (int64_t) strtoll(adaptionset_maxheight_val, NULL, 10); >> + xmlFree(adaptionset_maxheight_val); >> + } >> + adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate"); >> + if (adaptionset_minframerate_val) { >> + ret = av_parse_video_rate(&c->adaptionset_minframerate, adaptionset_minframerate_val); >> + xmlFree(adaptionset_minframerate_val); >> + if (ret < 0) >> + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid min frame rate '%s'\n", adaptionset_minframerate_val); > > AV_LOG_VERBOSE is clearly wrong. If you ignore the error, it should be > AV_LOG_WARNING; Ignore the waning shortly, just like the flvenc “duration and file size update failed” at the end of live streaming. At this should update if the specification said overwritten the framerate, but I don’t see that now, maybe I misunderstand that, need check that more time. > if not AV_LOG_ERROR. You are doing something between > these two: It might be that ret gets overwritten or it might be > returned. If it gets returned, you are skipping > parse_manifest_representation() by simply returning the error to > parse_manifest() which then ignores it. > >> + } >> + adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate"); >> + if (adaptionset_maxframerate_val) { >> + ret = av_parse_video_rate(&c->adaptionset_maxframerate, adaptionset_maxframerate_val); >> + xmlFree(adaptionset_maxframerate_val); >> + if (ret < 0) >> + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid max frame rate '%s'\n", adaptionset_maxframerate_val); >> + } >> + adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment"); >> + if (adaptionset_segmentalignment_val) { >> + c->adaptionset_segmentalignment = 0; >> + if (!av_strcasecmp(adaptionset_segmentalignment_val, "true")) >> + c->adaptionset_segmentalignment = 1; >> + xmlFree(adaptionset_segmentalignment_val); >> + } >> + adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching"); >> + if (adaptionset_bitstreamswitching_val) { >> + c->adaptionset_bitstreamswitching = 0; >> + if (!av_strcasecmp(adaptionset_bitstreamswitching_val, "true")) >> + c->adaptionset_bitstreamswitching = 1; >> + xmlFree(adaptionset_bitstreamswitching_val); >> + } >> + >> + return ret; >> +} >> + >> static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, >> xmlNodePtr adaptionset_node, >> xmlNodePtr mpd_baseurl_node, >> @@ -1124,26 +1206,16 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, >> xmlNodePtr period_segmentlist_node) >> { >> int ret = 0; >> - DASHContext *c = s->priv_data; >> xmlNodePtr fragment_template_node = NULL; >> xmlNodePtr content_component_node = NULL; >> xmlNodePtr adaptionset_baseurl_node = NULL; >> 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) { >> @@ -2148,6 +2220,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 (c->adaptionset_lang) { >> + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); >> + xmlFree(c->adaptionset_lang); > > This is very dangerous: You are freeing c->adaptionset_lang, but you are > not resetting the pointer to NULL. This leads to use-after-frees. > > Furthermore, I haven't read the relevant documents, but I can't believe > that there can't be multiple different languages for audio and subtitles > (that is after all the main purpose of having different audio and > subtitle streams in the first place!). It would make more sense to have > one language per representation as this seems to correspond to the > streams in FFmpeg. Yes, will update in version 2. > > Additionally, you are ony freeing c->adaptionset_lang if there is an > audio or subtitle stream; a video-only stream would still leak if it had > a defined language. And it would also leak if an error happens so that > we do not reach the lines above (e.g. if av_new_program() fails). > >> + } >> } >> for (i = 0; i < c->n_subtitles; i++) { >> rep = c->subtitles[i]; >> @@ -2155,6 +2231,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 (c->adaptionset_lang) { >> + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); >> + xmlFree(c->adaptionset_lang); >> + } >> } >> } >> >> > A final note: libxml2 allows to use custom allocator, free and strdup > functions. How about using our functions for this so that we don't need > to worry about e.g. xmlFree(NULL). And we could use av_freep(). Ok Thanks Steven
Steven Liu (12020-03-17): > These member will be used for get more correct information of the MPD > > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavformat/dashdec.c | 134 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 107 insertions(+), 27 deletions(-) > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 5bbe5d3985..27d44c2633 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -122,19 +122,17 @@ 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; > + int64_t adaptionset_minbw; > + int64_t adaptionset_maxbw; > + int64_t adaptionset_minwidth; > + int64_t adaptionset_maxwidth; > + int64_t adaptionset_minheight; > + int64_t adaptionset_maxheight; > + AVRational adaptionset_minframerate; > + AVRational adaptionset_maxframerate; > + int adaptionset_segmentalignment; > + int adaptionset_bitstreamswitching; > > int n_videos; > struct representation **videos; > @@ -1116,6 +1114,90 @@ end: > return ret; > } > > +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node) > +{ > + int ret = 0; > + DASHContext *c = s->priv_data; > + char *adaptionset_minbw_val = NULL; > + char *adaptionset_maxbw_val = NULL; > + char *adaptionset_minwidth_val = NULL; > + char *adaptionset_maxwidth_val = NULL; > + char *adaptionset_minheight_val = NULL; > + char *adaptionset_maxheight_val = NULL; > + char *adaptionset_minframerate_val = NULL; > + char *adaptionset_maxframerate_val = NULL; > + char *adaptionset_segmentalignment_val = NULL; > + char *adaptionset_bitstreamswitching_val = NULL; > + > + if (!adaptionset_node) { > + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n"); > + return AVERROR(EINVAL); > + } > + > + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang"); > + > + adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth"); > + if (adaptionset_minbw_val) { > + c->adaptionset_minbw = (int64_t) strtoll(adaptionset_minbw_val, NULL, 10); > + xmlFree(adaptionset_minbw_val); > + } > + adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth"); > + if (adaptionset_maxbw_val) { > + c->adaptionset_maxbw = (int64_t) strtoll(adaptionset_maxbw_val, NULL, 10); > + xmlFree(adaptionset_maxbw_val); > + } > + adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth"); > + if (adaptionset_minwidth_val) { > + c->adaptionset_minwidth = (int64_t) strtoll(adaptionset_minwidth_val, NULL, 10); > + xmlFree(adaptionset_minwidth_val); > + } > + adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth"); > + if (adaptionset_maxwidth_val) { > + c->adaptionset_maxwidth = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); > + xmlFree(adaptionset_maxwidth_val); > + } > + adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight"); > + if (adaptionset_minheight_val) { > + c->adaptionset_minheight = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); > + xmlFree(adaptionset_minheight_val); > + } > + adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight"); > + if (adaptionset_maxheight_val) { > + c->adaptionset_maxheight = (int64_t) strtoll(adaptionset_maxheight_val, NULL, 10); > + xmlFree(adaptionset_maxheight_val); > + } Please don't use copy-paste when coding. Whenever you might want it, make a helper function. > + adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate"); > + if (adaptionset_minframerate_val) { > + ret = av_parse_video_rate(&c->adaptionset_minframerate, adaptionset_minframerate_val); > + xmlFree(adaptionset_minframerate_val); > + if (ret < 0) > + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid min frame rate '%s'\n", adaptionset_minframerate_val); > + } > + adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate"); > + if (adaptionset_maxframerate_val) { > + ret = av_parse_video_rate(&c->adaptionset_maxframerate, adaptionset_maxframerate_val); > + xmlFree(adaptionset_maxframerate_val); > + if (ret < 0) > + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid max frame rate '%s'\n", adaptionset_maxframerate_val); > + } These don't seem to support the special values accepted by av_parse_video_rate(). > + adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment"); > + if (adaptionset_segmentalignment_val) { > + c->adaptionset_segmentalignment = 0; > + if (!av_strcasecmp(adaptionset_segmentalignment_val, "true")) > + c->adaptionset_segmentalignment = 1; > + xmlFree(adaptionset_segmentalignment_val); > + } > + adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching"); > + if (adaptionset_bitstreamswitching_val) { > + c->adaptionset_bitstreamswitching = 0; > + if (!av_strcasecmp(adaptionset_bitstreamswitching_val, "true")) > + c->adaptionset_bitstreamswitching = 1; > + xmlFree(adaptionset_bitstreamswitching_val); > + } Helper function. > + > + return ret; > +} > + > static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > xmlNodePtr adaptionset_node, > xmlNodePtr mpd_baseurl_node, > @@ -1124,26 +1206,16 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, > xmlNodePtr period_segmentlist_node) > { > int ret = 0; > - DASHContext *c = s->priv_data; > xmlNodePtr fragment_template_node = NULL; > xmlNodePtr content_component_node = NULL; > xmlNodePtr adaptionset_baseurl_node = NULL; > 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) { > @@ -2148,6 +2220,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 (c->adaptionset_lang) { > + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); > + xmlFree(c->adaptionset_lang); > + } > } > for (i = 0; i < c->n_subtitles; i++) { > rep = c->subtitles[i]; > @@ -2155,6 +2231,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 (c->adaptionset_lang) { > + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); > + xmlFree(c->adaptionset_lang); > + } > } > } > Regards,
Quoting Steven Liu (2020-03-19 00:30:52) > > Thanks > > Steven Liu > > > 2020年3月19日 上午12:48，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > > > Hello, > > > > before the actual review I want to tell you that I actually agree with > > Nicholas. I don't see the point of already parsing stuff that is not > > used yet, especially if it involves allocations and checks. > > Yes, I agreed with all of you, but I think we should try to "full > support all the attributes parse" what writes in the sepcification, > And try to use all the attribute. Because those attribute in the > specification is not useless and have some reason, I’m not member of > the specification author, You seem to be contradicting yourself. Either you agree that it is pointless to parse attributes that do not get used, in which case you shouldn't send patches that do precisely that. Or you believe that we should in fact parse things that do not get used, then you do not agree with the other commenters. Both at once are not possible. FWIW I believe that the entire point of demuxers is to export useful data to the caller. Therefore any code that does not contribute towards that goal is useless and should not be present in the tree.
> 2020年3月20日 下午4:34，Anton Khirnov <anton@khirnov.net> 写道： > > Quoting Steven Liu (2020-03-19 00:30:52) >> >> Thanks >> >> Steven Liu >> >>> 2020年3月19日 上午12:48，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >>> >>> Hello, >>> >>> before the actual review I want to tell you that I actually agree with >>> Nicholas. I don't see the point of already parsing stuff that is not >>> used yet, especially if it involves allocations and checks. >> >> Yes, I agreed with all of you, but I think we should try to "full >> support all the attributes parse" what writes in the sepcification, >> And try to use all the attribute. Because those attribute in the >> specification is not useless and have some reason, I’m not member of >> the specification author, > > You seem to be contradicting yourself. Either you agree that it is > pointless to parse attributes that do not get used, in which case you > shouldn't send patches that do precisely that. > Or you believe that we should in fact parse things that do not get used, > then you do not agree with the other commenters. Both at once are not > possible. > > FWIW I believe that the entire point of demuxers is to export useful > data to the caller. Therefore any code that does not contribute towards > that goal is useless and should not be present in the tree. Why the specification add these attributes into the documents if these attributes not useful, just kidding? I said I’m not the specification author. But I see the specification said: for example: @minFrameRate O specifies the minimum @framerate value in all Representations in this Adaptation Set. This value is encoded in the same format as the @frameRate attribute. If not present, the value is unknown. Then, I think demuxer should give user warning message if the result is not same as specification, because it maybe will get wrong result if continue. I agree all of you is the precondition, because I need friendly comments, but I think I need think what you guys said, and think about how to make the demuxer better, Not just say something and don’t care about it, I add these attributes is just want make dashdec more standard support, not just partial support. Review patch welcome. > > -- > Anton Khirnov > _______________________________________________ > 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
Steven Liu (12020-03-20): > Why the specification add these attributes into the documents if these attributes not useful, just kidding? > I said I’m not the specification author. What Anton and I are trying to explain to you is that either your code handles the attributes according to the specification, with actual technical consequences, or your code is useless. Getting the attribute, parsing it, getting it into a variable, carrying this variable around, and doing nothing with that variable is a waste of effort. For example, adaptionset_lang reaches the metadata dictionary, and is therefore available for the application: it's useful. On the other hand, adaptionset_minwidth is just stored in a variable and does nothing: get rid of if. Maybe later you'll come to implement something that uses adaptionset_minwidth: since you already wrote the code, you can commit all of it together. In short: don't build half a bridge, either build the whole bridge or keep the materials for later. Regards,
> 2020年3月20日 下午6:51，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-20): >> Why the specification add these attributes into the documents if these attributes not useful, just kidding? >> I said I’m not the specification author. > > What Anton and I are trying to explain to you is that either your code > handles the attributes according to the specification, with actual > technical consequences, or your code is useless. > > Getting the attribute, parsing it, getting it into a variable, carrying > this variable around, and doing nothing with that variable is a waste of > effort. > > For example, adaptionset_lang reaches the metadata dictionary, and is > therefore available for the application: it's useful. > > On the other hand, adaptionset_minwidth is just stored in a variable and > does nothing: get rid of if. > > Maybe later you'll come to implement something that uses > adaptionset_minwidth: since you already wrote the code, you can commit > all of it together. > > In short: don't build half a bridge, either build the whole bridge or > keep the materials for later. Do you mean, 1. I should add all the code in this patch, not make it two or four step complete the code? If this is right, I think I should submit version 3 to make that, 2. I just think send this patch first step to fix memleak, If this patch is ok I will send the second patch to use the minbw maxbw minframerate maxframerate, If the 2nd step is unnecessary, I can merge all the step in one patch and submit here. :D > > Regards, > > -- > Nicolas George Thanks Steven Liu
Steven Liu (12020-03-20):
> Do you mean,
I mean you should only propose code that actually does something. That
should be easy to understand: if you store into a variable without using
the value, remove it; then iterate.
Regards,
> 2020年3月20日 下午7:05，Nicolas George <george@nsup.org> 写道： > > Steven Liu (12020-03-20): >> Do you mean, > > I mean you should only propose code that actually does something. That > should be easy to understand: if you store into a variable without using > the value, remove it; then iterate. Ok, I get it, thanks all of your suggestions. > > Regards, > > -- > Nicolas George Thanks Steven Liu
diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index 5bbe5d3985..27d44c2633 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -122,19 +122,17 @@ 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; + int64_t adaptionset_minbw; + int64_t adaptionset_maxbw; + int64_t adaptionset_minwidth; + int64_t adaptionset_maxwidth; + int64_t adaptionset_minheight; + int64_t adaptionset_maxheight; + AVRational adaptionset_minframerate; + AVRational adaptionset_maxframerate; + int adaptionset_segmentalignment; + int adaptionset_bitstreamswitching; int n_videos; struct representation **videos; @@ -1116,6 +1114,90 @@ end: return ret; } +static int parse_manifest_adaptationset_attr(AVFormatContext *s, xmlNodePtr adaptionset_node) +{ + int ret = 0; + DASHContext *c = s->priv_data; + char *adaptionset_minbw_val = NULL; + char *adaptionset_maxbw_val = NULL; + char *adaptionset_minwidth_val = NULL; + char *adaptionset_maxwidth_val = NULL; + char *adaptionset_minheight_val = NULL; + char *adaptionset_maxheight_val = NULL; + char *adaptionset_minframerate_val = NULL; + char *adaptionset_maxframerate_val = NULL; + char *adaptionset_segmentalignment_val = NULL; + char *adaptionset_bitstreamswitching_val = NULL; + + if (!adaptionset_node) { + av_log(s, AV_LOG_WARNING, "Cannot get AdaptionSet\n"); + return AVERROR(EINVAL); + } + + c->adaptionset_lang = xmlGetProp(adaptionset_node, "lang"); + + adaptionset_minbw_val = xmlGetProp(adaptionset_node, "minBandwidth"); + if (adaptionset_minbw_val) { + c->adaptionset_minbw = (int64_t) strtoll(adaptionset_minbw_val, NULL, 10); + xmlFree(adaptionset_minbw_val); + } + adaptionset_maxbw_val = xmlGetProp(adaptionset_node, "maxBandwidth"); + if (adaptionset_maxbw_val) { + c->adaptionset_maxbw = (int64_t) strtoll(adaptionset_maxbw_val, NULL, 10); + xmlFree(adaptionset_maxbw_val); + } + adaptionset_minwidth_val = xmlGetProp(adaptionset_node, "minWidth"); + if (adaptionset_minwidth_val) { + c->adaptionset_minwidth = (int64_t) strtoll(adaptionset_minwidth_val, NULL, 10); + xmlFree(adaptionset_minwidth_val); + } + adaptionset_maxwidth_val = xmlGetProp(adaptionset_node, "maxWidth"); + if (adaptionset_maxwidth_val) { + c->adaptionset_maxwidth = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); + xmlFree(adaptionset_maxwidth_val); + } + adaptionset_minheight_val = xmlGetProp(adaptionset_node, "minHeight"); + if (adaptionset_minheight_val) { + c->adaptionset_minheight = (int64_t) strtoll(adaptionset_maxwidth_val, NULL, 10); + xmlFree(adaptionset_minheight_val); + } + adaptionset_maxheight_val = xmlGetProp(adaptionset_node, "maxHeight"); + if (adaptionset_maxheight_val) { + c->adaptionset_maxheight = (int64_t) strtoll(adaptionset_maxheight_val, NULL, 10); + xmlFree(adaptionset_maxheight_val); + } + adaptionset_minframerate_val = xmlGetProp(adaptionset_node, "minFrameRate"); + if (adaptionset_minframerate_val) { + ret = av_parse_video_rate(&c->adaptionset_minframerate, adaptionset_minframerate_val); + xmlFree(adaptionset_minframerate_val); + if (ret < 0) + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid min frame rate '%s'\n", adaptionset_minframerate_val); + } + adaptionset_maxframerate_val = xmlGetProp(adaptionset_node, "maxFrameRate"); + if (adaptionset_maxframerate_val) { + ret = av_parse_video_rate(&c->adaptionset_maxframerate, adaptionset_maxframerate_val); + xmlFree(adaptionset_maxframerate_val); + if (ret < 0) + av_log(s, AV_LOG_VERBOSE, "Ignoring invalid max frame rate '%s'\n", adaptionset_maxframerate_val); + } + adaptionset_segmentalignment_val = xmlGetProp(adaptionset_node, "segmentAlignment"); + if (adaptionset_segmentalignment_val) { + c->adaptionset_segmentalignment = 0; + if (!av_strcasecmp(adaptionset_segmentalignment_val, "true")) + c->adaptionset_segmentalignment = 1; + xmlFree(adaptionset_segmentalignment_val); + } + adaptionset_bitstreamswitching_val = xmlGetProp(adaptionset_node, "bitstreamSwitching"); + if (adaptionset_bitstreamswitching_val) { + c->adaptionset_bitstreamswitching = 0; + if (!av_strcasecmp(adaptionset_bitstreamswitching_val, "true")) + c->adaptionset_bitstreamswitching = 1; + xmlFree(adaptionset_bitstreamswitching_val); + } + + return ret; +} + static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, xmlNodePtr adaptionset_node, xmlNodePtr mpd_baseurl_node, @@ -1124,26 +1206,16 @@ static int parse_manifest_adaptationset(AVFormatContext *s, const char *url, xmlNodePtr period_segmentlist_node) { int ret = 0; - DASHContext *c = s->priv_data; xmlNodePtr fragment_template_node = NULL; xmlNodePtr content_component_node = NULL; xmlNodePtr adaptionset_baseurl_node = NULL; 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) { @@ -2148,6 +2220,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 (c->adaptionset_lang) { + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); + xmlFree(c->adaptionset_lang); + } } for (i = 0; i < c->n_subtitles; i++) { rep = c->subtitles[i]; @@ -2155,6 +2231,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 (c->adaptionset_lang) { + av_dict_set(&rep->assoc_stream->metadata, "lang", c->adaptionset_lang, 0); + xmlFree(c->adaptionset_lang); + } } }
These member will be used for get more correct information of the MPD Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/dashdec.c | 134 +++++++++++++++++++++++++++++++++--------- 1 file changed, 107 insertions(+), 27 deletions(-)