Xpra trac archive

Xpra: Ticket #465: improve picture buffer handling

At the moment, some picture decoders return the pixel data as a read-only string buffer (PIL does). Others return a read-write buffer just because downstream requires it (ie: OpenCL does, despite only reading from the pixel buffer..)

Tasks:


Tue, 17 Dec 2013 16:07:58 GMT - Antoine Martin: owner, status, description changed

Thu, 23 Jan 2014 11:00:01 GMT - Antoine Martin: description changed

Tue, 04 Feb 2014 12:13:01 GMT - Antoine Martin: description changed

Tue, 18 Feb 2014 11:22:21 GMT - Antoine Martin:

r5498 replaces r4963 with a much cleaner solution

Fri, 28 Feb 2014 05:38:04 GMT - Antoine Martin: attachment set

uses the new buffer API for decoding vpx via dec_vpx and tries to upload to PyOpenGL without copying

Fri, 28 Feb 2014 05:41:00 GMT - Antoine Martin:

As per this answer to my question on PyOpenGL zero copy buffer handling support, we should be able to pass a memoryview object directly for upload instead of calling clone_pixel_data in the OpenGL codepath. (see r5622 on why we copy). The patch above should do that for vpx - which I am unable to test as this requires the latest PyOpenGL and a machine with OpenGL support. More work would be needed:

etc.

Some pointers:

Sat, 05 Apr 2014 15:26:40 GMT - Antoine Martin: attachment set

updated patch using the new buffer API, works but leaks memory..

Mon, 07 Apr 2014 08:57:35 GMT - Antoine Martin: attachment set

enables memoryview direct memory copy with pyopengl for memoryviews, str and buffers (indirectly) - all leak

Mon, 07 Apr 2014 09:03:33 GMT - Antoine Martin:

Merged into a new buffer codec module in r6050, used by all decoders and CSC when "memoryview" is enabled (use --with-memoryview). As can be seen by running for 5 minutes with the patch above which forces the use of memoryviews (creating one if necessary), the memory leaks only when using pyopengl and memoryviews together... (not leaking with the plain pixbuf window backing, not leaking when we copy the pixels before use)

Good read on buffer and memoryview: Ugliness of 2.7.

Mon, 07 Apr 2014 11:15:43 GMT - Antoine Martin: attachment set

zerocopy patch for pyopengl texture upload

Mon, 07 Apr 2014 11:24:57 GMT - Antoine Martin:

Actually, it only leaks with pyopengl, but that's not where the leak is. The new code works absolutely fine, as can be seen by running with the dec_vpx decoder instead of dec_avcodec2. The problem is with ffmpeg2 frame refcounting, using pyopengl makes us free the pixel data from the UI thread later than usual and that's what seems to confuse the avframe logic.

Could be related to #457. Other tickets: the ffmpeg2 ticket was #415 and the buffer management for ffmpeg1 was #350.

As per this answer to my question to the libav mailing list, I think we need to use an approach more similar to what we did with ffmpeg1: "You need to fill AVFrame->buf[] using av_buffer_create when you allocate a frame, and that function takes a release callback which will be called once the buffer is unused and should be released. AVCodecContext->release_buffer is unused in this model."

I'm far from being the only one who is completely lost in this ffmpeg memory management mess, as can be seen with a quick google search, or directly on the Memory Leaks? - "Everything else is slightly or completely broken or works only on older FFmpeg versions. It's possible that you don't need to unref the frame on very new FFmpeg, but I haven't checked."

Mon, 07 Apr 2014 13:58:08 GMT - Antoine Martin:

New-style buffer support has been added in r6050, used in more places in r6053. It is disabled by default and requires the build time switch: --with-memoryview (and Python 2.7)

r6055 enables zerocopy memoryview based texture upload with opengl, but not for avcodec2 because of the problems discussed in comment:7

Still TODO:

Wed, 30 Apr 2014 08:32:49 GMT - Antoine Martin:

The argb module needed support for writeable buffers, added in r6248

Wed, 07 May 2014 15:38:55 GMT - Antoine Martin:

