GNOME Bugzilla – Bug 547603
[playbin2] add deinterlacing support
Last modified: 2010-04-29 17:08:14 UTC
Playbin2 should have deinterlacer support so Totem can enable/disable it easily.
*** Bug 515961 has been marked as a duplicate of this bug. ***
Created attachment 150332 [details] [review] Adds deinterlace to video chain After we get #605231 done, we can push this patch for always adding deinterlace to playsink and exporting its enabling/disabling through a property. This patch doesn't proxy it to playbin2, we need another one for it, should be trivial. I'll write once #605231 is solved.
Created attachment 150358 [details] [review] add enable-deinterlace property to playbin2 This also adds the property to playbin2.
Shouldn't you add another ffmpegcolorspace just before deinterlace, in case it doesn't support the color format?
I think the idea here is to make deinterlace act as a passthrough for all those cases to avoid adding a ffmpegcolorspace.
So for all unsupported color formats by deinterlace, no deinterlacing would be done? IMHO it would make more sense to add support for all common formats and for all others do conversion.
> I think the idea here is to make deinterlace act as a passthrough for all those > cases to avoid adding a ffmpegcolorspace. The reason to make deinterlace act as passthrough for all those unsupported formats is not to avoid plugging an ffmpegcolorspace in front (we still need that for the case where deinterlacing is done, as only one or two formats are supported by deinterlace), but to avoid doing two unnecessary colourspace conversions (before and after deinterlace) when no deinterlacing is needed/wanted. (I guess this depends a bit on whether the plan is to plug it conditionally or not, and whether we want to support disabling/enabling interlacing during playback, which I think we should).
hm... got it now. There will be the following cases for the caps handling: 1) video is not interlaced -> passthrough 2) video is interlaced and downstream accepts it -> passthrough 3) video is interlaced and downstream doesn't accept it 3.1) The deinterlacing of the format is supported -> deinterlace 3.2) The deinterlacing of the format is unsupported -> reject For 3.2, the upstream ffmpegcolorspace would make the conversion to a format with a supported colorspace conversion. Is this it?
(In reply to comment #8) > hm... got it now. > > There will be the following cases for the caps handling: > > 1) video is not interlaced -> passthrough > > 2) video is interlaced and downstream accepts it -> passthrough > > 3) video is interlaced and downstream doesn't accept it > > 3.1) The deinterlacing of the format is supported -> deinterlace > 3.2) The deinterlacing of the format is unsupported -> reject > > > For 3.2, the upstream ffmpegcolorspace would make the conversion to a format > with a supported colorspace *deinterlacing*. > > > Is this it? Edited something.
Does anything else need to be done here? Would be nice to get the code into GNOME 2.31 ASAP.
(In reply to comment #10) > Does anything else need to be done here? Would be nice to get the code into > GNOME 2.31 ASAP. deinterlace still needs support for more color formats before this can get commited IMHO. I should really work on this soon...
Ok, I implemented this locally a bit different by adding a new flag and putting ffmpegcolorspace before the deinterlacer and putting it right after the decoder before subtitles and scaling and all that. I'll push this after next release and after fixing some remaining issues.
Question (from someone without any playbin knowledge): When gst-plugins-cairo gets useful and we have plugins entirely in Cairo (ie decoder outputs video/x-cairo) will the deinterlacer be smart enough to handle this case? I'm asking because I'm not sure you're thinking about this potential future usecase and I had to hardcode a fix into playbin to use cairo converters instead of ffmpegcolorspace/videoscale already to get projectm working.
Well, that's the same problem as with videoscale, ffmpegcolorspace and textoverlay... and has nothing to do with this bug. We need something generic in the future to prevent hardcoding of element names everywhere. cairoscale will probably not be the perfect scaling element that can be used in all situations either. (But deinterlace might support video/x-cairo in the future, yes. But that's not a solution)
commit eec0f7c8766e55b3d9993b38df4ce2d45612938d Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Apr 26 17:30:44 2010 +0200 playsink: Add support for deinterlacing This is disabled by default and can be enabled with the deinterlace flag. Fixes bug #547603.