GNOME Bugzilla – Bug 758515
dashdemux: Set framerate based on the Manifest on the caps
Last modified: 2015-12-08 08:08:06 UTC
Dashdemux has set the width and height information from MPD description. However, some embedded devices which are not insufficient H/W resources need more information to assign the H/W resource. For example, Framerate information needed to assign the H/W resources. So I suggested that dashdemux also need to set the framerate information from MPD description. I attached the patch for my suggestion. Could you plz kindly review my suggestion?
Created attachment 316072 [details] [review] This patch suggested for the setting framerate at dashdemux.
Comment on attachment 316072 [details] [review] This patch suggested for the setting framerate at dashdemux. Instead of this GstFrameRate helper structure (which is being leaked in multiple cases here), just set gint fps_den = 0, fps_num = 0; and do: static gboolean gst_mpd_client_get_video_stream_framerate (GstActiveStream * stream, gint * fps_den, gint *fps_num) Also, the framerate should probably only be set on the caps if it could be retrieved.
Created attachment 316132 [details] [review] I modified my code as your comment.
Review of attachment 316132 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +886,3 @@ gst_caps_set_simple (caps, "width", G_TYPE_INT, width, "height", + G_TYPE_INT, height, "framerate", GST_TYPE_FRACTION, fps_num, + fps_den, NULL); I think it makes sense to a) set the framerate also if width==0 || height == 0 and b) to only set the framerate if we actually know it (get_video_stream_framerate() returns a boolean).
Created attachment 316635 [details] [review] Modified as your comment.
Review of attachment 316635 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +878,3 @@ width = gst_mpd_client_get_video_stream_width (stream); height = gst_mpd_client_get_video_stream_height (stream); + fps_ret = I would call this have_fps but this also works :) @@ +888,3 @@ gst_caps_set_simple (caps, "width", G_TYPE_INT, width, "height", + G_TYPE_INT, height, "framerate", GST_TYPE_FRACTION, fps_num, + fps_den, NULL); Here you always set a framerate if width/height exists. @@ +892,3 @@ + if (fps_ret) { + gst_caps_set_simple (caps, "framerate", GST_TYPE_FRACTION, fps_num, + fps_den, NULL); Just move all this out of the if. if (width > 0 && height > 0) { set width and height } if (fps_ret) { set framerate }
Created attachment 316686 [details] [review] suggestion for setting the framerate at dashdemux.
Thank you for your review. I modified the code as your comment. Thanks.
Review of attachment 316686 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +868,3 @@ guint width = 0, height = 0; + gint fps_num = 0, fps_den = 1; + gboolean have_ret = FALSE; have_fps maybe? have_ret is very generic ::: ext/dash/gstmpdparser.c @@ +5642,3 @@ + if (stream == NULL || stream->cur_adapt_set == NULL + || stream->cur_representation == NULL) + return 0; FALSE instead of 0 And maybe add the checks below, like ... @@ +5644,3 @@ + return 0; + + if (stream->cur_adapt_set->maxFrameRate != NULL) { if (stream->cur_adapt_set && stream->cur_adapt_set->maxFrameRate != NULL) { ... } @@ +5646,3 @@ + if (stream->cur_adapt_set->maxFrameRate != NULL) { + *fps_num = stream->cur_adapt_set->maxFrameRate->num; + *fps_den = stream->cur_adapt_set->maxFrameRate->den; We should probably prefer the framerate attribute and only fall back to maxFramerate if framerate does not exist. According to the spec, both attributes can be in AdaptationSet, Representation and SubRepresentation
Created attachment 316714 [details] [review] Setting the framerate at dashdemux.
See my last two comments about changing the code flow and preferring framerate over maxFramerate.
I know your last two comments. 1) if (stream->cur_adapt_set && stream-cur_adapt_set->maxFrameRate != NULL) I think "stream->cur_adapt_set &&" does not need. Because of "stream->cur_adapt_set" already check the as below. + if (stream == NULL || stream->cur_adapt_set == NULL + || stream->cur_representation == NULL) + return FALSE; 2) I already check your comment. Please See your gstmpdparser.h. And check the struct as below. struct _GstActiveStream { ... GstAdaptationSetNode *cur_adapt_set; GstRepresentationNode *cur_representation; .. } struct GstAdaptationSetNode { ... GstFrameRate *minFrameRate; GstFrameRate *maxFrameRate; GstRepresentationBaseType *RepresentationBase; ... } struct GstRepresentationNode { ... GstRepresentationBaseType *RepresentationBase; ... } struct GstRepresentationBaseType { ... GstFrameRate *frameRate; ... }
(In reply to Suhwang Kim from comment #12) > I know your last two comments. > > 1) if (stream->cur_adapt_set && stream-cur_adapt_set->maxFrameRate != NULL) > I think "stream->cur_adapt_set &&" does not need. Because of > "stream->cur_adapt_set" already check the as below. > + if (stream == NULL || stream->cur_adapt_set == NULL > + || stream->cur_representation == NULL) > + return FALSE; That's exactly my point :) Remove the additional check at the top and add the relevant checks to each if. That way it's easier to extend the code later. > 2) I already check your comment. > Please See your gstmpdparser.h. > [...] What I mean is that you should use the frameRate field if available, and only otherwise use maxFrameRate. Currently the code does it the other way around.
First of your comment and intend, I agree with you :) I'm going to modify as your comment. Thank you. Second one, I want to see the other side. The forward future gstreamer is likely to play important role in many embedded devices. So I want you to see the other side such as embedded system. Many embedded system is difficult to change the H/W resource under the adaptive streaming. It is about seamless playback. So When embedded devices is assigned by H/W resources, It would selected maxResolution, maxFramerate and so on. So I selected the MaxFramerate if available. How about your opinion? If you agree with my opinion, I would not modify the code. If not, I would modify the code as your comment. I also respect for your opinion. :) Thanks,
There's also a max-framerate caps field btw, it's used e.g. by libgstvideo. For the case you mention, there should be a better selection algorithm in general and all information we have available would be available via the new GstStream API then. The application could then do the decision about which stream to select. Also if framerate is available, it is the average framerate. If you hardware can decode that average framerate if it was a fixed framerate, chances are very good that it can decode this as average framerate too. Sometimes there might be higher framerates for a short time, but on average it should be able to decode it in real-time and if you have a minimal amount of buffering in the pipeline there should be no problem.
Created attachment 316744 [details] [review] modify as your comment. Thank your for your detailed comment. I agree with you. So I modified the code as your comment.
Review of attachment 316744 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +5659,3 @@ + if (stream->cur_adapt_set && stream->cur_adapt_set->maxFrameRate != NULL) { + *fps_num = stream->cur_adapt_set->maxFrameRate->num; + *fps_den = stream->cur_adapt_set->maxFrameRate->den; maxFrameRate should probably be in RepresentationBase. According to the spec it can also be in Representation and SubRepresentation, not only in AdaptationSet. Can you provide a second patch to the parser to change this accordingly? Apart from that this patch looks ok now
Created attachment 316790 [details] [review] This patch added max/min FrameRate in Representation
Created attachment 316791 [details] [review] This patch suggested for the setting framerate at dashdemux.
Yes I can provide a second patches. :) Sorry for delay.
Review of attachment 316790 [details] [review]: ::: ext/dash/gstmpdparser.h @@ +223,3 @@ GstRatio *sar; + GstFrameRate *minFrameRate; + GstFrameRate *maxFrameRate; They are still in AdaptationSet too. Remove them from there as they are now handled in RepresentationBase :)
Review of attachment 316791 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +5679,3 @@ + if (stream->cur_adapt_set && stream->cur_adapt_set->maxFrameRate != NULL) { + *fps_num = stream->cur_adapt_set->maxFrameRate->num; + *fps_den = stream->cur_adapt_set->maxFrameRate->den; This would then also disappear when the other patch is updated
Created attachment 316909 [details] [review] maxFramerate should be in RepresentationBase with your comment.
Created attachment 316910 [details] [review] This patch is suggestion for the setting framerate at dashdemux .
Dear Sebastian Dröge Thank you for your comment. I did not seem to care. :) Thank you again.
commit a878d6e67eca1bf3069dd75bab38b00bd20f3b88 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 8 09:53:11 2015 +0200 dash: Fix unit test after moving of framerates to RepresentationBaseType commit 6cbaec75935b503f6dd2aaaac253fe10c6a0a6cc Author: suhwang.kim <suhwang.kim@lge.com> Date: Tue Dec 8 09:33:39 2015 +0900 dashdemux: Suggestion for setting the framerate information. Dashdemux has set the width and height information from MPD manifest. Some embedded devices which are not insufficient H/W resources need more information such as framerate to assign H/W resources. So I suggested that dashdemux also needs to set the framerate information from MDP manifest. https://bugzilla.gnome.org/show_bug.cgi?id=758515 commit 6d8dd482c4a1a826cb50b35ea9e42dd09a2b6d9b Author: suhwang.kim <suhwang.kim@lge.com> Date: Tue Dec 8 09:23:22 2015 +0900 dashdemux: maxFrameRate & minFrameRate should be in RepresentationBase. According to the spec, they can be in AdaptationSet, Representation and SubRepresentation. So They should be in RepresentationBase. https://bugzilla.gnome.org/show_bug.cgi?id=758515