GNOME Bugzilla – Bug 724828
playbin: improve autoplug_query_caps return
Last modified: 2015-11-17 06:06:51 UTC
The following patch improve the autoplug_query_caps return. The function now returns: downstream_caps + intersect_first(filter_caps, element_caps) instead of: intersect_first(filter_caps, downstream_caps + element_caps)
Created attachment 269833 [details] [review] playbin: improve autoplug_query_caps return
Comment on attachment 269833 [details] [review] playbin: improve autoplug_query_caps return Before the if (target) part, "result" is only filtered already if have_sink is TRUE. Otherwise result contains unfiltered caps from either the subtitleoverlay factories, or from the autoplug factories.
Created attachment 270170 [details] [review] playbin: improve autoplug_query_caps return Patch updated. Caps are filtered is have_sink is FALSE.
Comment on attachment 270170 [details] [review] playbin: improve autoplug_query_caps return The return value of gst_subtitle_overlay_create_factory_caps() needs to be filtered too
(In reply to comment #4) > (From update of attachment 270170 [details] [review]) > The return value of gst_subtitle_overlay_create_factory_caps() needs to be > filtered too Unless I missed something, the return of gst_subtitle_overlay_create_factory_caps() is filtered since have_sink is FALSE in this case.
Yes you missed something :) subcaps is merged into result directly, and the !have_sink path only merges new caps into result. result is never filtered by itself, thus the subcaps are never filtered.
Created attachment 270377 [details] [review] playbin: improve autoplug_query_caps return Patch updated: * gst_subtitle_overlay_create_factory_caps () are now filtered before they are used or appended to result (btw, result is NULL before entering this code block) * when have_sink is FALSE and there is no autoplug_factories, result is now filtered * when have_sink is FALSE, autoplug_factories templates are now filtered individually before they are appended to result instead of filtering directly (result + autoplug_factories templates caps). Hope it makes more sense now.
Review of attachment 270377 [details] [review]: ::: gst/playback/gstplaybin2.c @@ +4697,3 @@ + + if (!n) + GST_PLAY_BIN_FILTER_CAPS (filter, result); This does not make much sense to me. Why are you filtering "result" here? Where does result come from and why isn't the filtering done there if necessary? Everything else looks good.
(In reply to comment #8) > Review of attachment 270377 [details] [review]: > > ::: gst/playback/gstplaybin2.c > @@ +4697,3 @@ > + > + if (!n) > + GST_PLAY_BIN_FILTER_CAPS (filter, result); > > This does not make much sense to me. Why are you filtering "result" here? Where > does result come from and why isn't the filtering done there if necessary? It was in the case result hasn't been filtered before, but according to the current code result is always filtered. This part has been removed from the next update of the patch.
Created attachment 270457 [details] [review] playbin: improve autoplug_query_caps return
commit 45dfceacdb6506a001d435c94ba160389264f3dd Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Feb 20 20:01:30 2014 +0000 playbin: improve autoplug_query_caps return Makes autoplug_query_caps return downstream_caps + intersect_first(filter_caps, element_caps) https://bugzilla.gnome.org/show_bug.cgi?id=724828
Minor nitpick regarding the commit message: it would be nice if in future you would also mention the *effect* and/or rationale/goal of the change, so people can quickly see what this is for, what it's supposed to help with, what issue this fixes.