Xpra: Ticket #937: Inconsistent colours with csc cython module

I am using 0.15.4 over an adsl link between two machines, running glxgears on the remote machine. I notice that changing the encoding dramatically changes the colours. You can see in the attached video the red and blue cogs completely swap colours in VP8 or VP9 compared to PNG.

Tue, 04 Aug 2015 22:09:45 GMT - jonathan.underwood: component changed

(data moved to the attachment)

Tue, 04 Aug 2015 22:10:41 GMT - jonathan.underwood:

Unfortunately the 5mb limit is preventing me from uploading a video demonstrating the problem, sadly

Wed, 05 Aug 2015 02:06:25 GMT - Antoine Martin: attachment set

converting huge inline comment to an attachment

Wed, 05 Aug 2015 02:16:06 GMT - Antoine Martin: owner changed

Most probably a duplicate of #922, which swapped the red and blue channels because that was needed on my test system with libvpx 1.4

During my testing for #922, I found that this only happened with csc cython, not with swscale - which is why I felt confident that the change was right, at least for little endian systems that I have for testing.

@jonathan.underwood: Can you test with --csc-modules=swscale (you will need to build / install ffmpeg-xpra or equivallent) and with libvpx 1.4 to see which combinations exhibit this problem? (I am short of time right now)

Wed, 05 Aug 2015 14:36:28 GMT - jonathan.underwood:

I can confirm this problem goes away when using swscale, no matter which encoding I use - I tried x264, vp9, vp8, and png, and all give the same (correct) result.

All tests have been with libvpx 1.3.0-6 as shipped with Fedora 22. I am afraid testing with libvpx 1.4 isn't going to be a small amount of work.

Wed, 05 Aug 2015 14:46:38 GMT - jonathan.underwood:

Hm. However, if I specify --csc-modules=cython (keeping the swscale and x264 codecs installed) I can't reproduce the old behaviour.

Wed, 05 Aug 2015 14:50:23 GMT - jonathan.underwood:

Ah, ok, I need to specify --csc-modules=cython on the server end, then I can reproduce the behaviour.

Aside: I wonder if this is another bug - shouldn't I be able to specify --csc-modules on the client, and it override what's used on the server?

Wed, 05 Aug 2015 15:07:26 GMT - jonathan.underwood:

Just to be clear, with --csc-modules=cython *only* the VP8 and VP9 encodings switch colours. the x264 encoding still has things right.

Wed, 05 Aug 2015 22:32:30 GMT - jonathan.underwood:

OK, reverting r9983 fixes this problem. After reverting and using csc_cython, I see consistent colours across all encodings.

Fri, 21 Aug 2015 10:34:49 GMT - Antoine Martin:

Fixed for me in r10393, tested with libvpx 1.4:

Then tested with both on client and server, looks fine in all cases.

@jonathan.underwood: does this work for you? (if so, I will backport this change to 0.15.x)

Looks to me like #922 was bogus - but I am certain I did see the colours swapped when I filed that ticket...

Fri, 21 Aug 2015 13:21:25 GMT - jonathan.underwood:

OK, with that patch, I am hitting problems with this:

xpra start :100 --csc-modules=swscale --start-child=urxvt256c
xpra --mmap=no --opengl=no --csc-module=cython attach ssh:<host>:100

Red and blue switched with VP8/9, and I see a whole load of this:

2015-08-21 14:18:44,371 do_paint_rgb32 error
Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/xpra/client/window_backing_base.py", line 307, in do_paint_rgb32
    success = (self._backing is not None) and self._do_paint_rgb32(img_data, x, y, width, height, rowstride, options)
  File "/usr/lib64/python2.7/site-packages/xpra/client/gtk2/pixmap_backing.py", line 84, in _do_paint_rgb32
    rgba = memoryview_to_bytes(self.unpremultiply(img_data))
  File "/usr/lib64/python2.7/site-packages/xpra/client/window_backing_base.py", line 158, in unpremultiply
    return unpremultiply_argb(img_data)
  File "xpra/codecs/argb/argb.pyx", line 193, in xpra.codecs.argb.argb.unpremultiply_argb (xpra/codecs/argb/argb.c:2882)
  File "xpra/codecs/argb/argb.pyx", line 229, in xpra.codecs.argb.argb.do_unpremultiply_argb (xpra/codecs/argb/argb.c:3127)
