GNOME Bugzilla – Bug 720388
deinterlace: handle unsupported features caps in passthrough mode
Last modified: 2016-01-27 15:51:06 UTC
This patch allows the deinterlace element to disable itself and enter passthrough mode when it does not support the caps features. This is particulary useful if an hardware decoder is plugged by playbin and its output is interlaced.
Created attachment 264140 [details] [review] deinterlace: handle unsupported features caps in passthrough mode
Review of attachment 264140 [details] [review]: Please also provide a unit test for this :) ::: gst/deinterlace/gstdeinterlace.c @@ +2399,3 @@ + caps = gst_caps_new_empty_simple ("video/x-raw"); + features = + gst_caps_features_new (GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, NULL); The default is sysmem caps features. No need to do this here. @@ +2440,3 @@ + if (gst_deinterlace_can_handle_caps (caps)) { + GST_INFO_OBJECT (self, "Unsupported caps features, setting disabled mode"); + self->mode = GST_DEINTERLACE_MODE_DISABLED; This should only happen in auto-mode. Also the mode set by the property should be kept, just for these caps a different mode should internally be used.
Also in auto-mode it should do passthrough if otherwise unsupported caps are used, e.g. sysmem capsfeatures but a unsupported video format.
Created attachment 264496 [details] [review] deinterlace: handle unsupported features caps in passthrough mode Patch updated taking into account Sebastian's remarks.
Review of attachment 264496 [details] [review]: ::: gst/deinterlace/gstdeinterlace.c @@ +2147,1 @@ interlacing_mode != GST_VIDEO_INTERLACE_MODE_PROGRESSIVE)) { This seems wrong. The CAPS query result should not depend on the currently set caps here
Created attachment 265948 [details] [review] deinterlace: add fallback-to-passthrough property
(In reply to comment #5) > Review of attachment 264496 [details] [review]: > > ::: gst/deinterlace/gstdeinterlace.c > @@ +2147,1 @@ > interlacing_mode != GST_VIDEO_INTERLACE_MODE_PROGRESSIVE)) { > > This seems wrong. The CAPS query result should not depend on the currently set > caps here Then it should depend on the peer caps, right ?
Review of attachment 265948 [details] [review]: See https://bugzilla.gnome.org/show_bug.cgi?id=720345#c17 ::: gst/deinterlace/gstdeinterlace.c @@ +672,3 @@ + * If fallback-to-passthrough is enabled, deinterlace will enter + * passthrough mode if it does not support the incoming caps. This is useful + * when deinterlace is auto-inserted by playbin after an hardware decoder. Since: 1.4
What's the plan here now? Not doing it and relying on playbin only plugging deinterlace if the sink does not support it for interlaced content?
Also the CAPS/ACCEPT_CAPS handler in deinterlace is completely broken. It decides different things based on what the last configured caps on the sinkpad were. It should run completely independent of that
Created attachment 319767 [details] [review] deinterlace: Rewrite caps negotiation Previously the result of the CAPS query and ACCEPT_CAPS depended on what kind of caps were last set, and e.g. if we last had interlaced caps or not. That's just broken. Also previously the handling of non-sysmem caps features was rather random and unusuable. Now the behaviour is the following, depending on the mode property: 1) mode=disabled Completely do passthrough of everything 2) mode=interlaced Only accept formats we can actually deinterlace, and accept interlaced and progressive content and always run the deinterlacer and output progressive content 3) mode=auto (i.e. playbin) Accept all progressive formats as passthrough, accept all formats that we can deinterlace ourselves (which we do then), but also accept everything else for which we then just passthrough. In auto mode, deinterlacing is best effort: If we can, we deinterlace, if we can't we just output interlaced content. https://bugzilla.gnome.org/show_bug.cgi?id=720388 https://bugzilla.gnome.org/show_bug.cgi?id=760553
Created attachment 319769 [details] [review] deinterlace: Rewrite caps negotiation Previously the result of the CAPS query and ACCEPT_CAPS depended on what kind of caps were last set, and e.g. if we last had interlaced caps or not. That's just broken. Also previously the handling of non-sysmem caps features was rather random and unusuable. Now the behaviour is the following, depending on the mode property: 1) mode=disabled Completely do passthrough of everything 2) mode=interlaced Only accept formats we can actually deinterlace, and accept interlaced and progressive content and always run the deinterlacer and output progressive content 3) mode=auto (i.e. playbin) Accept all progressive formats as passthrough, accept all formats that we can deinterlace ourselves (which we do then), but also accept everything else for which we then just passthrough. In auto mode, deinterlacing is best effort: If we can, we deinterlace, if we can't we just output interlaced content. https://bugzilla.gnome.org/show_bug.cgi?id=720388 https://bugzilla.gnome.org/show_bug.cgi?id=760553
Created attachment 319770 [details] [review] deinterlace: Implement reconfiguration a bit better And e.g. consider reconfiguration caused by RECONFIGURE events too.
Created attachment 319771 [details] [review] deinterlace: Add mode=auto-strict In this mode we will passthrough all progressive caps but interlaced caps must be caps where we actually support deinterlacing. This is the only difference between auto and auto-strict, auto would passthrough all unsupported interlaced caps.
The patch https://bugzilla.gnome.org/attachment.cgi?id=319769 "deinterlace: Rewrite caps negotiation (23.42 KB, patch)" doesn't apply on master head because on line 545 of the patch naïvely has been transformed (or is transformed by the browser or whatever) to naïvely
It applies fine for me. It probably has to do with the UTF-8 coding. Perhaps an issue with your current locale setting or such.
commit 5d728b3ce5c63e747f3a9019304f18e2042dcc5e Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Jan 27 16:43:22 2016 +0100 deinterlace: Add negotiation unit tests for all 4 modes These now check the output caps based on the input caps and a following capsfilter and make sure the caps are exactly as expected. https://bugzilla.gnome.org/show_bug.cgi?id=760995 https://bugzilla.gnome.org/show_bug.cgi?id=720388 commit bd27a1f30b4458f2edee53c76dd07fb35904b61d Author: Vivia Nikolaidou <vivia@toolsonair.com> Date: Tue Jan 26 17:39:20 2016 +0100 deinterlace: Do passthrough in auto mode if downstream only supports interlaced If the following conditions are met: 1) upstream and downstream caps are compatible 2) upstream is interlaced 3) downstream doesn't support progressive mode then deinterlace will just do passthrough instead of failing to link. This is done with the following scenario in mind: videotestsrc ! "video/x-raw,interlace-mode=interleaved" ! deinterlace name=dein_src ! tee name=t ! queue ! deinterlace name=dein_file ! filesink t. ! queue ! deinterlace name=dein_desktop ! autovideosink In this case, dein_src will do the deinterlacing. However, videotestsrc ! "video/x-raw,interlace-mode=interleaved" ! deinterlace name=dein_src ! tee name=t ! queue ! deinterlace name=dein_file ! filesink t. ! queue ! deinterlace name=dein_desktop ! autovideosink t. ! queue ! "video/x-raw,interlace-mode=interleaved" ! fakesink In this case, caps auto-negotiation will make dein_file and dein_desktop do the deinterlacing, while dein_src will be passthrough. https://bugzilla.gnome.org/show_bug.cgi?id=760995 commit 46735f8de94d7e5a21a37fef003196bff5755fe5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 26 18:05:51 2016 +0100 deinterlace: Add mode=auto-strict In this mode we will passthrough all progressive caps but interlaced caps must be caps where we actually support deinterlacing. This is the only difference between auto and auto-strict, auto would passthrough all unsupported interlaced caps. https://bugzilla.gnome.org/show_bug.cgi?id=720388 commit 2e8d4e8c7a7c24af7301793d32b77f4ec5d2d75c Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 26 17:50:30 2016 +0100 deinterlace: Implement reconfiguration a bit better And e.g. consider reconfiguration caused by RECONFIGURE events too. https://bugzilla.gnome.org/show_bug.cgi?id=720388 commit 8c1c091439ee9c732a7c65905926d6b2216a5e5e Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 26 11:57:09 2016 +0100 deinterlace: Rewrite caps negotiation Previously the result of the CAPS query and ACCEPT_CAPS depended on what kind of caps were last set, and e.g. if we last had interlaced caps or not. That's just broken. Also previously the handling of non-sysmem caps features was rather random and unusuable. Now the behaviour is the following, depending on the mode property: 1) mode=disabled Completely do passthrough of everything 2) mode=interlaced Only accept formats we can actually deinterlace, and accept interlaced and progressive content and always run the deinterlacer and output progressive content 3) mode=auto (i.e. playbin) Accept all progressive formats as passthrough, accept all formats that we can deinterlace ourselves (which we do then), but also accept everything else for which we then just passthrough. In auto mode, deinterlacing is best effort: If we can, we deinterlace, if we can't we just output interlaced content. https://bugzilla.gnome.org/show_bug.cgi?id=720388 https://bugzilla.gnome.org/show_bug.cgi?id=760553
*** Bug 760553 has been marked as a duplicate of this bug. ***