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 633294 - deinterlace breaks some DVD menu scenarios
deinterlace breaks some DVD menu scenarios
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.10.26
Assigned To: Jan Schmidt
GStreamer Maintainers
: 630026 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-10-27 21:30 UTC by Jan Schmidt
Modified: 2010-12-11 21:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix for deinterlace handling of static DVD video frames. (1.22 KB, patch)
2010-10-27 21:32 UTC, Jan Schmidt
none Details | Review
Refactor chain function (3.17 KB, patch)
2010-11-05 16:13 UTC, Robert Swain
committed Details | Review
Implement field history flushing (8.25 KB, patch)
2010-11-05 16:14 UTC, Robert Swain
none Details | Review
Implement field history flushing (10.49 KB, patch)
2010-11-11 17:02 UTC, Robert Swain
committed Details | Review
deinterlace:5 debug log (76.83 KB, text/plain)
2010-11-12 11:17 UTC, Tim-Philipp Müller
  Details
deinterlace: Flush QoS and history before applying segment (1.42 KB, patch)
2010-11-12 13:01 UTC, Jan Schmidt
committed Details | Review

Description Jan Schmidt 2010-10-27 21:30:34 UTC
In particular, the deinterlacer makes some menus with static single-frame displays not work, because it doesn't push out the last frame.

Note, these are not 'still frame' menus - those have no audio, and a single static frame, and deinterlace receives a 'still-frame' event preceding those. In this case, there is audio playing, but the video stream becomes sparse - video stops arriving, and new-segment updates arrive instead.

Patch coming.
Comment 1 Jan Schmidt 2010-10-27 21:32:34 UTC
Created attachment 173358 [details] [review]
fix for deinterlace handling of static DVD video frames.
Comment 2 Sebastian Dröge (slomo) 2010-10-28 13:00:38 UTC
I don't think this patch is correct. The last_buffer always contains the last buffer, even if you don't have a special DVD stream. So whenever you receive a newsegment event and push that frame downstream, you're producing one more frame than the original source contained, e.g. when looping over a source stream.
Comment 3 Jan Schmidt 2010-10-28 22:13:51 UTC
Then explain to me where the last frame went, mister. ;)

Without this patch, the (for example) Back To The Future chapters menu displays a white frame - which is the last frame of the preceding transition movie before arriving at the single video frame that constitutes the menu.

Pushing the last buffer makes it work... so, either the correct final frame isn't being pushed, or it's not being rendered for another reason (like timestamps going backward, or being discarded for QoS?)

Also, if this isn't correct - then the still frame event handling isn't either, because it does the same thing.

Another thing that occurs to me - perhaps there's a discont buffer being pushed here, in which case the deinterlacer might not be pushing it because it doesn't have enough history.

It may be obvious how little I've actually read the deinterlacer frame handling logic.
Comment 4 Sebastian Dröge (slomo) 2010-10-29 06:25:45 UTC
My guess is, that this frame is not pushed downstream because the deinterlacer needs some fields before it can output anything, for the default method (iirc) 3 fields, i.e. 2 frames. And this is obviously creating problems for sparse video streams :)

I don't know what the correct fix would be and there's a similar problem for the last frame of every stream, which is not pushed downstream because more data is needed to deinterlace it. Any ideas? :) Don't deinterlace them at all?
Comment 5 Jan Schmidt 2010-10-29 06:45:06 UTC
I think there are 3 options, and which one probably depends on the deinterlacer algorithm:
  1) don't deinterlace them at all
  2) implement a degenerate deinterlacer mode in each algorithm for these cases
  3) for adaptive deinterlacers, maybe deinterlacing by duplicating fields to produce a 'full' set would produce acceptable output (might be best implemented as the 'degenerate' handling in option 2), inside each deinterlacer algo.

Maybe other people have better ideas too.

When I get time, I'll try and confirm that it's disconts causing the problem - I now suspect it's the most likely reason.
Comment 6 Tim-Philipp Müller 2010-10-31 15:15:00 UTC
Any news here? This is not a regression, right?
Comment 7 Jan Schmidt 2010-11-01 00:56:57 UTC
Well, it worked before playbin2 started inserting deinterlace - so I say yes.
Comment 8 Robert Swain 2010-11-02 11:19:14 UTC
I'm working on a solution for this. I need to check if any methods require past fields to be able to do their job such that the first fields going into deinterlace never get output. If I find that to be the case, I'll have to fix that too. Regardless, we need to deal with flushing out the field history in some sane and useful way.