ValueError: byte must be in range(0, 256)

This is with libvpx 1.3 and Fedora 22 - I haven't yet got a test env with libvpx 1.4 working.

Fri, 21 Aug 2015 13:40:54 GMT - jonathan.underwood:

Also, if I connect without the --opengl=no, then all is fine. So, the problem seems to only manifest when opengl is being used.

Fri, 21 Aug 2015 13:54:58 GMT - Antoine Martin: attachment set

what I see with the test application

Fri, 21 Aug 2015 13:55:54 GMT - Antoine Martin:

Well, that is interesting.

what I see with the test application

Fri, 21 Aug 2015 14:06:45 GMT - jonathan.underwood:

Sorry, I should have been clearer, although I start an urxvt256, from that terminal I start glxgears for the actual test.

Fri, 21 Aug 2015 14:10:30 GMT - jonathan.underwood:

OK, I tried the transparent_colors.py test in a session where I see red and blue swapped in glxgears, and the transparent_colors.py doesn't show the problem!

Fri, 21 Aug 2015 15:53:15 GMT - Antoine Martin:

r10201 applied to v0.15.x in r10396. (not needed for 0.14.x which uses the slower bytearray code)

It took me a long time to figure out! That's because trunk was half broken (only YUV to RGB was broken, RGB to YUV was fine) whereas in the older branches (0.14 and 0.15) cython was broken in both directions... which made things work! (except with opengl enabled, or when talking to a server that uses swscale or with a trunk server...) Then with x264 you often end up using YUV444 which comes out as RGBP, and RGBP to RGB was fine too... which was confusing things even more! (for testing I forced a CSC mode using: XPRA_FORCE_CSC_MODE=YUV420P xpra start ...)

The fix for RGB is in r10397 for trunk. I don't have a big endian system to test, but r10398 is my best guess. Backported to v0.14.x and v0.15.x in r10399.

This explains why I had filed #922: I was not imagining things, yay!

OK, I tried the transparent_colors.py test in a session where I see red and blue swapped in glxgears, and the transparent_colors.py doesn't show the problem!

Makes sense: the swapping occurs in the cython converter, and we don't select video for the static transparent_colors test, so no csc step occurs. It is useful to know, because this confirms that the argb code is working OK for you too.

@jonathan.underwood: does this work for you? can I close?

Fri, 21 Aug 2015 15:54:16 GMT - Antoine Martin: summary changed

(editing bug title)

Fri, 21 Aug 2015 16:00:10 GMT - jonathan.underwood:

I'll test, but let me make sure I understand what I should be patching against 0.15.4 with - both r10396 and r10399 ? Or just r10399? (Note I haven't looked at either yet!).

Fri, 21 Aug 2015 16:53:32 GMT - jonathan.underwood:

OK, testing 0.15.4 with both r10396 and r10399 applied shows it's still broken, but differently broken :).

Now, with H264, red and blue are reversed, and the red color seems slightly different (cf PNG).

With VP8/9 the colours are the correct way round, but the tones of the blue red and green aren't the same as with PNG - particularly the red is lighter/pinker.

This is all with

xpra start :100 --csc-modules=swscale --start-child=urxvt256c
xpra --mmap=no --opengl=no --csc-module=cython attach ssh:<host>:100

Sat, 22 Aug 2015 12:42:24 GMT - Antoine Martin:

