GNOME Bugzilla – Bug 698712
playbin: autoplug video decoder and sink based on caps features
Last modified: 2013-05-07 13:26:15 UTC
As per the discussion in #gstreamer, the autoplugging of video_decoder and video_sink should be based on the number of common capsfeatures if their ranks are the same. This will help to autoplug the h/w accelerated video decoder and renderer if such elements are existing. patch is here: http://cgit.freedesktop.org/~sree/gst-plugins-base/commit/?id=b78d8b4e94350f08531e5fe010a123fb030ae716
Created attachment 242314 [details] [review] patch
Review of attachment 242314 [details] [review]: ::: gst/playback/gstplaybin2.c @@ +3300,3 @@ + + /* comparison based on the name of sink elements */ + return strcmp (GST_OBJECT_NAME (fs1), GST_OBJECT_NAME (fs2)); And if they are equal check the name of the decoder elements? @@ +3327,3 @@ + ve->vdec = gst_object_ref (d_factory); + ve->vsink = gst_object_ref (s_factory); + ve_list = g_list_prepend (ve_list, ve); You could keep this list smaller by only including decoder-sink combinations that are compatible by their template caps @@ +3355,3 @@ + tmp_templ = walk->data; + if (tmp_templ->direction == GST_PAD_SRC) + d_templ = tmp_templ; Shouldn't you break on the first srcpad here? @@ +3371,3 @@ + for (j = 0; j < s_caps_size; j++) { + s_features = gst_caps_get_features ((const GstCaps *) s_tmpl_caps, j); + if (gst_caps_features_is_equal (d_features, s_features)) You should also check that the corresponding structures are compatible @@ +3445,3 @@ GST_PLUGIN_FEATURE_LIST_DEBUG (mylist); + /* check whether the caps are asking for a list of video_decoders */ All this shouldn't be done just for video @@ +3470,3 @@ + GST_ELEMENT_FACTORY_TYPE_MEDIA_IMAGE, GST_RANK_MARGINAL); + video_sinks = + g_list_sort (video_sinks, gst_plugin_feature_rank_compare_func); I think these lists should be created at a central place like the other element factory lists already, otherwise this needs some locking here as the autoplug signals can be called from multiple threads at once iirc @@ +3474,3 @@ + GST_SOURCE_GROUP_LOCK (group); + /* create the video element list */ + group->velements = create_velements_list (mylist, video_sinks); Try to find a better name than mylist :) @@ +3725,3 @@ + ve = (GstVideoElement *) tmp->data; + if (!ve->is_dirty && factory == ve->vdec) { + ve->is_dirty = 1; This is not threadsafe, and why is this loop necessary at all? Shouldn't the overall loop only be over all velements that have factory==ve->vdec? @@ +3797,3 @@ + GST_SOURCE_GROUP_LOCK (group); + gst_element_set_state (group->video_sink, GST_STATE_NULL); + gst_object_unref (group->video_sink); Why do you destroy the previously selected sink here? @@ +3812,3 @@ + for (; tmp; tmp = tmp->next) { + ve = (GstVideoElement *) tmp->data; + ve->is_dirty = 1; Also not threadsafe and see above ;)
replay to comment2: Review of attachment 242314 [details] [review]: ::: gst/playback/gstplaybin2.c @@ +3300,3 @@ + + /* comparison based on the name of sink elements */ + return strcmp (GST_OBJECT_NAME (fs1), GST_OBJECT_NAME (fs2)); And if they are equal check the name of the decoder elements? I don't think it is possible to have two sink with same name ! possible?
(In reply to comment #3) > replay to comment2: > Review of attachment 242314 [details] [review] [details]: > > ::: gst/playback/gstplaybin2.c > @@ +3300,3 @@ > + > + /* comparison based on the name of sink elements */ > + return strcmp (GST_OBJECT_NAME (fs1), GST_OBJECT_NAME (fs2)); > > And if they are equal check the name of the decoder elements? > > I don't think it is possible to have two sink with same name ! possible? Well, no. But you could have multiple decoder-sink structs that have the same sink but different decoders.
reply to comment2: @@ +3327,3 @@ + ve->vdec = gst_object_ref (d_factory); + ve->vsink = gst_object_ref (s_factory); + ve_list = g_list_prepend (ve_list, ve); You could keep this list smaller by only including decoder-sink combinations that are compatible by their template caps the list of decoders here is already the filtered list based on caps. so the list won't be that big .Only thing is that it takes all video_sink_elements. And the sink-decoder template compatibility is checking in autoplug_select_cb. Does it not enough?
reply to comment2: @@ +3355,3 @@ + tmp_templ = walk->data; + if (tmp_templ->direction == GST_PAD_SRC) + d_templ = tmp_templ; Shouldn't you break on the first srcpad here? aha,,right. might cause one extra iteration. will fix it.
reply to comment2: @@ +3470,3 @@ + GST_ELEMENT_FACTORY_TYPE_MEDIA_IMAGE, GST_RANK_MARGINAL); + video_sinks = + g_list_sort (video_sinks, gst_plugin_feature_rank_compare_func); I think these lists should be created at a central place like the other element factory lists already, otherwise this needs some locking here as the autoplug signals can be called from multiple threads at once iirc If i understood correctly, we should keep two global list for audio_decoders and video_decoders like playbin->elements. right?
(In reply to comment #7) > reply to comment2: > @@ +3470,3 @@ > + GST_ELEMENT_FACTORY_TYPE_MEDIA_IMAGE, GST_RANK_MARGINAL); > + video_sinks = > + g_list_sort (video_sinks, gst_plugin_feature_rank_compare_func); > > I think these lists should be created at a central place like the other element > factory lists already, otherwise this needs some locking here as the autoplug > signals can be called from multiple threads at once iirc > > > If i understood correctly, we should keep two global list for audio_decoders > and video_decoders like playbin->elements. right? IMHO a list of compatible audio_decoder-audio_sink and video_decoder-video_sink combinations (two lists, like playbin->elements), which should also answer your question from comment #5 :)
@@ +3474,3 @@ + GST_SOURCE_GROUP_LOCK (group); + /* create the video element list */ + group->velements = create_velements_list (mylist, video_sinks); Try to find a better name than mylist :) mylist was not invented by myself :) it was there in the previous implementation.
@@ +3725,3 @@ + ve = (GstVideoElement *) tmp->data; + if (!ve->is_dirty && factory == ve->vdec) { + ve->is_dirty = 1; This is not threadsafe, and why is this loop necessary at all? Shouldn't the overall loop only be over all velements that have factory==ve->vdec? suppose the factory is not compatible with the sink element during caps compatibility check (line_no: 3772 to 3781) , then it will go for second iteration of while() before returning GST_AUTOPLUG_SELECT_SKIP. During the second iteration of while() , it should check for other possible factory-sink combination in the velement list also (which is not already checked==not dirty). Am i missing something? sorry for the thread-safty error :(
@@ +3797,3 @@ + GST_SOURCE_GROUP_LOCK (group); + gst_element_set_state (group->video_sink, GST_STATE_NULL); + gst_object_unref (group->video_sink); Why do you destroy the previously selected sink here? I think the previous explanation is enough..right? :) If the created sink is not compatible with the factory, then unref the sink and make sure that there is no more factory:video_sink combination in the velements list before returning GST_AUTOPLUG_SELECT_SKIP.
(In reply to comment #11) > @@ +3797,3 @@ > + GST_SOURCE_GROUP_LOCK (group); > + gst_element_set_state (group->video_sink, GST_STATE_NULL); > + gst_object_unref (group->video_sink); > > Why do you destroy the previously selected sink here? > > I think the previous explanation is enough..right? :) > If the created sink is not compatible with the factory, then unref the sink and > make sure that there is no more factory:video_sink combination in the velements > list before returning GST_AUTOPLUG_SELECT_SKIP. Yes, but why isn't the loop like this without all the diry flags? for (l = velements; l; l = l->next) { if (l->data->vdec != factory) continue; current_code; }
aha,,right . Thanks for the review. I will add the necessary changes.
Thanks for working on that :)
Created attachment 242415 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures.
Review of attachment 242415 [details] [review]: Looks almost good ::: gst/playback/gstplaybin2.c @@ +293,3 @@ +typedef struct +{ + GstElementFactory *dec; /* audo:video decoder */ Typo: audo @@ +323,3 @@ + GList *aelements; /* a list of GstAVElements for audio stream */ + GList *velements; /* a list of GstAVElements for video stream */ Why don't you store it inside GstPlayBin, and create it at the same time as the other elements list? @@ +3527,3 @@ + GST_SOURCE_GROUP_LOCK (group); + /* create the audio/video element list */ + *ave_list = create_avelements_list (factory_list, sink_list); IMHO this should be done at a central place and not inside the autoplug callbacks, similar to the other elements list
(In reply to comment #16) > > @@ +323,3 @@ > > + GList *aelements; /* a list of GstAVElements for audio stream */ > + GList *velements; /* a list of GstAVElements for video stream */ > > Why don't you store it inside GstPlayBin, and create it at the same time as the > other elements list? > Hm, I think i didn't understand the correct meaning of GstSourceGroup :(. Group is only intended for the next URI (if we set).right? Aha, right ,the older group always get deactivated before activating the next.. Stupid me :(
Yes, the GstSourceGroup is all information relevant for an URI (i.e. there's one for the current and one for the next)
(In reply to comment #16) > @@ +3527,3 @@ > + GST_SOURCE_GROUP_LOCK (group); > + /* create the audio/video element list */ > + *ave_list = create_avelements_list (factory_list, sink_list); > > IMHO this should be done at a central place and not inside the autoplug > callbacks, similar to the other elements list I guess you are asking me to do it in a separate function ? (not to move the stuffs from autoplug callback)
(In reply to comment #18) > Yes, the GstSourceGroup is all information relevant for an URI (i.e. there's > one for the current and one for the next) Actually the velements and aelements are also specific to one URI,,right? :)
No, I meant to create these two lists in a central place (e.g. NULL->READY state change like the other elements). I might be missing something but shouldn't they be completely independent of the actual streams?
(In reply to comment #20) > (In reply to comment #18) > > Yes, the GstSourceGroup is all information relevant for an URI (i.e. there's > > one for the current and one for the next) > > Actually the velements and aelements are also specific to one URI,,right? :) These lists are changing for each uri.. So i guess GstSourceGroup is the right place for these..Also i guess that these lists are supposed to get freed during deactivation instead of playbin->finalize() (something similar to group->video_sink and group->audio_sink). What do you think? (In reply to comment #21) > No, I meant to create these two lists in a central place (e.g. NULL->READY > state change like the other elements). I might be missing something but > shouldn't they be completely independent of the actual streams? *NO* i think :) The list is creating based on the srcpad caps of demuxers. So it is supposed to get created in autoplug_factories_cb() itself. Each URI can come up with different demuxers and decdoers ... Am i missing something??
Ah! The factory_list you pass to create_avelements_list() is from the autoplug_factories_cb(). I would create a decoder-sink list of *all* possible decoders somewhere in NULL->READY like playbin->elements. And then only filter that list created in NULL->READY against the factory_list from autoplug_factories_cb(). The way you create this complicated decoder-sink list with the number of common features only once, and later only filter that against the actual possible factories.
Hm, so do you want to change it like that ? Then it will create a huge list of decoders and sink since it is possible to have mulitple combinations :)... Does the current implementation is better that that?
(In reply to comment #24) > Hm, so do you want to change it like that ? > Then it will create a huge list of decoders and sink since it is possible to > have mulitple combinations :)... Does the current implementation is better that > that? Well, let's do some profiling then ;) You might be right... anyway, this is just an optimization :) But by keeping it in the group but having it based on the factories you get in autoplug_factory, this will not work properly if there are multiple audio or video streams using different codecs (and thus requiring different decoders usually).
Do you have any sample like that? (multiple video streams with different codecs in a single container)..
For video no, but for audio it's common. Also you can just create containers with any number of video/audio streams in any codec with gst-launch. Matroska handles almost all codecs.
(In reply to comment #27) > For video no, but for audio it's common. Also you can just create containers > with any number of video/audio streams in any codec with gst-launch. Matroska > handles almost all codecs. Aha, this make sense :)..I will think about it a bit more .. Thanks Sebastian ...
Created attachment 242522 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures. I have changed a bit in my implementation itself to support multiple streams in single container. Whenever the autoplug_factory asks for new decoders list, it will create a new dec-sink list. Not that good , because it will create new list for demuxers and parsers.(anyway it will deactivate the older one) :) . But might be better than keeping all combination of dec-sink list. But if you are still seeing that the all-combination-of-dec-sink-list approach is better then i will look in to that sometimes later.
The problem with that is that it assumes that autoplug_factories will always be followed by the autoplug_select for the same pad, with nothing else happening in between. I don't think this assumption is valid.
(In reply to comment #30) > The problem with that is that it assumes that autoplug_factories will always be > followed by the autoplug_select for the same pad, with nothing else happening > in between. I don't think this assumption is valid. For eg: if multiple video_pads (with different encoded data) are calling autoplug_factories without invoking autoplug_select. right? Is there a case like that? :) Just a clarification: Does the current playbin implementation is working fine with multiple codec streams in a single container?
(In reply to comment #31) > (In reply to comment #30) > > The problem with that is that it assumes that autoplug_factories will always be > > followed by the autoplug_select for the same pad, with nothing else happening > > in between. I don't think this assumption is valid. > > For eg: if multiple video_pads (with different encoded data) are calling > autoplug_factories without invoking autoplug_select. right? Is there a case > like that? :) It's all multithreaded :) > Just a clarification: Does the current playbin implementation is working fine > with multiple codec streams in a single container? Yes
(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > The problem with that is that it assumes that autoplug_factories will always be > > > followed by the autoplug_select for the same pad, with nothing else happening > > > in between. I don't think this assumption is valid. > > > > For eg: if multiple video_pads (with different encoded data) are calling > > autoplug_factories without invoking autoplug_select. right? Is there a case > > like that? :) > > It's all multithreaded :) > Thats right..:) but I was thinking about some practical cases.. Yup, I guess i understood it :) > > Just a clarification: Does the current playbin implementation is working fine > > with multiple codec streams in a single container? > > Yes
The playbin->elements are creating from autoplug_factoreis () not during NULL_TO_READY.. I guess it is to support the dynamic addition of elements also. I wonder what would be the better place for playbin->aelements/playbin->velements creation. --create it from playbin_update_elemetns_list() and utilize the same playbin->elemetns_lock ? ? OR --create it from NULL_TO_READY and utilize PLAYBIN_LOCK()?
Oh right. Yes, do it from gst_play_bin_update_elements_list() and also only if the cookie has changed. However that list would need to be independent of the actual caps and will be the "large" list of all possible combinations (how large is it actually?).
Yup, now I changed my mind and the list is independent of caps :).. Also going to use the same playbin->elements_lock instead of creating a new one for velements/aelements. fine? simple grep: gst-inspect-1.0 | grep decoder | grep video | wc -l 37 gst-inspect-1.0 | grep sink | grep video | wc -l 7 gst-inspect-1.0 | grep sink | grep audio | wc -l 5 gst-inspect-1.0 | grep decoder | grep audio | wc -l 26 :)
+ gst-inspect-1.0 | grep decoder | grep image | wc -l 22
Yes sounds good
Created attachment 242571 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures. This one maintains the global list of dec-sink combination irrespective of the caps.. Do we need to lock with playbin->elements_lock with in the autoplug_select callback ?
(In reply to comment #39) > Created an attachment (id=242571) [details] [review] > playbin: autoplug the audio/video decoders and sinks based on capsfeatures. > > This one maintains the global list of dec-sink combination irrespective of the > caps.. > > Do we need to lock with playbin->elements_lock with in the autoplug_select > callback ? Yes, otherwise looks good now. One way to keep this decoder-sink combination list much smaller would be to only include combinations here that have more than one caps feature in common (i.e. not only handle system memory but also something else special). For all other decoder-sink combinations we already know that they have exactly on caps feature in common (system memory), and thus don't need to store that information.
Created attachment 242618 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures.
(In reply to comment #40) > Yes, otherwise looks good now. > > > One way to keep this decoder-sink combination list much smaller would be to > only include combinations here that have more than one caps feature in common > (i.e. not only handle system memory but also something else special). For all > other decoder-sink combinations we already know that they have exactly on caps > feature in common (system memory), and thus don't need to store that > information. IMHO, it is better to add as a separate optimization patch.
Created attachment 242622 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures.
Hi Sebastian, Anything blocking to push this? Are you expecting to do the small-dec-sink-combination patch also? If so, then we can't set the sink element from autoplug_select for most of the decoders since we are keeping only the list of dec-sink combination which is having more than one capsfeatres. so it will go for the autovideosink/autoaudiosink approach to plug the sink elements . I feels that it is an overhead .
Nothing is really blocking this, I'll look at this on Monday :) For the optimization, yes it should be a separate commit. But why wouldn't we be able to select a proper sink anymore then? We can do that right now without the dec-sink list too
A unit test for this would be great :)
(In reply to comment #45) > Nothing is really blocking this, I'll look at this on Monday :) Thanks :) > > For the optimization, yes it should be a separate commit. But why wouldn't we > be able to select a proper sink anymore then? We can do that right now without > the dec-sink list too In the current implementation (with my patch), we are searching in the dec-sink list with in autoplug_select to find out the appropriate sink element . So there is no autovideosink or autoaudiosink ..right? But if we keep a dec-sink list which is only having combinations with more than one capsfeatures ,then how it could be possible to find out the sink element with out using autovideosink/autoaudiosink. (eg: in a case of having no element in dec-sink list)? What i am saying is, "keep the large dec-sink list and get an advantage of no autovideosink/autoaudiosink initiation :)
(In reply to comment #47) > In the current implementation (with my patch), we are searching in the dec-sink > list with in autoplug_select to find out the appropriate sink element . So > there is no autovideosink or autoaudiosink ..right? > > But if we keep a dec-sink list which is only having combinations with more than > one capsfeatures ,then how it could be possible to find out the sink element > with out using autovideosink/autoaudiosink. (eg: in a case of having no element > in dec-sink list)? > > What i am saying is, "keep the large dec-sink list and get an advantage of no > autovideosink/autoaudiosink initiation :) playbin already has something more clever than the auto*sink instantiation. It choses, depending on the caps, the most optimal sink with the highest rank. It only falls back to auto*sink if it can't do anything more clever. What advantage do we get from the full dec-sink list over the sparse one here? If we're looking at dec-sink combinations with 1 common caps features we don't need to look at the list at all and can only look at the elements list as of now and select the highest ranked decoder and the highest ranked sink that can handle the caps.
(In reply to comment #48) > (In reply to comment #47) > > > playbin already has something more clever than the auto*sink instantiation. It > choses, depending on the caps, the most optimal sink with the highest rank. It > only falls back to auto*sink if it can't do anything more clever. It seems that "gst-launch playbin uri=///" is creating an autovideosink by default.(with out having any dec-sink-list patch) !! > What advantage do we get from the full dec-sink list over the sparse one here? > If we're looking at dec-sink combinations with 1 common caps features we don't > need to look at the list at all and can only look at the elements list as of > now and select the highest ranked decoder and the highest ranked sink that can > handle the caps.
Review of attachment 242622 [details] [review]: ::: gst/playback/gstplaybin2.c @@ +3393,3 @@ + + if (!gst_caps_can_intersect (d_caps, s_caps)) + continue; Optimization idea: You can combine this with the structure_can_intersect() call in the other loop @@ +3446,3 @@ + if (gst_caps_features_is_equal (d_features, s_features) && + gst_structure_can_intersect (d_struct, s_struct)) + n_common++; Here you're counting the number of compatible GstStructure combinations of these two elements. Not sure if that is the number we want to look at, e.g. consider the following: - decoder A has one structure per GstVideoFormat per GstCapsFeatures. Say it supports 5 GstVideoFormats, and for each of these 3 GstCapsFeatures: "EGLImage", "GLTextureUploadMeta", "VASurface". This gives you 15 structures - decoder B supports the same GstCapsFeatures and GstVideoFormats, but has only 3 structures Which one is better? Your algorithm would say A is better but I'd say that both should be equivalent. I think what we should count here is (of all structures that are compatible) the number of distinct caps features that are supported by both. Which would be 3 in both cases of my example. @@ +3574,2 @@ /* Check if we already have an audio/video sink and if this is the case * put it as the first element of the array */ This here btw is part of the non-auto*sink sink selection I meant
(In reply to comment #50) > Review of attachment 242622 [details] [review]: > > ::: gst/playback/gstplaybin2.c > @@ +3393,3 @@ > + > + if (!gst_caps_can_intersect (d_caps, s_caps)) > + continue; > > Optimization idea: You can combine this with the structure_can_intersect() call k :) > in the other loop > > @@ +3446,3 @@ > + if (gst_caps_features_is_equal (d_features, s_features) && > + gst_structure_can_intersect (d_struct, s_struct)) > + n_common++; > > Here you're counting the number of compatible GstStructure combinations of > these two elements. Not sure if that is the number we want to look at, e.g. > consider the following: > - decoder A has one structure per GstVideoFormat per GstCapsFeatures. Say it > supports 5 GstVideoFormats, and for each of these 3 GstCapsFeatures: > "EGLImage", "GLTextureUploadMeta", "VASurface". This gives you 15 structures > - decoder B supports the same GstCapsFeatures and GstVideoFormats, but has only > 3 structures > > Which one is better? Your algorithm would say A is better but I'd say that both > should be equivalent. I think what we should count here is (of all structures > that are compatible) the number of distinct caps features that are supported by > both. Which would be 3 in both cases of my example. Aha, good point ! this can happen for sink also :) may be i can create a list for capsfeatures also. > @@ +3574,2 @@ > /* Check if we already have an audio/video sink and if this is the case > * put it as the first element of the array */ > > This here btw is part of the non-auto*sink sink selection I meant I didn't get it actually :) Do you mean that current gstplaybin2 won't autoplug the auto*sinks?
(In reply to comment #51) > > Which one is better? Your algorithm would say A is better but I'd say that both > > should be equivalent. I think what we should count here is (of all structures > > that are compatible) the number of distinct caps features that are supported by > > both. Which would be 3 in both cases of my example. > > Aha, good point ! this can happen for sink also :) may be i can create a list > for capsfeatures also. Yes, but no need to store that. Just create one during creation of the dec-sink list and use it for counting. > > @@ +3574,2 @@ > > /* Check if we already have an audio/video sink and if this is the case > > * put it as the first element of the array */ > > > > This here btw is part of the non-auto*sink sink selection I meant > > I didn't get it actually :) > Do you mean that current gstplaybin2 won't autoplug the auto*sinks? Yes, it's automatically plugging the highest ranked sink that supports a stream. This is used e.g. to make sure that encoded MP3 streams are directly passed to pulseaudio if pulseaudio can handle that.
Please could you add the URL to this bug to the commit message in the next iteration, so people can more easily find this discussion again if they come across the change.
Created attachment 242835 [details] [review] playbin: autoplug the audio/video decoders and sinks based on capsfeatures.
Review of attachment 242835 [details] [review]: Looks good, I'll push this tomorrow after some testing. Just some comments below ::: gst/playback/gstplaybin2.c @@ +3447,3 @@ + n_common_cf = get_n_common_capsfeatures (d_factory, s_factory); + if (n_common_cf < 1) + continue; For the optimization idea this should be < 2 or <= 1. There will always be one in common: system memory. And it is 0 only if there are no compatible caps at all @@ +3818,3 @@ + ave = (GstAVElement *) tmp->data; + if (ave->dec != factory) + continue; For the optimization idea the decoder might not be in the list, but the code below should still happen (the check if the already selected sink works or not). @@ +3902,3 @@ } /* it's a sink, see if an instance of it actually works */ And here is another part of the magic sink selection that is already there, we get here for sinks that support an encoded stream.
Thanks for the review :)
The list lengths here are: audio: 487 video: 626 And we're iterating over these lists a lot. As we're always looking for the decoder factory in this list it might make sense to use either a hash table with the decoder factory as key, or some kind of balanced binary tree structure (or equivalent) like GSequence. Or the optimization to keep the list itself much smaller. I'm not going to push it the way it is now without optimizing this part a bit. Also for some reason it causes g_criticals(), e.g. in gst123 when playing a video file: (gst123:1674): GLib-GObject-WARNING **: invalid cast from `GstXvImageSink' to `GstBin' (gst123:1674): GStreamer-CRITICAL **: gst_bin_iterate_sinks: assertion `GST_IS_BIN (bin)' failed (gst123:1674): GStreamer-CRITICAL **: gst_iterator_next: assertion `it != NULL' failed (gst123:1674): GLib-GObject-CRITICAL **: g_value_unset: assertion `G_IS_VALUE (value)' failed But that's probably a bug in there. I guess it assumes that the sinks are always bins (and by default it would often be autovideosink, a bin), which is not true anymore after your change (xvimagesink is chosen here because it fits best, which is right).
Yes, indeed. Ignore the gst123 problem, it's their bug.
I will try to explain the problem with optimization approach with an example :) Suppose we have a decoder decoder_A: and Sink_1, Sink_2 and Sink_3 decoder_A : Sink_1 : 2 common capsfeatures with decoder_A having rank PRIMARY and Sink_1 having rank PRIMARY decoder_A : Sink_2 : 2 common capsfeatures with decoder_A having rank PRIMARY and Sink_2 having rank SECONDARY. decoder_A : Sink_3 : 1 common capsfeatures with decoder_A having rank PRIMARY and Sink_3 having rank PRIMARY As per optimization logic: we will only keep first two options in the dec-sink list. Suppose in the autoplug_select callback, if the decoder_A+Sink_1 combination fails to connect then it will search in the dec-sink list and select the decoder_A+Sink_2 combination. But we are supposed to select the decoder_A+Sink_3 combination. Am i right?
If you have no objection then i will proceed with GSequence instead of GList for the dec-sink list.(with out the optimization patch)
Yes, sounds good. These few kb of memory wasted by the dense list shouldn't be that much of a problem
Created attachment 243011 [details] [review] playbin: Use GSequence instead of GList to store the GstAVElement list. This one is just replacing the GList with GSequence. I don't know whether it is giving any significant advantage in this scenario :) Because it is still iterating through the Sequence (not using g_sequence_search, g_sequence_lookup methods).
If you're not using search/lookup it's mostly pointless, only useful for the insertion then. Why can't you use those functions instead of iterating? I didn't look too close but all your iterating was basically a search, right?
case1: avelement list creation: we might get an advantage in insertion. case2: decoder list creation in autoplug_factories: case3: comparison in autoplug_select: I wonder how it make sense to use the _search() method in both of these cases(1 & 2) since we are searching for an AVElement in the sequence (and where we have no clue about the sink elements in each AVElement). We cannot use the decoder factory as key since there are many elements having same key.Creating a temporary avelement with only a decoder_factory and searching for that avelement in the sequence (with a comparison function based on on the rank/name of only the decoder_factory) is a wired way since we need to iterate the sequece again (if the first case failed) for finding out the next dec-sink combination. Don't know whether i am missing something!
A minor correction: (In reply to comment #64) > case1: avelement list creation: we might get an advantage in insertion. > > case2: decoder list creation in autoplug_factories: > case3: comparison in autoplug_select: > > I wonder how it make sense to use the _search() method in both of these cases(1 > & 2) cases (2 & 3) since we are searching for an AVElement in the sequence (and where we have > no clue about the sink elements in each AVElement). We cannot use the decoder > factory as key since there are many elements having same key.Creating a > temporary avelement with only a decoder_factory and searching for that > avelement in the sequence (with a comparison function based on on the rank/name > of only the decoder_factory) is a wired way since we need to iterate the > sequece again (if the first case failed) for finding out the next dec-sink > combination. > > > Don't know whether i am missing something!
Search based on decoder name and mark it as dirty each time is a way to do this :).. Do we really need to do it like that?
You can use the decoder factory as key and search for it. That will give you the iterator to the first element with this decoder factory, and starting from that you can iterate over the next elements until you reached an element with a different decoder factory. That's possible in GSequence because it is sorted and allows multiple elements with the same key.
The problem is that the list in which we are searching has sorted based on many factores which includes the rank of sink elemetns, n_common_cf of sink-dec etc. So how could it be possible to correctly handle the case of *NON EQUAL FACTORIES* in cmp_func of g_sequence_search() ?
(In reply to comment #67) > You can use the decoder factory as key and search for it. That will give you > the iterator to the first element with this decoder factory, and starting from > that you can iterate over the next elements until you reached an element with a > different decoder factory. That's possible in GSequence because it is sorted > and allows multiple elements with the same key. I hope you understood what i mean by comment 68 :) Please tell if any further explanation/work needed on top of this to get it upstream.
Yeah let me think about that a bit more, sorry. Using GSequence the way you do it right now is pointless, but there must be a way to optimize this to not require iterating twice over 1000 list elements for each decoder.
(In reply to comment #70) > Yeah let me think about that a bit more, sorry. Using GSequence the way you do > it right now is pointless, but there must be a way to optimize this to not > require iterating twice over 1000 list elements for each decoder. I know it is pointless :) (comment 62). Yup, needs to find out some way to optimize it. But what I said is: "doing just a g_sequence_search() is not possible with this list"
Please don't hesitate to add the optimization patch on top of my initial patch if you are working on that :)
Hmm, what about only sorting the GSequence by the decoder factory (and nothing else). 1) Creation is trivial and we get the insertion advantage there 2) Not so trivial, we would create a list of possible decoders in autoplug_select and only sort this small list by our metrics (number of common caps features, etc). Gives us the lookup advantage plus a small extra cost for sorting a list with less than 10 elements (which is basically for free) 3) We would use lookup there and then take the first iterator to the used decoder factory, and continue iterating from there on until the decoder factory changes. Gives us the lookup advantage and no extra costs What do you think? Am I missing something?
(In reply to comment #73) > Hmm, what about only sorting the GSequence by the decoder factory (and nothing > else). > > 1) Creation is trivial and we get the insertion advantage there > 2) Not so trivial, we would create a list of possible decoders in > autoplug_select autoplug_select ?? or autoplug_factories and only sort this small list by our metrics (number of common > caps features, etc). Gives us the lookup advantage plus a small extra cost for > sorting a list with less than 10 elements (which is basically for free) > 3) We would use lookup there and then take the first iterator to the used > decoder factory, and continue iterating from there on until the decoder factory > changes. Gives us the lookup advantage and no extra costs > > What do you think? Am I missing something?
(In reply to comment #74) > (In reply to comment #73) > > Hmm, what about only sorting the GSequence by the decoder factory (and nothing > > else). > > > > 1) Creation is trivial and we get the insertion advantage there > > 2) Not so trivial, we would create a list of possible decoders in > > autoplug_select > > autoplug_select ?? or autoplug_factories autoplug_factories, sorry. 2) is autoplug_factories, 3) is autoplug_select
(In reply to comment #73) > 2) Not so trivial, we would create a list of possible decoders in > autoplug_factories and only sort this small list by our metrics (number of >common > caps features, etc). Gives us the lookup advantage plus a small extra cost for > sorting a list with less than 10 elements (which is basically for free) This is what we are doing now ! creating a small decoders GList based on the global dec-sink sequence priority.. I don't know whether we are talking about different things :)
Review of attachment 243011 [details] [review]: No, you're not doing that in your patch :) ::: gst/playback/gstplaybin2.c @@ +3449,3 @@ ave->sink = gst_object_ref (s_factory); ave->n_comm_cf = n_common_cf; + g_sequence_append (ave_seq, ave); This should use g_sequence_insert_sorted() and the compare function should only compare the factories (by pointer). Due to the way how GSequence is implemented this has no overhead over "append and sort in the end". @@ +3453,3 @@ sl = sink_list; } + g_sequence_sort (ave_seq, (GCompareDataFunc) avelement_compare, NULL); This is not necessary anymore then @@ +3486,3 @@ } + seq_iter = g_sequence_get_begin_iter (avelements); This should iterate over the (filtered!) decoder factories we get passed and use g_sequence_lookup() with the decoder factories and the compare-decoder-factory-pointer comparison function. Then go to the first iterator that has this decoder factory and iterate until the last iterator with that same decoder factory, looking for the "best" one among them, and append that avelement to a new GList. After this iteration you sort this new GList according to the important metric (number of common caps features) and then do the same as the old code. @@ +3817,3 @@ + while (!g_sequence_iter_is_end (seq_iter)) { + ave = (GstAVElement *) g_sequence_get (seq_iter); + if (ave->dec != factory) { Here you also use g_sequence_lookup() and g_sequence_iter_prev() to find the first iterator for this decoder factory. Then create a new GList with these few avelements and sort it according to the metric (number of common caps features). Then iterate over this new, very small list as before.
(In reply to comment #77) > Review of attachment 243011 [details] [review]: > > No, you're not doing that in your patch :) > > ::: gst/playback/gstplaybin2.c > @@ +3449,3 @@ > ave->sink = gst_object_ref (s_factory); > ave->n_comm_cf = n_common_cf; > + g_sequence_append (ave_seq, ave); > > This should use g_sequence_insert_sorted() and the compare function should only > compare the factories (by pointer). You mean to compare based on rank and name ??? otherwise how to insert in to correct position? Due to the way how GSequence is implemented > this has no overhead over "append and sort in the end". > > @@ +3453,3 @@ > sl = sink_list; > } > + g_sequence_sort (ave_seq, (GCompareDataFunc) avelement_compare, NULL); > > This is not necessary anymore then > > @@ +3486,3 @@ > } > GSequence ref manual is preferring to use g_sequence_sort instead of insert_sort if the list is big.
You can add your patch on top this as you wish. I think that will make the process faster :) Sorry, I already spent more time than i expected on this.
Created attachment 243486 [details] [review] 0001-playbin-Use-the-GSequence-more-efficiently.patch On top of the other two patches
commit 7ff4f8f4d48a221adf356412f8b3db8a5d021b76 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Tue May 7 14:42:05 2013 +0200 playbin: Use the GSequence more efficiently This makes it possible to take advantage of the O(log n) lookups of GSequence on the ~1000 element lists and only do iterations on <10 element lists. Previously the code iterated over ~1000 element lists multiple times. commit 52c5115ff0b1d6c2655076c93c168a930bdd0433 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu May 2 00:01:17 2013 +0300 playbin: Use GSequence instead of GList to store the GstAVElement list. The GstAVElement list might be big. Use GSequence to optimize it. commit a8d1b4549184f4aec88132b3650fa2b8fe1ca970 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Mon Apr 29 22:17:53 2013 +0300 playbin: autoplug the audio/video decoders and sinks based on capsfeatures. Autoplug the decoder elements and sink elements based on the number of common capsfeatures if the ranks are the same. This will also helps to autoplug the h/w_decoder and h/w_renderer. https://bugzilla.gnome.org/show_bug.cgi?id=698712