GNOME Bugzilla – Bug 747225
video-converter: doesn't work properly with GRAY16/ARGB64/16bpp formats
Last modified: 2016-09-22 14:28:50 UTC
Using video-converter when input and output frames are GRAY16_LE (or _BE) and no scaling required, only half of the each row data is copied to destination frame.
Created attachment 300794 [details] [review] use stride for memcpy Patch attached. Please review.
Created attachment 300796 [details] [review] use slow path for both GRAY16_LE and _BE Another patch attached for slow path for gray16. Please review (also let me know if I should squash both)
Comment on attachment 300794 [details] [review] use stride for memcpy This does not seem right to me. Do you have a gst-launch pipeline or unit test that demonstrates the issue?
Perhaps I should mention non-passthrough mode. I tried with videobox (with patch attached in bug 737401) with border (any side). But, if you explicitly disable passthrough in videoconvert element's set_info, you can reproduce with following pipeline too: gst-launch-1.0 videotestsrc ! "video/x-raw,format={GRAY16_LE}" ! videoconvert ! "video/x-raw,format={GRAY16_LE}" ! videoconvert ! ximagesink In gst_video_scaler_2d, n_elems and width multiplier will be 1 for GRAY16 formats. So, for no-scaling case, bytes to memcpy will be 320 instead of 640. I'm not sure about changing the width multiplier ex: 2 for GRAY16, as it causes problems while scaling (like crash in video_scale_h_ntap_u16). In gst_video_scaler_2d no-scaling case, since src_stride = dest_stride, I used stride for memcpy. For other cases, vscale or hscale will not be NULL. Let me know if I got something wrong. Thanks.
> you can reproduce with following pipeline too: > gst-launch-1.0 videotestsrc ! "video/x-raw,format={GRAY16_LE}" ! > videoconvert ! "video/x-raw,format={GRAY16_LE}" ! videoconvert ! ximagesink I can't reproduce this with this pipeline. Do you still get the issue, with current git master?
Ravi?
If you explicitly disable passthrough mode in videoconvert element as mentioned above, you can still reproduce with that pipeline. (Or with videobox patch )
I can reproduce this after setting passthrough_on_same_caps=FALSE in class_init. The same happens with ARGB64 fwiw. Basically it doesn't take into account the bits per element or pstride.
Comment on attachment 300794 [details] [review] use stride for memcpy Using the stride does not seem right to me. It's possible that the padding is much larger than the row pixel data, we only want to copy actual pixels.
Created attachment 336075 [details] [review] other possible patch The semantics of all the parameters get_functions returns isn't very clear, but I think that's a possible patch for this. It certainly fixes this case.
I had a similar patch locally, but I think ideally the internal API should be fixed and/or documented, otherwise we just pile weird on top of what-the :)
commit 7a02e9676f8ff61e912c8a4c09b1b89d1edfc082 Author: Wim Taymans <wtaymans@redhat.com> Date: Thu Sep 22 16:15:54 2016 +0200 video-scaler: take number of bits into account when copying Copy twice the amount of pixels for 16 bits formats. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=747225