Turns out to be a very very embarrassing bug. I used to be good at math, I swear! I had swapped the U and V channels in the YUV to RGB formulae... and so I ended up having to swap the red and blue channels to make it "look" more as it should be. You mentioned that the colours looked washed out, and since I was getting nowhere finding the bug, I took a look at this raw pixel data and eventually narrowed down on the real cause of the bug. The code also gained a nifty debugging feature in the process, you can run the client and servers with:

XPRA_CSC_CYTHON_DEBUG_POINTS=150x150,10x200 xpra ...

And the csc cython step will dump both the RGB and YUV values for those points, so we can more easily see what is going on and also how much loss there is from the encoding step.

The proper fix is in r10405, will backport.

Since I was there, I also made some minor code improvements - and got about 40% better performance on my main system. (I will update wiki/CSC/Performance, still room for more improvements: we're limited to about 50fps @ 1080p, which sounds like a lot but isn't: csc should be much cheaper than this - swscale goes many times faster than this... using assembler)

@jonathan.underwood: does that work for you? trunk for now, I will deal with older branches after more testing, though I've already tested with vp8, vp9 and h264.

Sun, 23 Aug 2015 05:53:12 GMT - Antoine Martin: owner, status changed

(found some more issues - taking the ticket back until this is all fixed)

Sun, 23 Aug 2015 16:30:30 GMT - Antoine Martin: owner, status changed

So when I started on this ticket I was thinking: how hard can it be to get red and blue channels in the right order? Surely, it can't take me more than an hour... And that alone was HARD, but that was just the beginning!

One of the main difficulties with this csc code was testing that it does what it is meant to do when I had no way of verifying visually that the RGB data that came out made sense: the non-opengl pixmap backing only supported RGB and RGBX, but the cython csc module can also convert to BGR and BGRX.. So r10409 makes it possible to augment the pixmap backing with support for both BGR and BGRX, using Python Pillow to convert it back to RGB / RGBX before displaying it. Just run the client with:

    xpra attach ...

(fixups for this code in r10411 and r10414)

Next, we need to be able to force the RGB modes will come out of the csc cython module so we can actually exercise this BGR(X) code, r10412 does this by allowing us to restrict the output colorspaces using XPRA_CSC_CYTHON_%s_COLORSPACES, ie to test BGR output with YUV420P:

    xpra --csc-module=cython --opengl=no ...

(minor error logging fixup for this code in r10413)

In testing all those unusual combinations, I found that the rgb24 / rgb32 paint code was not robust enough (24 vs 32: is it the number of bits per pixel? availability of alpha channel? well, it's a bit random and unreliable!)... and so I hit another bug, now fixed in r10406. Then I got distracted by another related bug: #961. (was a regression in trunk) And then I hit another bug with vp9: #962. And I believe I have hit another bug with delta compression (#756) getting mangled with BGR(X)... (not a high priority since no-one uses those RGB pixel modes - and it might have been fixed by r10406 already)

But finally, I did test the latest trunk client's

against current 0.14.x, 0.15.x and trunk builds on a remote system via TCP (using swscale to make sure the csc step would be correct there), and all the colours were correct on the client! (no washed out colours, no colours swapped either)

Note: those tests were all done using libvpx-xpra-devel, ffmpeg-xpra-devel and x264-xpra-devel. But then I also tested a plain build (vp8 only) on Fedora 22 with libvpx 1.3:

sudo dnf install libvpx-devel
./setup.py build --with-vpx --without-enc_x264

And still no colour problems!

So r10426 applies the same change to v0.15.x, which I have tested quite thoroughly too. (r10457 for v0.14.x)

@jonathan.underwood: if you do find a problem, I will need very precise steps to reproduce it. And make sure you ./setup.py clean between builds...

Sun, 30 Aug 2015 10:08:02 GMT - Antoine Martin: status changed; resolution set

Assuming this is working OK now. Closing.

Wed, 02 Sep 2015 13:57:27 GMT - jonathan.underwood:

Yes, confirmed that this is now fixed. Apologies for the delayed response, have been slammed at work.

Sat, 23 Jan 2021 05:10:17 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/937