GNOME Bugzilla – Bug 722291
mpeg2dec: deinterlace no longer automatically deinterlaces mpeg2dec output
Last modified: 2014-04-04 10:06:53 UTC
While testing on git master vs 1.2.2, I found that deinterlace does not do it's job properly anymore, without forcing it: Auto: gst-launch-1.0 -v filesrc location=some.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! queue ! deinterlace mode=0 ! imagefreeze ! xvimagesink http://people.collabora.com/~nicolas/DeinterlaceAuto.png Forced: gst-launch-1.0 -v filesrc location=some.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! queue ! deinterlace mode=0 ! imagefreeze ! xvimagesink http://people.collabora.com/~nicolas/DeinterlaceForced.png On 1.2.2, auto case is properly deinterlace. I initially though it was related to the new passthrough code, but reverting it didn't solve the issue. This may need bisecting.
Created attachment 266386 [details] Log from 1.2 (working)
Created attachment 266387 [details] From master, Not working
Introduced by: commit 65732d9c97dd2091994c88167501e85de5e5755d Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Dec 30 10:14:09 2013 +0100 audio/video-info: Initialize the complete struct to 0 in the beginning Instead of only initializing some parts in some code paths. Also makes it easier to use the reserved bits of the structs later. https://bugzilla.gnome.org/show_bug.cgi?id=720810 The deinterlace plugin itself doesn't do any obvious misuse of video-info, still looking.
Created attachment 266541 [details] [review] Fix deinterlacing on mpeg2 output The bug was in mpeg2dec. The change I made is based on interpreting the intent of the code from the comment. It doesn't seem to have broken anything, and the size copy got introduced by the port to 0.11, so not much context to look at to get more evidence if this fix is really right.
Keeping open for now, in case someone points out this does not preserve something that should.
Comment on attachment 266541 [details] [review] Fix deinterlacing on mpeg2 output Not sure, you probably need to update the strides and plane offsets here too. At least gst_video_info_set_format() would've done that.
Created attachment 266543 [details] [review] fix deinterlacing on mpeg2 outpout Good point. Added in new patch. Also size.
Comment on attachment 266543 [details] [review] fix deinterlacing on mpeg2 outpout I think you only need a single videoinfo here for the actual format we put into buffers, and store the cropping in separate variables. And then maybe for generating caps for the non-videoinfo variant it might be necessary to create a temporary videoinfo. But then you want to preserve everything except for the width, height and need to re-calculate (NOT just copy, they might change if you change width/height) the offsets and strides.
What I'm copying here is what was just calculated from the width/height. That's why I added the _set_format again, so fill_tables is called.
Ah yes, nevermind. But still I think this could in general be simplified a bit... should work like this though, just seems quite convoluted :)
I just looked at the patch again, and I'm not sure how you'd simplify it. Since we call a function that initializes a structure, but we don't want to override all info in the existing structure, the patches uses a temp strucure and moves the info we want. What did you have in mind as a simplification ?
Comment on attachment 266543 [details] [review] fix deinterlacing on mpeg2 outpout I can't remember, and looking again at it the patch looks good to me.
Excellent :) Pushed to master and 1.2: commit 6b887060b1d6eadf5ea47a17cb9dfd62050609b7 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 17 10:46:50 2014 +0000 mpeg2dec: do not reset the whole video info when setting size New changes to gstvideo will reset all the video info state when calling _set_format, overwriting what was previously set in the preceding code. The comment says the following code is meant to preserve the pre-crop size, so let's just keep the size and related data as this does not seem to break anything else (this is what the _set_format call would have set before the change that reset all data, except the colorimetry).