After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 722291 - mpeg2dec: deinterlace no longer automatically deinterlaces mpeg2dec output
mpeg2dec: deinterlace no longer automatically deinterlaces mpeg2dec output
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal blocker
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-15 20:06 UTC by Nicolas Dufresne (ndufresne)
Modified: 2014-04-04 10:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Log from 1.2 (working) (13.98 KB, text/x-log)
2014-01-15 20:21 UTC, Nicolas Dufresne (ndufresne)
  Details
From master, Not working (11.12 KB, text/x-log)
2014-01-15 20:22 UTC, Nicolas Dufresne (ndufresne)
  Details
Fix deinterlacing on mpeg2 output (1.20 KB, patch)
2014-01-17 10:55 UTC, Vincent Penquerc'h
reviewed Details | Review
fix deinterlacing on mpeg2 outpout (1.90 KB, patch)
2014-01-17 11:13 UTC, Vincent Penquerc'h
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2014-01-15 20:06:47 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.
Comment 1 Nicolas Dufresne (ndufresne) 2014-01-15 20:21:53 UTC
Created attachment 266386 [details]
Log from 1.2 (working)
Comment 2 Nicolas Dufresne (ndufresne) 2014-01-15 20:22:13 UTC
Created attachment 266387 [details]
From master, Not working
Comment 3 Vincent Penquerc'h 2014-01-17 10:08:10 UTC
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.
Comment 4 Vincent Penquerc'h 2014-01-17 10:55:30 UTC
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.
Comment 5 Vincent Penquerc'h 2014-01-17 10:57:08 UTC
Keeping open for now, in case someone points out this does not preserve something that should.
Comment 6 Sebastian Dröge (slomo) 2014-01-17 11:05:18 UTC
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.
Comment 7 Vincent Penquerc'h 2014-01-17 11:13:31 UTC
Created attachment 266543 [details] [review]
fix deinterlacing on mpeg2 outpout

Good point. Added in new patch. Also size.
Comment 8 Sebastian Dröge (slomo) 2014-01-17 11:21:18 UTC
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.
Comment 9 Vincent Penquerc'h 2014-01-17 11:26:22 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2014-01-17 12:02:06 UTC
Ah yes, nevermind. But still I think this could in general be simplified a bit... should work like this though, just seems quite convoluted :)
Comment 11 Vincent Penquerc'h 2014-04-03 13:06:43 UTC
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 12 Sebastian Dröge (slomo) 2014-04-03 18:55:30 UTC
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.
Comment 13 Vincent Penquerc'h 2014-04-04 10:06:21 UTC
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).