We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3857 - SDL_ConvertPixels misses YUV conversions
Summary: SDL_ConvertPixels misses YUV conversions
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: *don't know* (show other bugs)
Version: don't know
Hardware: All Linux
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-02 12:37 UTC by Sylvain
Modified: 2017-10-07 22:27 UTC (History)
1 user (show)

See Also:

Attachments
patch (51.66 KB, patch)
2017-10-02 12:37 UTC, Sylvain 		Details | Diff
test all combinations of format conversion (9.29 KB, text/x-csrc)
2017-10-02 12:38 UTC, Sylvain 		Details
test partial updatetexture (9.23 KB, text/x-csrc)
2017-10-02 12:39 UTC, Sylvain 		Details
patch warning (4.00 KB, patch)
2017-10-07 09:21 UTC, Sylvain 		Details | Diff
patch warning and yuv coefficients update (5.41 KB, patch)
2017-10-07 19:56 UTC, Sylvain 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sylvain 2017-10-02 12:37:14 UTC 
Created attachment 2964 [details]
patch

Few issues with YUV on SDL2 when using odd dimensions, and missing conversions from/back to YUV formats.

1) The big part is that SDL_ConvertPixels() does not convert to/from YUV in most cases. This now works with any format and also with odd dimensions,
  by adding two internal functions SDL_ConvertPixels_YUV_to_ARGB8888 and SDL_ConvertPixels_ARGB8888_to_YUV (could it be XRGB888 ?).
  The target format is hard coded to ARGB888 (which is the default in the internal of the software renderer).
  In case of different YUV conversion, it will do an intermediate conversion to a ARGB8888 buffer.

  SDL_ConvertPixels_YUV_to_ARGB8888 is somehow redundant with all the "Color*Dither*Mod*".
  But it allows some completeness of SDL_ConvertPixels to handle all YUV format. 
  It also works with odd dimensions.

  Moreover, I did some benchmark(SDL_ConvertPixel vs Color32DitherYV12Mod1X and Color32DitherYUY2Mod1X).
  gcc-6.3 and clang-4.0. gcc performs better than clang. And, with gcc, SDL_ConvertPixels() performs better (20%) than the two C function Color32Dither*().
  For instance, to convert 10 times a 3888x2592 image, it takes ~195 ms with SDL_ConvertPixels and ~235 ms with Color32Dither*().
  Especially because of gcc vectorize feature that optimises all conversion loops (-ftree-loop-vectorize).

  Nb: I put no image pitch for the YUV buffers. because it complexify a little bit the code and the API :
  There would be some ambiguity when setting the pitch exactly to image width:
  would it a be pitch of image width (for luma and chroma). or just contiguous data ? (could set pitch=0 for the later).


2) Small issues with odd dimensions: 
  If width "w" is odd, luma plane width is still "w" whereas chroma planes will be "(w + 1)/2". Almost the same for odd h.
  Solution is to strategically substitute "w" by "(w+1)/2" at the good places ...

- In the repository, SDL_ConvertPixels() handles YUV only if yuv source format is exactly the same as YUV destination format.
  It basically does a memcpy of pixels, but it's done incorrectly when width or height is odd (wrong size of chroma planes). This is fixed.

- SDL Renderers don't support odd width/height for YUV textures.
  This is fixed for software, opengl, opengles2. (opengles 1 does not support it and fallback to software rendering).
  This is *not* fixed for D3D and D3D11 ... (and others, psp ?)
  Only *two* Dither function are fixed ... not sure if others are really used.

- This is not possible to create a NV12/NV12 texture with the software renderer, whereas other renderers allow it. 
  This is fixed, by using SDL_ConvertPixels underneath.

- It was not possible to SDL_UpdateTexture() of format NV12/NV21 with the software renderer. this is fixed.

Here's also two testcases:
- that do all combination of conversion.
- to test partial UpdateTexture
Comment 1 Sylvain 2017-10-02 12:38:55 UTC 
Created attachment 2965 [details]
test all combinations of format conversion
Comment 2 Sylvain 2017-10-02 12:39:52 UTC 
Created attachment 2966 [details]
test partial updatetexture
Comment 3 Ozkan Sezer 2017-10-02 17:44:38 UTC 
In SDL_surface.c, all occurrences of the 'restrict' keyword needs
replacing with '__restrict'.  Otherwise the following happens:

src/video/SDL_surface.c:32:49: error: expected ';', ',' or ')' before 'src'
         Uint32 src_format, const void *restrict src, 
                                                 ^
