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 755392 - video: bugs with gst_video_frame_copy and videoconvert (with test scripts)
video: bugs with gst_video_frame_copy and videoconvert (with test scripts)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal blocker
: 1.6.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-22 08:30 UTC by Carlos Rafael Giani
Modified: 2015-09-24 21:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Tarball containing sources, scripts, and instructions (2.47 KB, application/x-compressed-tar)
2015-09-22 18:53 UTC, Carlos Rafael Giani
  Details
workaround for videoconvert regression (2.00 KB, patch)
2015-09-23 18:41 UTC, Carlos Rafael Giani
none Details | Review
video-frame: Fix gst_video_frame_copy() for formats with pstride==0 (1.69 KB, patch)
2015-09-24 09:35 UTC, Sebastian Dröge (slomo)
committed Details | Review
Patch for 10-bit RGB bayer dither mask setup (6.09 KB, patch)
2015-09-24 11:19 UTC, Carlos Rafael Giani
none Details | Review
Patch for 10-bit RGB bayer dither mask setup, v2 (5.64 KB, patch)
2015-09-24 11:25 UTC, Carlos Rafael Giani
none Details | Review
Patch for shifting bayer matrix values if needed (5.91 KB, patch)
2015-09-24 12:20 UTC, Carlos Rafael Giani
rejected Details | Review
video-dither: Use saturated add when adding ordered dither for > 8 bit targets (864 bytes, patch)
2015-09-24 16:08 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Carlos Rafael Giani 2015-09-22 08:30:57 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.
Comment 1 Carlos Rafael Giani 2015-09-22 18:51:06 UTC
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?).
Comment 2 Carlos Rafael Giani 2015-09-22 18:53:04 UTC
Created attachment 311903 [details]
Tarball containing sources, scripts, and instructions
Comment 3 Carlos Rafael Giani 2015-09-23 17:21:37 UTC
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
Comment 4 Carlos Rafael Giani 2015-09-23 18:41:09 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2015-09-23 18:48:58 UTC
Can't you just take the 16 bit dither and shift it by 6 bits?
Comment 6 Sebastian Dröge (slomo) 2015-09-24 08:06:31 UTC
On IRC Carlos said that this works, so let's go with that instead.
Comment 7 Sebastian Dröge (slomo) 2015-09-24 08:20:12 UTC
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*
Comment 8 Sebastian Dröge (slomo) 2015-09-24 09:33:30 UTC
(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.
Comment 9 Sebastian Dröge (slomo) 2015-09-24 09:35:50 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2015-09-24 09:59:00 UTC
Waiting for Carlos' patch :) Carlos, can you also confirm that my patch helps with the remaining problems you saw?
Comment 11 Carlos Rafael Giani 2015-09-24 11:19:51 UTC
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.
Comment 12 Carlos Rafael Giani 2015-09-24 11:25:26 UTC
Created attachment 312044 [details] [review]
Patch for 10-bit RGB bayer dither mask setup, v2

Removed leftover dead code
Comment 13 Sebastian Dröge (slomo) 2015-09-24 11:33:11 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2015-09-24 11:34:18 UTC
GST_VIDEO_FORMAT_INFO_DEPTH() btw
Comment 15 Carlos Rafael Giani 2015-09-24 12:20:44 UTC
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.
Comment 16 Sebastian Dröge (slomo) 2015-09-24 16:02:44 UTC
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.
Comment 17 Sebastian Dröge (slomo) 2015-09-24 16:06:45 UTC
Or just use saturated add, like video_orc_dither_ordered_u8.
Comment 18 Sebastian Dröge (slomo) 2015-09-24 16:08:05 UTC
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 19 Sebastian Dröge (slomo) 2015-09-24 21:02:53 UTC
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.
Comment 20 Sebastian Dröge (slomo) 2015-09-24 21:03:16 UTC
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