When we need to flush out the history and push frames downstream, I propose that we try to do the best we can with what we have in the field history.

- Use the chosen method until either the field history is empty or more fields are required.
- If there are >= 2 fields remaining in the history, use vfir until either the field history is empty or more fields are required.
- If there is 1 field remaining, use linear.

The vfir method is supposed to be better quality than linearblend and both in turn are better than linear. I think this hierarchy will provide the best output we can give with what we have.

Sparse streams may have a newsegment event, then the stream's buffers which may just be one buffer or may be more. The problem lies at the end of that segment 'in the gap' so to speak. There will be a newsegment event for the gap that has update=true and this event will come directly after the last segment. If we flush the field history on a newsegment, this should push out all the frames we can from the field history and so should solve Jan's problem.

Comments welcome.
Comment 9 Robert Swain 2010-11-05 10:05:32 UTC
I've implemented the above in a field analysis/telecine branch on which I am working. With vanilla master and my branch, I can't reproduce the issue apart from one time with a sample obtained from Jan.

With current releases available in Fedora 14, I can reproduce easily when using any deinterlace method with fields_required > 1 (i.e. the default greedyh needs 4, vfir needs 2, but not linear as that needs only 1).

After some discussion and me thinking it was a QoS issue, Jan suggested that perhaps the MPEG decoder could re-send the last buffer with some 'no qos on this buffer' flag. Unfortunately we're out of buffer flags but perhaps we could send it with discont and make the sinks ignore QoS for discont buffers or something horrible like that. :)

However, I have tested using xvimagesink with the F14 packages with qos=false and I still see the issue, so it's not a QoS problem. I do think it's very strange that I can't reproduce it with vanilla git master of everything though while Jan can every time.

As this is no longer something I need to implement for my current task, I'm inclined to down-prioritise it for my personal hacking right now and revisit it once I've got some other things sorted. Unless someone can figure out what the issue it so I can hack up a fix (unless they beat me to it :)).
Comment 10 Jan Schmidt 2010-11-05 12:29:55 UTC
Patches or it didn't happen, dude
Comment 11 Robert Swain 2010-11-05 16:13:50 UTC
Created attachment 173890 [details] [review]
Refactor chain function

This is to aid field history flushing in the next patch.
Comment 12 Robert Swain 2010-11-05 16:14:16 UTC
Created attachment 173891 [details] [review]
Implement field history flushing
Comment 13 Robert Swain 2010-11-05 16:17:56 UTC
I just remembered that I need to check if the user chosen method uses only one field before blindly using linear. And perhaps I need to consider changes in dimensions with the different methods. I'll do that on Monday.