Lots more work done on this to try to improve GTK3 support (see #90), for example: r6397. This is so ugly, there has to be a better way. What sort of an upgrade is this? python2 string behaviour is more error prone when dealing with character encodings, but still way better then this awful mess..

Sat, 17 May 2014 05:26:20 GMT - Antoine Martin: milestone changed

I think this causes random crashes even with python 2.7 when I enable it, deferring it.

Thu, 11 Sep 2014 02:48:45 GMT - Antoine Martin:

Good link: An Introduction to the Python Buffer Protocol

Mon, 27 Oct 2014 19:30:43 GMT - Antoine Martin:

Also related: #717 had to disable zero copy opengl buffer upload..

Wed, 15 Apr 2015 11:14:44 GMT - Antoine Martin: attachment set

changes that may be needed for memoryview support encoding side

Wed, 15 Apr 2015 11:18:21 GMT - Antoine Martin:

Running with memoryviews seems to work OK for the client, but with the server I get screen corruption. The patch above is needed to avoid errors - nor merging it just yet because I'm not sure it is right. Here's the sort of errors you get without it:

Thu, 16 Apr 2015 08:49:51 GMT - Antoine Martin: priority changed

Fixes in r9019 (needs backporting), r9020. The only problem remaining is the xor code, r9021 adds a test for it, r9022 + r9023 + r9024 improve the code a little bit. It manifests itself as a memory corruption, and maybe it is. Raising.

Thu, 16 Apr 2015 10:18:26 GMT - Antoine Martin: priority changed

Found the first problem: we allocate a new buffer in the imagewrapper's allocate_buffer, and it frees the old one... which we are still using.

This explains why the same code works when used as client, but not as server: the server will call restride which does this naughty use-after-free. The fix is not simple unfortunately, but it is critical.

Some minor fixes and tweaks in r9026 + r9027.

I have a patch for not freeing the buffer until we are done copying it, but:

Thu, 16 Apr 2015 10:19:02 GMT - Antoine Martin: attachment set

work in progress patch to fix use-after-free

Thu, 16 Apr 2015 16:40:12 GMT - Antoine Martin: attachment set

what the corruption looks like

Thu, 16 Apr 2015 17:27:26 GMT - Antoine Martin:

For the record, the problem only occurs when building with --with-memoryview and with delta enabled (any number of buckets - see #756). To make it easier to trigger I launch the server with: 

XPRA_ENCODING_STRICT_MODE=1 XPRA_MIN_DELTA_SIZE=0 XPRA_MAX_DELTA_SIZE=6553600 \
    xpra start -d delta

And the client with: 

XPRA_DELTA_BUCKETS=1 XPRA_OPENGL_PAINT_BOX=1 \
    xpra attach --no-mmap  --encoding=rgb -d delta

The output looks like this when corruption occurs: what the corruption looks like

There is nothing of interest in the debug output:

Stumped.

Fri, 17 Apr 2015 05:16:51 GMT - Antoine Martin:

Things I have ruled out, I think, by turning it off (either with switches or modifying the code):

Some minor tweaks and improvements in r9034, r9035.

Fri, 17 Apr 2015 05:17:36 GMT - Antoine Martin: attachment set

the patch I am using for debugging

Fri, 17 Apr 2015 08:05:20 GMT - Antoine Martin:

Well, that was HARD.

Using this bit of code to print the address of the buffers we pass around: 

def pp(info, v):
    cdef unsigned char * buf = NULL              #@DuplicatedSignature
    cdef Py_ssize_t buf_len = 0                     #@DuplicatedSignature
    assert object_as_buffer(v, <const void**> &buf, &buf_len)==0, "cannot get buffer pointer for %s: %s" % (type(v), v)
    print("%s=%#x (len=%s)" % (info, <unsigned long> buf, buf_len))

I quickly found that we were sometimes xoring the same memory region.. Turns out that slicing a memoryview just gives you a new memoryview pointing to the same memory... And so when the pixels get re-allocated, we end up pointing to whatever got allocated there after it got freed! Fixed in r9039. I've also merged the ugly reallocate fix above in r9040. Both need backporting. And I still want to make this code a bit nicer: moving the restride code to ximage (the only image wrapper that needs restriding).

More minor related updates (debugging, cleanups, etc) in r9036, r9037, r9038.

Fri, 17 Apr 2015 12:26:06 GMT - Antoine Martin:

r9043 makes the xor code about 25% faster by avoiding unnecessary copying of the output (ideally we could do it in place in some cases when we own the buffer already... but the wrapper does not expose that information, yet - same issue as restriding: these should be methods on the wrapper, a bit like what PIL does with its Image class)

Sun, 19 Apr 2015 10:58:21 GMT - Antoine Martin: owner, priority, status changed

Forgot to record some partial fixes to the mmap code, wrongly committed together with sound fixes in r9018. Found some missing bits in less used codepaths: r9060 and r9062 fix those. Minimal backports for v0.14.x have been applied in r9056, r9057, r9059, r9061. There is a follow up ticket with further enhancements: #839.

Note: the v0.14.x branch is not affected by the ximage use-after-free bug because we do extra memory copies in this older version of the code. I am lowering the priority now that the corruption bugs have been fixed, and also because they would not affect our default non-gtk3 builds.

@afarr: these trunk (v0.15) changes could have a significant impact on performance. (and a negative one if I have made mistakes..) So it is worth running some benchmarks to check that trunk has not regressed (unlikely but worth checking), and also comparing trunk "normal builds" with --with-memoryviews builds. (both client and server, mix and match them too) Since the changes are very likely to affect some encodings more than others (rgb with delta, webp?, nvenc?), it may well be necessary to narrow down the benchmarks using command lines similar to the ones in comment:17 to try to identify if there really are differences between the old and new implementations.

I do not intend to make "new buffers" the default for v0.15.x (too late in the release cycle for that), but once we get some data on how well this performs, we can consider it for v0.16.x

Sun, 03 May 2015 11:46:38 GMT - Antoine Martin:

0.16 enables memoryview by default in r9223, so checking the performance is becoming more important.

Sat, 16 May 2015 14:21:08 GMT - Antoine Martin:

Since this is deep in the C / Cython layer, it is difficult to know what option was used for building the codecs, r9394 + r9395 (trunk / 0.16) expose it as buffer_api in the codec loader info, ie: with Fedora building against local libraries: 

codecs versions:
* PIL                  : 2.7.0
* avcodec2             : 56.1.100
* buffer_api           : 1
* cython               : 0.3.0.22
* dec_webp             : 0.4.2
* enc_webp             : 0.4.2
* numpy                : 1.8.2
* nvenc4               : 4.0.0
* nvenc5               : 5.0.0
* swscale              : 3.0.100
* vpx                  : 1.3.0
* x264                 : 142

And also via xpra info: 

$ xpra info | grep buffer_api
encoding.buffer_api.version=1

Note: in theory, it is still possible to build one module against one version and another module against a different version.. (for example by interrupting the build half way through, then switching to the other mode to continue.. or when fixing bugs). We only report what was used when building the argb codec (which serves as a bit of a dumping ground for utility cython functions).

Fri, 22 May 2015 16:41:43 GMT - Antoine Martin:

Some related bugs with link to the fixes: #863, #866

Thu, 28 May 2015 03:33:08 GMT - Antoine Martin:

As per ticket:863#comment:13, this got tested with a Fedora server: Building --with-memoryview and --without-memoryview has no noticeable effect in the 0.15.X branch or trunk..

Thu, 28 May 2015 03:54:04 GMT - Antoine Martin:

As per ticket:866#comment:8, the proxy server with nvenc and nvenc standalone should be tested more thoroughly.

Tue, 28 Jul 2015 06:52:03 GMT - Antoine Martin:

Note: a backport to v0.15.x caused a performance regression, see #926.

As part of this ticket, please add more performance data to wiki/CSC/Performance. Maybe this data should be part of some regression testing.

Sun, 02 Aug 2015 16:54:06 GMT - Antoine Martin: milestone changed

Note: I believe that there may well be some issues left in 0.15 that we never caught before the release, so it is a good thing that this is not enabled by default there - re-assigning to the 0.16 milestone where it is now the default with Python 2.7 and later.

Further work on buffers will continue in #935.

Fri, 07 Aug 2015 06:53:25 GMT - Antoine Martin:

Huge use after free bug fixed in r10230, caused crashes with the vpx decoder on win32 (less of a problem with avcodec because although we free the buffer, avcodec still used it - so it didn't crash)

Tue, 05 Jan 2016 06:35:30 GMT - Antoine Martin:

Support for old style buffers will be dropped in 0.18: #1073

Wed, 16 Mar 2016 07:17:43 GMT - Antoine Martin: status changed; resolution set

No problems found in 0.16 with the new buffers. Closing.

Sat, 23 Jan 2021 04:56:23 GMT - migration script:

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