GNOME Bugzilla – Bug 684251
deinterlace: Bug in logic in _output_frame causing incorrect behaviour?
Last modified: 2012-10-06 12:03:04 UTC
<superdump> ds: so thaytan has hit an issue with a back to the future DVD menu with deinterlace in playbin2 <superdump> ds: i've tried to look at the issue but some of the changes made a while ago to make it actually work properly are rather confusing <superdump> ds: specifically commit 0446787e <superdump> ds: the problematic part is: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/deinterlace/gstdeinterlace.c#n1841 <superdump> ds: when you added that check, you didn't add a comment noting what the check means and i can't figure it out without really diving back into deinterlace <superdump> ds: so... please verify that check is correct and then consider this log, ignoring timestamps: http://noraisin.net/gst/gst-deinterlace.log.gz (thaytan: please leave it up if you can) <superdump> (i say ignoring timestamps because thaytan found a bug with the field duration that was screwing that up) <superdump> ds: the problem seems to be that an interlaced frame is received (thus adding two field to the history and cur_field_idx gets incremented to 1) then the top field is processed (decrementing the cur_field_idx to 0) and then instead of also deinterlacing the bottom field, this check is hit and output_frame exits »» superdump should have put all that in a bug report <thaytan> superdump: there's still time!
Here's the output I get with current git: renderer:src segment: rate 1 format 3, start: 0:00:10.000000000, stop: 99:99:99.999999999, time: 0:00:00.000000000 base: 0:00:00.000000000 renderer:src Buffer PTS 0:00:10.000000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.000000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.060000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.040000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.100000000 duration 0:00:00.020000000 Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstPulseSinkClock renderer:src Buffer PTS 0:00:10.080000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.140000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.120000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.180000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.160000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.220000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.200000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.260000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.240000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.300000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.280000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.340000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.320000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.380000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.360000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.420000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.400000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.460000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.440000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.500000000 duration 0:00:00.020000000 renderer:src Buffer PTS 0:00:10.480000000 duration 0:00:00.020000000 WARNING: from element /GstPlayBin:playbin0/GstPlaySink:playsink/GstBin:vbin/GstAutoVideoSink:videosink/GstXvImageSink:videosink-actual-sink-xvimage: A lot of buffers are being dropped. Additional debug info: gstbasesink.c(2671): gst_base_sink_is_too_late (): /GstPlayBin:playbin0/GstPlaySink:playsink/GstBin:vbin/GstAutoVideoSink:videosink/GstXvImageSink:videosink-actual-sink-xvimage: If I delete lines 1841-1843, then the timestamps are correct. diff --git a/gst/deinterlace/gstdeinterlace.c b/gst/deinterlace/gstdeinterlace.c index bb4022e..d0f22b2 100644 --- a/gst/deinterlace/gstdeinterlace.c +++ b/gst/deinterlace/gstdeinterlace.c @@ -1838,10 +1838,6 @@ restart: if (self->cur_field_idx < 0) return ret; - if (!flushing && self->cur_field_idx < 1) { - return ret; - } - /* deinterlace bottom_field */ if ((self->field_history[self->cur_field_idx].flags == PICTURE_INTERLACED_BOTTOM && (self->fields == GST_DEINTERLACE_BF
I think that patch is correct, or at least, "converges toward the correct answer with minimal effort".
Created attachment 224637 [details] test script This script suspiciously has the same timestamp as the offending patch. I don't have a strong recollection what it does, other than to test SSIM of the various deinterlace methods, which required getting field-to-frame alignment exactly right.
(In reply to comment #2) > I think that patch is correct, or at least, "converges toward the correct > answer with minimal effort". ++ for what it's worth. The longer answer being "I can't make sense of that check. It looks wrong to me. Why would we have to keep at least one field in the history?" Or something along those lines.
commit 480b894642abf0267839c44257fe9351f987ce50 Author: Robert Swain <robert.swain@collabora.co.uk> Date: Wed Sep 19 00:39:01 2012 +0200 deinterlace: Remove incorrect logic I don't understand why these lines were added, they don't make sense to me now and both David and I agree that removing them moves closer to related logic being correct, therefore, they're being removed. I've tested a few progressive, interlaced and telecine clips and they all behave properly timestamp-wise and visually after these changes. commit a35a9315556c9a3477cbf86fc4e79fdc05b1c182 Author: Robert Swain <robert.swain@collabora.co.uk> Date: Wed Sep 19 00:17:49 2012 +0200 deinterlace: Fix field duration The frame rate fraction is correctly adjusted in the cases preceding the field duration calculation and so the factor of 2 is incorrect. Those should really be attributed to Jan but he didn't commit them and Tim urged me to commit so... :) I've tested a bit with various clips and everything seems to behave well. At least in the default cases. I tested pattern locking more before I think.