Inconsistent colours with csc cython module

[totaam]

Issue migrated from trac ticket # 937

component: server | priority: major | resolution: fixed

2015-08-04 23:05:45: jonathan.underwood created the issue

---

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.

[totaam]

2015-08-04 23:09:45: jonathan.underwood changed component from android to server

[totaam]

2015-08-04 23:09:45: jonathan.underwood commented

---

(data moved to the attachment)

[totaam]

2015-08-04 23:10:41: jonathan.underwood commented

---

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

[totaam]

2015-08-05 03:06:26: antoine uploaded file ticket-937-xpra-info.txt (195.3 KiB)

converting huge inline comment to an attachment

[totaam]

2015-08-05 03:16:06: antoine changed owner from antoine to jonathan.underwood

[totaam]

2015-08-05 03:16:06: antoine commented

---

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)

[totaam]

2015-08-05 15:36:29: jonathan.underwood commented

---

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.

[totaam]

2015-08-05 15:46:38: jonathan.underwood commented

---

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

[totaam]

2015-08-05 15:50:24: jonathan.underwood commented

---

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?

[totaam]

2015-08-05 16:07:26: jonathan.underwood commented

---

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

[totaam]

2015-08-05 23:32:31: jonathan.underwood commented

---

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

[totaam]

2015-08-21 11:34:49: antoine commented

---

Fixed for me in r10393, tested with libvpx 1.4:

• cython on server only with (client uses opengl for painting yuv directly):

xpra start :10 --start-child=xterm --csc-modules=cython
xpra attach :10 --no-mmap --encoding=vp8 --opengl=yes

• cython on client only with:

xpra start :10 --start-child=xterm --csc-modules=swscale
xpra attach :10 --no-mmap --encoding=vp8 --opengl=no --csc-module=cython

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...

[totaam]

2015-08-21 14:21:25: jonathan.underwood commented

---

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.

[totaam]

2015-08-21 14:40:54: jonathan.underwood commented

---

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

[totaam]

2015-08-21 14:54:59: antoine uploaded file transparent-colors.png (98.3 KiB)

what I see with the test application

[totaam]

2015-08-21 14:55:55: antoine commented

---

Well, that is interesting.

• I've tried with Fedora 22 + urxvt256c and could not reproduce the problem!?
• the unpremultiply_argb codepath only fires with opengl disabled and with windows that use transparency, as opengl handles the premultiplied alpha natively
• the byte out of range is odd, I have a fix for that in trunk already which will need backporting: r10201, I'm just not really sure how we can end up with values out of range here, but never mind
• the red and blue swapped is more perplexing: which app did you see that on? the stacktrace uses transparency (hence the argb call) and goes via do_paint_rgb32, you should not be able to land there when using csc (none of the csc modules handle transparency) - there is a test app in our source tree and I just cannot get it to misbehave: [/browser/xpra/trunk/src/tests/xpra/test_apps/transparent_colors.py], it looks like this:
  [[Image(transparent-colors.png)]]

[totaam]

2015-08-21 15:06:46: jonathan.underwood commented

---

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

[totaam]

2015-08-21 15:10:31: jonathan.underwood commented

---

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!

[totaam]

2015-08-21 16:53:15: antoine commented

---

r10201 applied to v0.15.x in 10396. (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 10399.

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

[[BR]]

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!
[[BR]]
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?

[totaam]

2015-08-21 16:54:16: antoine changed title from Incosistent colours depending on encoding to Inconsistent colours with csc cython module

[totaam]

2015-08-21 16:54:16: antoine commented

---

(editing bug title)

[totaam]

2015-08-21 17:00:11: jonathan.underwood commented

---

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

[totaam]

2015-08-21 17:53:33: jonathan.underwood commented

---

OK, testing 0.15.4 with both 10396 and 10399 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

[totaam]

2015-08-22 13:42:25: antoine commented

---

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 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.

[totaam]

2015-08-23 06:53:12: antoine changed status from new to assigned

[totaam]

2015-08-23 06:53:12: antoine changed owner from jonathan.underwood to antoine

[totaam]

2015-08-23 06:53:12: antoine commented

---

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

[totaam]

2015-08-23 17:30:31: antoine changed status from assigned to new

[totaam]

2015-08-23 17:30:31: antoine changed owner from antoine to jonathan.underwood

[totaam]

2015-08-23 17:30:31: antoine commented

---

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_PIXMAP_INDIRECT_BGR=1 \
    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_PIXMAP_INDIRECT_BGR=1 XPRA_CSC_CYTHON_YUV420P_COLORSPACES=BGR \
    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

• csc cython module: --csc-module=cython --opengl=off
• with vp8, vp9 and h264 encodings --encoding=..
• with both the vpx decoder and avcodec2 decoders (--video-decoders=..
  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 10426 applies the same change to v0.15.x, which I have tested quite thoroughly too. (10457 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...

[totaam]

2015-08-30 11:08:02: antoine changed status from new to closed

[totaam]

2015-08-30 11:08:02: antoine set resolution to fixed

[totaam]

2015-08-30 11:08:02: antoine commented

---

Assuming this is working OK now. Closing.

[totaam]

2015-09-02 14:57:28: jonathan.underwood commented

---

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