xpra icon
Bug tracker and wiki

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#937 closed defect (fixed)

Inconsistent colours with csc cython module

Reported by: jonathan.underwood Owned by: jonathan.underwood
Priority: major Milestone: 0.16
Component: server Version: 0.15.x
Keywords: Cc:

Description

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.

Attachments (2)

ticket-937-xpra-info.txt (195.3 KB) - added by Antoine Martin 4 years ago.
converting huge inline comment to an attachment
transparent-colors.png (98.3 KB) - added by Antoine Martin 4 years ago.
what I see with the test application

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by jonathan.underwood

Component: androidserver

(data moved to the attachment)

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:2 Changed 4 years ago by jonathan.underwood

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

Changed 4 years ago by Antoine Martin

Attachment: ticket-937-xpra-info.txt added

converting huge inline comment to an attachment

comment:3 Changed 4 years ago by Antoine Martin

Owner: changed from Antoine Martin to jonathan.underwood

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)

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by jonathan.underwood

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

comment:6 Changed 4 years ago by 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?

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by jonathan.underwood

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

comment:9 Changed 4 years ago by Antoine Martin

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

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:10 Changed 4 years ago by 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.

Last edited 4 years ago by jonathan.underwood (previous) (diff)

comment:11 Changed 4 years ago by 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.

Changed 4 years ago by Antoine Martin

Attachment: transparent-colors.png added

what I see with the test application

comment:12 Changed 4 years ago by Antoine Martin

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:

what I see with the test application

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:13 Changed 4 years ago by jonathan.underwood

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

comment:14 Changed 4 years ago by 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!

comment:15 Changed 4 years ago by 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?

comment:16 Changed 4 years ago by Antoine Martin

Summary: Incosistent colours depending on encodingInconsistent colours with csc cython module

(editing bug title)

comment:17 Changed 4 years ago by 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!).

comment:18 Changed 4 years ago by 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

comment:19 Changed 4 years ago by 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.

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:20 Changed 4 years ago by Antoine Martin

Owner: changed from jonathan.underwood to Antoine Martin
Status: newassigned

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

comment:21 Changed 4 years ago by Antoine Martin

Owner: changed from Antoine Martin to jonathan.underwood
Status: assignednew

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

Last edited 4 years ago by Antoine Martin (previous) (diff)

comment:22 Changed 4 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

Assuming this is working OK now. Closing.

comment:23 Changed 4 years ago by jonathan.underwood

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

Note: See TracTickets for help on using tickets.