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 571146 - mpeg2dec: possibly uses wrong strides for 4:2:2 and 4:4:4 YUV with unusual display width or height
mpeg2dec: possibly uses wrong strides for 4:2:2 and 4:4:4 YUV with unusual di...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
0.10.x
Other Linux
: Normal normal
: 0.10.17
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-02-10 08:34 UTC by Sebastian Dröge (slomo)
Modified: 2010-12-29 22:20 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2009-02-10 08:34:03 UTC
Hi,
mpeg2dec probably uses wrong strides for 4:2:2 and 4:4:4 YUV:

4:2:2:
    halfsize = mpeg2dec->decoded_width * mpeg2dec->decoded_height;
    mpeg2dec->size = halfsize * 2;
    mpeg2dec->u_offs = halfsize;
    mpeg2dec->v_offs = halfsize + (halfsize / 2);

4:4:4:
    size = mpeg2dec->decoded_width * mpeg2dec->decoded_height;

    mpeg2dec->size = size * 3;
    mpeg2dec->u_offs = size;
    mpeg2dec->v_offs = size * 2;

We usually have the strides rounded up to the next integer multiply of 4 bytes and videotestsrc/ffmpegcolorspace are doing this too. Is there any reason why the above code is still correct, like size always being an integer multiply of 4?
Comment 1 Tim-Philipp Müller 2009-02-18 15:13:33 UTC
Isn't decoded_width and decoded_height always a multiple of some blocksize and hence guaranteed to be a multiple of 4 (or rather 8 or 16)? Any reason not to make this code use the new stuff in libgstvideo?
Comment 2 Jan Schmidt 2009-03-06 23:26:08 UTC
yes, decoded_width and decoded_height should always be a multiple of 16 pixel macrocells - that does't necessarily mean it's the right stride for whatever the actual display width and height are.

I don't think this is a blocker for the release, is it? (Is it a regression in some way?) 
Comment 3 Jan Schmidt 2009-03-13 16:33:04 UTC
Removing blocker flag.
Comment 4 Tim-Philipp Müller 2010-12-27 00:34:08 UTC
> Is there any reason why the above code is still correct, like
> size always being an integer multiply of 4?

AIUI mpeg2dec will always decode into a buffer allocated for decoding_{width,height}, which is guaranteed to be a multiple of 16. In the case where the display width or height are less than decoding_width/height we create a new buffer (using new_and_alloc it seems, yuk) and crop the picture 'manually' in the mpeg2dec element by copying from the decoded buffer into the display buffer with our own strides.

In short: we need to check the crop_copy_i422_buffer() and crop_copy_i420_buffer() funcs I guess.
Comment 5 Tim-Philipp Müller 2010-12-29 22:20:29 UTC
(Tested with I420 only):

 commit 40a470c7853990d843718b2b3793bc38cdee34ce
 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
 Date:   Wed Dec 29 21:42:36 2010 +0000

    mpeg2dec: refactor cropping code to use libgstvideo functions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=571146