GNOME Bugzilla – Bug 639321
deinterlace: field{1,3} scanline pointers seem to be off by one field line
Last modified: 2011-01-18 19:58:21 UTC
While working on some interlacing stuff I noticed something that looked strange in deinterlace. In one particular case (field 0 is a top field), the field 1 and field 3 scanline pointers are one field_stride down from where they should be. See the following patch. I think the improvement given by the patch can be seen using either linearblend or vfir (or weave, which is where I initially noticed it) when using one of Monty's dv samples [1]. Check the output of: gst-launch-0.10 uridecodebin uri=file:///path/to/canon-hv40-dv.dv ! deinterlace mode=1 method=vfir ! ffmpegcolorspace ! autovideosink and the same for method=linearblend before and after the patch. To my eyes, the image seems very vertically shaky before and more stable after. I think there may still be an issue (perhaps a workaround that was implemented?) in linearblend that leads to visible combing in something like every other output frame. Perhaps I will look into this shortly. If you want to step through the frames to look more closely, I was outputting to y4m by replacing autovideosink with y4menc ! filesink and then opening the files in ffplay as I'm having some issues with y4mdec at the moment. :) [1] http://people.xiph.org/~xiphmont/hv40_for_ed/canon-hv40-dv.dv
Created attachment 178148 [details] [review] deinterlace: Fix scanline pointers for fields 1 and 3 in the simple frame deinterlace methods when field 0 is top
I usually test with something like this: gst-launch videotestsrc pattern=ball num-buffers=10 ! video/x-raw-yuv,format=\(fourcc\)I420 ! interlace field-pattern=1:1 ! deinterlace method=vfir ! ffmpegcolorspace ! pngenc snapshot=false ! multifilesink location=%05d.png I see what you mean by bouncing in linearblend. However, with the patch, in the above pipeline with linearblend, it looks obviously wrong. I see no difference with vfir. This area of code is horrible. It should be rewritten, but perhaps not today.
Created attachment 178183 [details] [review] patch, rewriting bits I agree with you that there's something screwy going on with the original code. "Perhaps not today" is actually today, and I rewrote the _packed version, and it appears to work better than both the original and your patch. It has the added bonus of being easier to understand and verify.
Created attachment 178184 [details] [review] updated patch Rewrote planar code, too.
Review of attachment 178184 [details] [review]: This seems to fix all the issues I observed as well as making the code far easier to read and hence maintain. Nice. ::: gst/deinterlace/gstdeinterlacemethod.c @@ +351,3 @@ scanlines.bottom_field = (cur_field_flags == PICTURE_INTERLACED_BOTTOM); + if (!((i & 1) ^ (cur_field_flags == PICTURE_INTERLACED_BOTTOM))) { You can use scanlines.bottom_field here instead of the expression. Unless scanlines.bottom_field is not used anywhere else in which case it could be removed. @@ +454,3 @@ scanlines.bottom_field = (cur_field_flags == PICTURE_INTERLACED_BOTTOM); + if (!((i & 1) ^ (cur_field_flags == PICTURE_INTERLACED_BOTTOM))) { scanlines.bottom_field can be used here too instead of the expression.
As the release has been delayed a bit, I think this should probably be committed. And if someone has time, perhaps investigate the other modes that don't use these methods and rather use their own because it does seem to affect more than just linearblend, vfir and weave. It seems there was a known quality issue anyway: https://bugzilla.gnome.org/show_bug.cgi?id=617778
Patch thoroughly tested, deemed OK by me and pushed to master. I think greedy* and tomsmocomp need the same treatment for their scanline pointer calculations.
Comment on attachment 178148 [details] [review] deinterlace: Fix scanline pointers for fields 1 and 3 in the simple frame deinterlace methods when field 0 is top Superseded by David's patch
Marking as fixed. See 617778 for further development with greedy* and tomsmocomp as necessary.