Comments welcome.
Comment 14 David Schleef 2010-11-08 18:41:51 UTC
(In reply to comment #9)
> Unfortunately we're out of buffer flags

We can steal one from GstMiniObject.  This ABI is rapidly approaching obsolescence anyway.
Comment 15 Jan Schmidt 2010-11-09 04:06:22 UTC
With both patches attached, my test DVD (Back to the Future) still has trouble on the scenes menu. method=linear works OK, however:

gst-launch rsndvdbin device=/path/to/disc ! ffmpegcolorspace ! deinterlace method=linear ! queue ! dvdspu ! xvimagesink rsndvdbin0. ! queue max-size-buffers=1 ! dvdspu0.subpicture

works. Removing method=linear fails.
Comment 16 Robert Swain 2010-11-09 07:32:08 UTC
Hmm. Before the patches, using the default method (greedyh) there should be two fields left in the field history at the end. After the patches you should see one extra buffer being flushed out using the vfir method, perhaps two if it vfir doesn't consume two fields in which case the last field should be deinterlaced using linear.

Do you at least see that additional buffer being flushed out of deinterlace after the patches have been applied? (deinterlace:5 and look for "Flush" in the output.) If so, I guess you need to find out what is happening to that buffer as unfortunately and as mentioned, I cannot reproduce the issue.
Comment 17 Robert Swain 2010-11-11 17:02:15 UTC
Created attachment 174267 [details] [review]
Implement field history flushing

Changed the code to prefer the user-chosen method and only fallback if the user-chosen method requires more fields than are available.

Also, in setcaps, the field history must be flushed before any new caps are applied, not after.
Comment 18 Robert Swain 2010-11-11 17:03:44 UTC
I don't think there are any methods that output half-height frames that require >1 field of history so I just added a FIXME note about that for now as I have other priorities. If this is wrong, please tell me. :)
Comment 19 Tim-Philipp Müller 2010-11-12 11:06:02 UTC
I can reproduce this here as well, with a different DVD (first I tried actually). Rob's patches also don't make things work for me quite yet (haven't tried Jan's patch).

> Do you at least see that additional buffer being flushed out of deinterlace
> after the patches have been applied? (deinterlace:5 and look for "Flush" in the
> output.) If so, I guess you need to find out what is happening to that buffer
> as unfortunately and as mentioned, I cannot reproduce the issue.

$ GST_DEBUG=deinterlace:5 gst-launch rsndvdbin device=/home/tpm/Dvds/FRINGE_SEASON_1_DISC_4/ ! ffmpegcolorspace ! deinterlace ! queue ! dvdspu ! xvimagesink rsndvdbin0. ! queue max-size-buffers=1 ! dvdspu0.subpicture  2>&1 | grep -i Flush
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 0)
gstdeinterlace.c:607:gst_deinterlace_reset_history:<deinterlace0> Flushing history (count 2)
gstdeinterlace.c:980:gst_deinterlace_output_frame:<deinterlace0> Flushing field(s) using vfir method
gstdeinterlace.c:980:gst_deinterlace_output_frame:<deinterlace0> Flushing field(s) using linear method

(Sometimes I also get this, and then loads of 'flushing history (count 0)' with the occasional two 'output_frame' calls in an endless loop.)
Comment 20 Tim-Philipp Müller 2010-11-12 11:06:51 UTC
Marking as blocker since it's a regression for all practical purposes (ie. it breaks DVD playback for many cases in totem).
Comment 21 Tim-Philipp Müller 2010-11-12 11:17:24 UTC
Created attachment 174318 [details]
deinterlace:5 debug log

There also seems to be something going on with segment handling/clipping:

gstdeinterlace.c:770:gst_deinterlace_pop_history:<deinterlace0> Pop last history buffer -- current history size 1
gstdeinterlace.c:779:gst_deinterlace_pop_history:<deinterlace0> Returning buffer: 0:00:10.020000000 with duration 0:00:00.040000000 and size 622080
gstdeinterlace.c:328:gst_deinterlace_clip_buffer:<deinterlace0> Clipping buffer to the current segment: 0:00:10.020000000 -- 0:00:00.020000000
gstdeinterlace.c:330:gst_deinterlace_clip_buffer:<deinterlace0> Current segment: time segment start=0:00:10.505511111, stop=99:99:99.999999999, last_stop=0:00:10.505511111, duration=99:99:99.999999999, rate=1.000000, applied_rate=1.000000, flags=0x00, time=0:00:00.505511111, accum=0:00:00.505511111
gstdeinterlace.c:355:gst_deinterlace_clip_buffer:<deinterlace0> Buffer outside the current segment -- dropping
Comment 22 Jan Schmidt 2010-11-12 13:01:22 UTC
Created attachment 174325 [details] [review]
deinterlace: Flush QoS and history before applying segment

When handling newsegment, flush out the buffer history in the
existing segment, not the new one. Fixes playback in some DVD
cases.

Partially fixes #633294
Comment 23 Jan Schmidt 2010-11-12 13:02:25 UTC
Applied on top of Rob's 2 patches, attachment 174325 [details] [review] fixes the back to the future playback case for me.
Comment 24 Robert Swain 2010-11-12 13:55:18 UTC
Sweet! :) Good job.
Comment 25 David Schleef 2010-12-11 21:05:32 UTC
*** Bug 630026 has been marked as a duplicate of this bug. ***