GNOME Bugzilla – Bug 775615
decodebin3: Select default track by GstStreamFlags
Last modified: 2018-05-06 14:40:11 UTC
Default selected track will be determined by GST_STREAM_FLAG_{SELECT,UNSELECT}. Note that, if parent bin (i.e., playbin3) requested any track explicitly, the requested track will have higher priority than default selected track.
Created attachment 341382 [details] [review] decodebin3: Select default track by GstStreamFlags
Created attachment 344092 [details] [review] Choice 'GST_STREAM_FLAG_SELECT' stream as a defaut track when multiple parsebins
Hello All Decodebin3 has multiple parsebins as following. ---------------------------------- parsebin0 <= one video stream (e.g. stream-id: video-1, flag: GST_STREAM_FLAG_NONE) parsebin1 <= one audio stream (e.g. stream-id: audio-1, flag: GST_STREAM_FLAG_NONE) parsebin2 <= one audio stream (e.g. stream-id: audio-2, flag: GST_STREAM_FLAG_SELECT) ---------------------------------- Actually, below is test stream. ==> http://hls-streamer2-dev.minutv.ee/2d6870d65cd1b616ec04c107288081a9_1467970545web/vod/indur_test_lg/index.m3u8 Let's pretend that parsebin2's audio stream is default audio track. In this case 'stream-collection' msg is posted by each parsebins and total is 3 times. Last collection is from parsebin2 and stream(audio-2) can not be selected and can not be added 'requested-selection' list. Because, stream(audio-1) is already inserted to 'requested-selection' list. Thus, I remove stream(audio-1) to 'requested-selection' list and add stream(audio-2) to the list. Please review our patch. Thanks.
Review of attachment 341382 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1023,3 @@ + + /* 5. If not, match one stream of each type */ + for (i = 0; i < g_list_length (sorted); i++) { When iterating lists use the following: for (tmp = list; tmp; tmp = tmp->next) { something = (cast) tmp->data; ... }
Review of attachment 344092 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +409,3 @@ } G_STMT_END +#define DUMP_SELECTION_LIST(list, name) G_STMT_START { \ While this is quite useful (and should/could be used in more places), can you put this in a separate commit ? * protect it with "#ifndef GST_DISABLE_GST_DEBUG" so we don't iterate the list if no debugging is enabled * Pass an object as an argument and use the _OBJECT debugging variant * Use a lower debugging category @@ +1103,3 @@ + gst_stream_type_get_name (curtype)); + sorted = g_list_prepend (sorted, stream); + for (j = 0; j < g_list_length (reused_streams); j++) { See comment about iterating lists in the other patch review. @@ +1125,3 @@ + + /* 5. If not, match one stream of each type */ + for (i = 0; i < g_list_length (sorted); i++) { See comment about iterating lists in the other patch review.
Created attachment 344672 [details] [review] decodebin3: Add useful macro to debug variable selection lists Hello Edward Hervey I separated the above commit to debugging variable selection lists. Please review the new commit. Thanks.
Review of attachment 344672 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1027,3 @@ dbin->requested_selection = tmp; dbin->selection_updated = TRUE; +#ifndef GST_DISABLE_GST_DEBUG Put the #ifndef in the macro (since it's reused all the time)
Created attachment 344702 [details] [review] decodebin3: Select default track by GstStreamFlags
(In reply to Edward Hervey from comment #4) > Review of attachment 341382 [details] [review] [review]: > > ::: gst/playback/gstdecodebin3.c > @@ +1023,3 @@ > + > + /* 5. If not, match one stream of each type */ > + for (i = 0; i < g_list_length (sorted); i++) { > > When iterating lists use the following: > for (tmp = list; tmp; tmp = tmp->next) { > something = (cast) tmp->data; > ... > } Done :) Hello HoonHee Could you update attachment 344092 [details] [review] on top of this patch?
Created attachment 344703 [details] [review] decodebin3: Add useful macro to debug variable selection lists
(In reply to Edward Hervey from comment #7) > Review of attachment 344672 [details] [review] [review]: > > ::: gst/playback/gstdecodebin3.c > @@ +1027,3 @@ > dbin->requested_selection = tmp; > dbin->selection_updated = TRUE; > +#ifndef GST_DISABLE_GST_DEBUG > > Put the #ifndef in the macro (since it's reused all the time) I moved the "#ifndef" macro outside of DUMP_SELECTION_LIST(). Please review the patch again. Thanks.
Created attachment 344706 [details] [review] Choice 'GST_STREAM_FLAG_SELECT' stream as a defaut track when multiple parsebins
Created attachment 344712 [details] [review] decodebin3: Re-order all streams and add to collection
Hello Seungha Yang and Edward Hervey. I think that it is more efficient to sorting all streams by stream type, flag and stream-id whenever collections are merged. It would be first video, then audio, then others. Please check my approach and review my patch. Thanks.
Review of attachment 344712 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1102,3 @@ + * GCompareFunc to use with lists of GstStream. + * Sorts GstStreams by mime type and select flag and stream-id + * First video, then audio, then others. Is there any added-value for having it in that order of types ? It just adds complexity to the code. Just using the type itself (which is an enum) should be sufficient (it still groups them by stream type). @@ +1143,3 @@ + + ret = + (flaga == GST_STREAM_FLAG_SELECT) ? ((flagb == Use & instead of == (it's a flagset, not an enum). @@ +1153,3 @@ + ida = gst_stream_get_stream_id (sa); + idb = gst_stream_get_stream_id (sb); + ret = (ida) ? ((idb) ? g_strcmp0 (ida, idb) : -1) : 1; just use g_strcmp0() directly. It handles NULL entries gracefully. @@ +1165,3 @@ GstStreamCollection *res = NULL; GList *tmp; + GList *all_streams = NULL; rename it to unsorted_streams for clarity.
(In reply to Seungha Yang from comment #8) > Created attachment 344702 [details] [review] [review] > decodebin3: Select default track by GstStreamFlags Is this still needed now that the collection is pre-sorted ?
(In reply to Edward Hervey from comment #16) > (In reply to Seungha Yang from comment #8) > > Created attachment 344702 [details] [review] [review] [review] > > decodebin3: Select default track by GstStreamFlags > > Is this still needed now that the collection is pre-sorted ? I don't think this patch is needed if we accept attachment 344712 [details] [review] :)
Created attachment 344771 [details] [review] decodebin3: Re-order all streams and add to collection
Review of attachment 344712 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1102,3 @@ + * GCompareFunc to use with lists of GstStream. + * Sorts GstStreams by mime type and select flag and stream-id + * First video, then audio, then others. I am not sure that I understand correctly about your question. I know that it is not a good solution to sorting whenever collection is updated in terms of code complexity. But, in case of adaptive stream likes HLS, there is a stream which has 1-video and 2-audio. For that stream, number of parsebin is 3 and we can not guarantee that collection messages from each parsebin are posted in order. Also, there is preferred stream by SELECT flag. So, I think that it is not enough to sort by just stream type. Therefore, we have to re-order all streams by stream type, SELECT flag and stream-id when multiple parsebins. @@ +1143,3 @@ + + ret = + (flaga == GST_STREAM_FLAG_SELECT) ? ((flagb == Done. @@ +1153,3 @@ + ida = gst_stream_get_stream_id (sa); + idb = gst_stream_get_stream_id (sb); + ret = (ida) ? ((idb) ? g_strcmp0 (ida, idb) : -1) : 1; Done. @@ +1165,3 @@ GstStreamCollection *res = NULL; GList *tmp; + GList *all_streams = NULL; Done.
Review of attachment 344712 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1102,3 @@ + * GCompareFunc to use with lists of GstStream. + * Sorts GstStreams by mime type and select flag and stream-id + * First video, then audio, then others. I'm fine with ordering by type and flags. I was just pointing out that we don't need a specific order for the stream type ordering. i.e. Don't use va and vb. Just use typea and typeb directly.
Created attachment 345004 [details] [review] decodebin3: Re-order all streams and add to collection
(In reply to Edward Hervey from comment #20) > Review of attachment 344712 [details] [review] [review]: > > ::: gst/playback/gstdecodebin3.c > @@ +1102,3 @@ > + * GCompareFunc to use with lists of GstStream. > + * Sorts GstStreams by mime type and select flag and stream-id > + * First video, then audio, then others. > > I'm fine with ordering by type and flags. I was just pointing out that we > don't need a specific order for the stream type ordering. > > i.e. Don't use va and vb. Just use typea and typeb directly. As your comment, I used typea and typeb directly instead of using va and vb. IMHO, It would be better and good to see for all developers if all streams are ordered by video, audio, text and others. Also, it is not easy to recognize if all streams are not ordered and complicated. Thanks.
Created attachment 350372 [details] [review] decodebin3: Re-order all streams and add to collection Dear All. In same stream type, old compare condition always returns -1 if stream A and stream B is different. When sorting, sa and sb is as following. ========================================================================== sa: 1afcda6b6e111d8beb07deb3bac202b948112257a70b25691531005a49e3e3d0/003 sb: 1afcda6b6e111d8beb07deb3bac202b948112257a70b25691531005a49e3e3d0/002 ========================================================================== We expect 002 is high order than 003 but, 003 is placed highly than 002 by returning always -1. Thus, I updated to return -1 or +1 if stream-id is different in same type. Please review my patch. Thanks.
Review of attachment 350372 [details] [review]: ::: gst/playback/gstdecodebin3.c @@ +1127,3 @@ + + if (ret != 0) { + GST_LOG ("Sort by stream-type: %d", ret); You can just put this block in the previous block ("if (typea != typeb)"). @@ +1147,3 @@ + ida = gst_stream_get_stream_id (sa); + idb = gst_stream_get_stream_id (sb); + ret = (ida) ? ((idb) ? (g_strcmp0 (ida, idb) > 0 ? 1 : -1) : -1) : 1; No need to check if ida/idb are NULL or not. g_strcmp0 handles NULL inputs.
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!
commit 1e28cba08883129cfcdb619b94858bbeb026fcb3 Author: hoonhee.lee <hoonhee.lee@lge.com> Date: Tue Apr 25 16:35:45 2017 +0900 decodebin3: Re-order all streams and add to collection Sort all streams from parsebins by stream type and SELECT flag and stream-id. First video, then audio, then others. https://bugzilla.gnome.org/show_bug.cgi?id=775615