src/video/SDL_surface.c:37:30: error: expected ';', ',' or ')' before 'src'
         const void *restrict src, int src_pitch,
                              ^
src/video/SDL_surface.c: In function 'SDL_ConvertPixels_REAL':
src/video/SDL_surface.c:1208:13: warning: implicit declaration of function 'SDL_ConvertPixels_YUV_to_ARGB8888' [-Wimplicit-function-declaration]
             SDL_ConvertPixels_YUV_to_ARGB8888(width, height, src_format, src, dst, dst_pitch);
             ^
src/video/SDL_surface.c:1233:13: warning: implicit declaration of function 'SDL_ConvertPixels_ARGB8888_to_YUV' [-Wimplicit-function-declaration]
             SDL_ConvertPixels_ARGB8888_to_YUV(width, height, src, src_pitch, dst_format, dst);
             ^
src/video/SDL_surface.c: At top level:
src/video/SDL_surface.c:1326:50: error: expected ';', ',' or ')' before 'src'
          Uint32 src_format, const void *restrict src, 
                                                  ^
src/video/SDL_surface.c:1503:90: error: expected ';', ',' or ')' before 'src'
 static int SDL_ConvertPixels_ARGB8888_to_YUV(int width, int height, const void *restrict src, int src_pitch,
                                                                                          ^

The above failure is with gcc-4.9.4 (against glibc-2.8).

Replacing restrict with __restrict makes it to build with several
gcc versions I tried including gcc-3.4.6, gcc-4.3.0, and gcc-4.9.4.
Doing so also makes it compatible with Open Watcom (1.5 and newer)
and MSVC (2005 and newer.)
Comment 4 Sylvain 2017-10-02 18:28:03 UTC 
Thanks for finding this issue !

Anyway, maybe the "restrict" can be just removed.
I added it so there is no versioned/duplication of vectorisation loop created because of possible aliasing. It wouldn't affect the code efficiency. It's only reducing size of code.
Comment 5 Sam Lantinga 2017-10-06 23:50:39 UTC 
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/696d0036f442
Comment 6 Sylvain 2017-10-07 09:21:51 UTC 
Created attachment 2968 [details]
patch warning

Here's a patch for some warnings: 
- variable unused 
- dropping "const" qualifier
Comment 7 Sylvain 2017-10-07 19:56:35 UTC 
Created attachment 2969 [details]
patch warning and yuv coefficients update

And update to the patch:
There are various YUV-RGB conversion coefficients, according to https://www.fourcc.org/fccyvrgb.php
I choose the first (from Video Demystified, with integer multiplication), 
but the current SDL2 Dither functions use in fact the next one, which follows a specifications called CCIR 601.

Here's a patch to use the second ones and with previous warning corrections. 
There are less multiplications involved because Chroma coefficient is 1.
Also, doing float multiplication is as efficient with vectorization.
In the end, the YUV decoding is faster: ~165 ms vs my previous 195 ms.

Moreover, if SDL2 is compiled with -march=native, then YUV decoding time drops to ~130ms, while older ones remains around ~220 ms.





For information, from jpeg-9 source code:
jpeg-9/jccolor.c

   * YCbCr is defined per CCIR 601-1, except that Cb and Cr are
   * normalized to the range 0..MAXJSAMPLE rather than -0.5 .. 0.5.
   * The conversion equations to be implemented are therefore
   * Y  =  0.29900 * R + 0.58700 * G + 0.11400 * B
   * Cb = -0.16874 * R - 0.33126 * G + 0.50000 * B  + CENTERJSAMPLE
   * Cr =  0.50000 * R - 0.41869 * G - 0.08131 * B  + CENTERJSAMPLE

jpeg-9/jdcolor.c

   * YCbCr is defined per CCIR 601-1, except that Cb and Cr are
   * normalized to the range 0..MAXJSAMPLE rather than -0.5 .. 0.5.
   * The conversion equations to be implemented are therefore
   *
   * R = Y                + 1.40200 * Cr
   * G = Y - 0.34414 * Cb - 0.71414 * Cr
   * B = Y + 1.77200 * Cb
Comment 8 Sylvain 2017-10-07 19:58:22 UTC 
Re-opening so it's not lost ..
Comment 9 Sam Lantinga 2017-10-07 22:27:13 UTC 
Got it, thanks!
https://hg.libsdl.org/SDL/rev/a68ad1ddb897