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 663248 - videoconvert: bad memory accesses in orc code for odd width buffers
videoconvert: bad memory accesses in orc code for odd width buffers
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal major
: NONE
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-11-02 15:42 UTC by Vincent Penquerc'h
Modified: 2015-04-16 10:55 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Vincent Penquerc'h 2011-11-02 15:42:57 UTC
I've tracked them down to the ORC versions of some of the AYUV related routines when run on buffers with odd width. Since AYUV is not subsampled, I guess the issue is with the other format converted from/to.

I'm not 100% sure, but I think these AYUV related routines process two pixels at a time, and the caller will pass (width+1)/2. There seems to be no way to convert an extra column alone, save custom code each time.

Someone who is fluent with ORC code could maybe look at cogorc_convert_UYVY_AYUV and say if I'm correct about that two pixels thing. We might then need to either convert it to single pixel at a time, or have an alternate version to use for odd widths.

Easily checked by running the pipelines/colorspace test. The crash goes with this patch (which removes the tests for these AYUV related formats (though I've not run that with valgrind, so it might be there still are more bad accesses):


diff --git a/gst/colorspace/colorspace.c b/gst/colorspace/colorspace.c
index bb8c58d..d3d804d 100644
--- a/gst/colorspace/colorspace.c
+++ b/gst/colorspace/colorspace.c
@@ -2397,7 +2397,7 @@ static const ColorspaceTransform transforms[] = {
       COLOR_SPEC_NONE, TRUE, convert_UYVY_I420},
   {GST_VIDEO_FORMAT_UYVY, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_YUY2,
       COLOR_SPEC_NONE, TRUE, convert_UYVY_YUY2},
-  {GST_VIDEO_FORMAT_UYVY, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_AYUV,
+  {GST_VIDEO_FORMAT_UYVY+88888, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_AYUV,
       COLOR_SPEC_NONE, TRUE, convert_UYVY_AYUV},
   {GST_VIDEO_FORMAT_UYVY, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_Y42B,
       COLOR_SPEC_NONE, TRUE, convert_UYVY_Y42B},
@@ -2406,11 +2406,11 @@ static const ColorspaceTransform transforms[] = {
 
   {GST_VIDEO_FORMAT_AYUV, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_I420,
       COLOR_SPEC_NONE, TRUE, convert_AYUV_I420},
-  {GST_VIDEO_FORMAT_AYUV, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_YUY2,
+  {GST_VIDEO_FORMAT_AYUV+88888, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_YUY2,
       COLOR_SPEC_NONE, TRUE, convert_AYUV_YUY2},
-  {GST_VIDEO_FORMAT_AYUV, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_UYVY,
+  {GST_VIDEO_FORMAT_AYUV+88888, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_UYVY,
       COLOR_SPEC_NONE, TRUE, convert_AYUV_UYVY},
-  {GST_VIDEO_FORMAT_AYUV, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_Y42B,
+  {GST_VIDEO_FORMAT_AYUV+8888, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_Y42B,
       COLOR_SPEC_NONE, TRUE, convert_AYUV_Y42B},
   {GST_VIDEO_FORMAT_AYUV, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_Y444,
       COLOR_SPEC_NONE, TRUE, convert_AYUV_Y444},
@@ -2421,7 +2421,7 @@ static const ColorspaceTransform transforms[] = {
       COLOR_SPEC_NONE, TRUE, convert_Y42B_YUY2},
   {GST_VIDEO_FORMAT_Y42B, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_UYVY,
       COLOR_SPEC_NONE, TRUE, convert_Y42B_UYVY},
-  {GST_VIDEO_FORMAT_Y42B, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_AYUV,
+  {GST_VIDEO_FORMAT_Y42B+8888, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_AYUV,
       COLOR_SPEC_NONE, TRUE, convert_Y42B_AYUV},
   {GST_VIDEO_FORMAT_Y42B, COLOR_SPEC_NONE, GST_VIDEO_FORMAT_Y444,
       COLOR_SPEC_NONE, TRUE, convert_Y42B_Y444},
Comment 1 Tim-Philipp Müller 2012-05-25 11:36:06 UTC
Does this also/still apply to videoconvert in git master? If yes, I would suggest we move the bug to -base. If not, I would suggest we WONTFIX it.
Comment 2 Nicolas Dufresne (ndufresne) 2012-12-16 01:54:57 UTC
I have seen green artifact with odd width in videoconvert, but haven't had time to dig into it. It's most likely related. Valgrind also report some bad memory access from ORC, but I was assuming the my version or ORC might be too old.
Comment 3 David Schleef 2013-02-20 02:33:02 UTC
At least part of this bug (UYVY->AYUV and YUY2->AYUV) is caused by:

From d69e4e7a2c4107acf9f09338c78c79306cdf79b9 Mon Sep 17 00:00:00 2001
From: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date: Thu, 24 Nov 2011 11:09:20 +0100
Subject: [PATCH 1/1] videoconvert: fix width/height mismatches

https://bugzilla.gnome.org/show_bug.cgi?id=663238
---
 gst/videoconvert/videoconvert.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

In any case, this is not a NEEDINFO.
Comment 4 Tim-Philipp Müller 2013-02-20 09:34:34 UTC
Let's move it to videoconvert then..
Comment 5 Wim Taymans 2013-09-09 10:38:21 UTC
odd width/height problems are a bit everywhere, this is the status:

 - video-format: pack/unpack functions work in all cases
 - videotestsrc: uses pack/unpack functions, works in all cases
 - videoconvert: fastpath only when it supports odd witdh/height, other paths
                 use pack/unpack functions and works correctly too

commit 9b4e2b4b36a7e27564bf31584de8741c6365ee82
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Sep 6 12:44:10 2013 +0200

    videoconvert: disable fastpath for odd width on some formats

commit fae9d82515cba6d799dc83f093a7685aadb94e9e
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Sep 6 12:43:27 2013 +0200

    video-format: fix NV24 pack/unpack function
    
    We can't reuse the NV12 functions, we need to make new ones.

commit 26d04c7582c9d01c40ce36bf99be24124e3dd344
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Sep 6 12:42:47 2013 +0200

    video-format: handle odd width in more pack/unpack functions

commit 2f6f0ee2149e83841e0e7cb8c69197e385db3d51
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Thu Sep 5 18:33:28 2013 +0100

    video-format: minor pack_YVYU optimisation
    
    Re-use already calculated line offset.

commit ede804041c5efa5c6e977fb94c43c669a57d1adc
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Thu Sep 5 17:46:03 2013 +0200

    videotestsrc: flush pending lines on odd height

commit e97f6401deb3272c03ec1fe8da0442910d5b2ecf
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Thu Sep 5 17:22:50 2013 +0200

    videoconvert: add additional width/height constraints
    
    Some of the fastpath function can only work with aligned widht/height
    so make sure we check this as well when choosing a fastpath.
    Add fastpath for I420/YV12 -> BGRx
Comment 6 Wim Taymans 2013-09-09 10:38:48 UTC
videoscale does not handle odd width correctly in many cases.
Comment 7 Wim Taymans 2015-03-09 10:50:20 UTC
please try again with git, it's all new code.
Comment 8 Vincent Penquerc'h 2015-03-09 15:06:43 UTC
This made me think of something from earlier:

https://bugzilla.gnome.org/show_bug.cgi?id=720692

Can you try this patch to see if it helps ?
Comment 9 Vincent Penquerc'h 2015-03-09 15:07:04 UTC
Gah, wrong bug, sorry.
Comment 10 Vincent Penquerc'h 2015-03-13 12:44:20 UTC
For saneish values, it seems to work fine. However, for rescaling to 1x1, 16x1, or 320x1:

** (gst-launch-1.0:7272): CRITICAL **: gst_video_resampler_init: assertion 'out_size != 0' failed

** (gst-launch-1.0:7272): CRITICAL **: resampler_zip: assertion 'r1->max_taps == r2->max_taps' failed
*** glibc detected *** gst-launch-1.0: munmap_chunk(): invalid pointer: 0x00007f7849417fd6 ***

320x2:

A stream of:
0:00:08.963817917  7307 0x7fa708037850 ERROR                default video-frame.c:147:gst_video_frame_map_id: failed to map video frame plane 2
WARNING: from element /GstPipeline:pipeline0/GstVideoScale:videoscale0: Internal GStreamer error: code not implemented.  Please file a bug at http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer.
Additional debug info:
gstvideofilter.c(292): gst_video_filter_transform (): /GstPipeline:pipeline0/GstVideoScale:videoscale0:
invalid video buffer received

At 320x3, it starts working.

The second one seems like a different issue, not sure about the memory corruption from the first one,
Comment 11 Vincent Penquerc'h 2015-04-16 10:45:57 UTC
I can't see those anymore, with pipeline:
gst-launch-1.0 uridecodebin uri=file://... ! videoscale ! video/x-raw,width=320,height=1 ! videoconvert ! fakesink

It's also valgrind clean, including for weird widths.
Comment 12 Tim-Philipp Müller 2015-04-16 10:55:58 UTC
Let's close it then. If it's still an issue with the current code, someone will run into it again soon enough and file a new bug.