GNOME Bugzilla – Bug 755392
video: bugs with gst_video_frame_copy and videoconvert (with test scripts)
Last modified: 2015-09-24 21:03:16 UTC
I noticed some odd artifacts when copying NV16 frames. The first half of the columns are copied correctly, the second half is greenish. NV24 frames exhibit similar problems. Looking into this, it seems that the subsampling information in the video-format.c table for NV16 and NV24 is wrong. It reuses the subsampling data for Y42B/Y444, but does not seem to take into account the fact that there are effectively *2* bytes per pixel in the chroma plane for NV* formats, not 1. As a result, the row memcpy in gst_video_frame_copy() copies only the first half of the row: for (j = 0; j < h; j++) { memcpy (dp, sp, w); dp += ds; sp += ss; } For example, for NV16, w is width/2, even though it should equal width. I'll try to create a test element that simply calls gst_video_frame_copy() to do further tests.
Turns out that this problem is fixed in 1.5.91. However, there are others that are still broken, and there are regressions. I attached a tarball with a test plugin and test scripts to replicate this. Results from the tests: - NV16 and NV24 are broken with 1.2.3 and 1.4.5. The right half of the frame columns is greenish, which is the result of gst_video_frame_copy() copying only half of the pixels in each chroma plane row. With 1.5.91, the copy is correct. - v210, UYVP, IYU1, r210 only show a green frame with 1.2.3, 1.4.5, and 1.5.91. This is a gst_video_frame_copy() bug. - r210, GBR_10LE, GBR_10BE are correct with 1.2.3 and 1.4.5, but broken with 1.5.91. This is a videoconvert bug (a regression?).
Created attachment 311903 [details] Tarball containing sources, scripts, and instructions
Update about the videoconvert regression: the breakage happens when videoconvert converts to r210/GBR_10LE/GBR_10BE and bayer dithering is enabled. Also, these three formats are all about 4:4:4 RGB with 10 bit channels, explaining why the frames are broken in exactly the same way. Example pipeline: gst-launch-1.0 videotestsrc ! "video/x-raw, format=BGRx" ! videoconvert dither=bayer ! "video/x-raw, format=r210" ! videoconvert dither=0 ! "video/x-raw, format=BGRx" ! ximagesink
Created attachment 311974 [details] [review] workaround for videoconvert regression This is a workaround that disables dithering if the dither mode is Bayer and the output format is r210, GBR_10LE, or GBR_10BE. Bayer only works properly with 8- and 16-bit channels, and adding 10-bit support seems nontrivial.
Can't you just take the 16 bit dither and shift it by 6 bits?
On IRC Carlos said that this works, so let's go with that instead.
So to summarize (In reply to Carlos Rafael Giani from comment #1) > - NV16 and NV24 are broken with 1.2.3 and 1.4.5. The right half of the frame > columns is greenish, which is the result of gst_video_frame_copy() copying > only half of the pixels in each chroma plane row. > With 1.5.91, the copy is correct. Nothing to do here obviously > - v210, UYVP, IYU1, r210 only show a green frame with 1.2.3, 1.4.5, and > 1.5.91. This is a gst_video_frame_copy() bug. Not a regression, but if someone comes up with a patch fast enough it would be great to have it included in 1.6.0. v210 and r210 are both very complex formats, they probably should just get a special case in gst_video_frame_copy(). Same for UYVP I guess. OTOH they are all one plane, so just copying height times a stride long line should just work? IYU1 is just packed 4:1:1 YUV, so should in theory just work the same as the other packed YUV formats really. > - r210, GBR_10LE, GBR_10BE are correct with 1.2.3 and 1.4.5, but broken with > 1.5.91. This is a videoconvert bug (a regression?). Going to be fixed by shifting the dither. I would assume the same change is necessary for all our 10 bit formats, including v210, UYVP, I420_10*, I422_10*, Y444_10*, A420_10*, A422_10*, A444_10*
(In reply to Sebastian Dröge (slomo) from comment #7) > > - v210, UYVP, IYU1, r210 only show a green frame with 1.2.3, 1.4.5, and > > 1.5.91. This is a gst_video_frame_copy() bug. > > Not a regression, but if someone comes up with a patch fast enough it would > be great to have it included in 1.6.0. r210 works here, looks like the dithering bug only. v210, UYVP, IYU1 is copied with width=0 because pstride is 0 (PSTR0). Patch coming.
Created attachment 312020 [details] [review] video-frame: Fix gst_video_frame_copy() for formats with pstride==0 v210, UYVP and IYU1 are complex formats for which pixel stride does not really have a meaning. If we copy width*pstride bytes per line, it's not going to do the right thing. As a fallback, copy stride bytes per line. This might copy uninitialized bytes at the end of each line, but at least copies the frame.
Waiting for Carlos' patch :) Carlos, can you also confirm that my patch helps with the remaining problems you saw?
Created attachment 312042 [details] [review] Patch for 10-bit RGB bayer dither mask setup This patch right-shifts the values that get computed in setup_bayer() if requested. For most formats, this right shift is a no-op, but for the 3 problematic 10-bit RGB ones, it right-shifts by 6 bits, since the mask setup in this function computes 16-bit values.
Created attachment 312044 [details] [review] Patch for 10-bit RGB bayer dither mask setup, v2 Removed leftover dead code
Review of attachment 312044 [details] [review]: ::: gst-libs/gst/video/video-converter.c @@ +2178,3 @@ + case GST_VIDEO_FORMAT_r210: + case GST_VIDEO_FORMAT_GBR_10LE: + case GST_VIDEO_FORMAT_GBR_10BE: instead of hardcoding this, you can get this information from the GstVideoFormat. ::: gst-libs/gst/video/video-dither.c @@ +321,3 @@ if (shift[k] < 8) v = v >> (8 - shift[k]); + p[n_comp * j + k] = v >> additional_shift; It could just do that based on the GstVideoFormat that is passed in gst_video_dither_new(). From the format it can know if it's 16 bit, 10 bit, 8 bit or anything else.
GST_VIDEO_FORMAT_INFO_DEPTH() btw
Created attachment 312050 [details] [review] Patch for shifting bayer matrix values if needed Updated patch. It cannot use the format depth values directly, since the pack format is different from the actual output format sometimes (for example, in the r210 case). This patch introduces a target_depth value that stores the actual depth of the target frame. setup_bayer() then can compute an addition right shift out of this.
So the problem here is apparently that video_orc_dither_ordered_4u16_mask() overflows. It would like to use 32 bit integers as intermediate values during the calculations AFAIU.
Or just use saturated add, like video_orc_dither_ordered_u8.
Created attachment 312070 [details] [review] video-dither: Use saturated add when adding ordered dither for > 8 bit targets Otherwise our 16 bit integers are going to overflow in intermediate calculations, causing video to become mostly black.
Comment on attachment 312070 [details] [review] video-dither: Use saturated add when adding ordered dither for > 8 bit targets Let's get this in then, it fixes this problem and in the worst case makes something else slower but shouldn't break anything.
commit 8239da2311ddc1bcf3e1c1aa5e236ff55528c2ff Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Sep 24 18:06:58 2015 +0200 video-dither: Use saturated add when adding ordered dither for > 8 bit targets Otherwise our 16 bit integers are going to overflow in intermediate calculations, causing video to become mostly black. https://bugzilla.gnome.org/show_bug.cgi?id=755392