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:
OpenCL
? (if possible..) see r4281 and r5255, question to the PyOpenCL
mailing list here: Should be fixed in git - which means we can make the buffer read-only again and require a newer version of pyopencl (global switch?)
r5498 replaces r4963 with a much cleaner solution
uses the new buffer API for decoding vpx via dec_vpx and tries to upload to PyOpenGL without copying
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:
clone_pixel_data
barfs that we cannot modify the view..
etc.
Some pointers:
updated patch using the new buffer API, works but leaks memory..
enables memoryview direct memory copy with pyopengl for memoryviews, str and buffers (indirectly) - all leak
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.
zerocopy patch for pyopengl texture upload
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."
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:
The argb module needed support for writeable buffers, added in r6248
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..
I think this causes random crashes even with python 2.7 when I enable it, deferring it.
Good link: An Introduction to the Python Buffer Protocol
Also related: #717 had to disable zero copy opengl buffer upload..
changes that may be needed for memoryview support encoding side
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:
error processing damage data: pixel buffer is too small: expected at least 161476 bytes but got 0 Traceback (most recent call last): File "/usr/lib64/python2.7/site-packages/xpra/server/source.py", line 1769, in encode_loop fn_and_args[0](*fn_and_args[1:]) File "/usr/lib64/python2.7/site-packages/xpra/server/window_source.py", line 1187, in make_data_packet_cb packet = self.make_data_packet(damage_time, process_damage_time, wid, image, coding, sequence, options) File "/usr/lib64/python2.7/site-packages/xpra/server/window_source.py", line 1513, in make_data_packet ret = encoder(coding, image, options) File "/usr/lib64/python2.7/site-packages/xpra/server/window_source.py", line 1584, in webp_encode return webp_encode(coding, image, self.rgb_formats, self.supports_transparency, q, s, options) File "/usr/lib64/python2.7/site-packages/xpra/server/picture_encode.py", line 62, in webp_encode cdata = enc_webp.compress(image.get_pixels(), w, h, stride=stride/4, quality=quality, speed=speed, has_alpha=alpha) File "xpra/codecs/webp/encode.pyx", line 345, in xpra.codecs.webp.encode.compress (xpra/codecs/webp/encode.c:1948) assert pic_buf_len>=c, "pixel buffer is too small: expected at least %s bytes but got %s" % (c, pic_buf_len) AssertionError: pixel buffer is too small: expected at least 161476 bytes but got 0
error processing damage data: bad argument type for built-in operation Traceback (most recent call last): File "/usr/lib64/python2.7/site-packages/xpra/server/source.py", line 1769, in encode_loop fn_and_args[0](*fn_and_args[1:]) File "/usr/lib64/python2.7/site-packages/xpra/server/window_source.py", line 1187, in make_data_packet_cb packet = self.make_data_packet(damage_time, process_damage_time, wid, image, coding, sequence, options) File "/usr/lib64/python2.7/site-packages/xpra/server/window_source.py", line 1513, in make_data_packet ret = encoder(coding, image, options) File "/usr/lib64/python2.7/site-packages/xpra/server/window_video_source.py", line 1251, in video_encode ret = self._video_encoder.compress_image(csc_image, quality, speed, options) File "xpra/codecs/nvenc4/encoder.pyx", line 1774, in xpra.codecs.nvenc4.encoder.Encoder.compress_image (xpra/codecs/nvenc4/encoder.c:19357) return self.do_compress_image(image, options) File "xpra/codecs/nvenc4/encoder.pyx", line 1821, in xpra.codecs.nvenc4.encoder.Encoder.do_compress_image (xpra/codecs/nvenc4/encoder.c:20292) self.inputBuffer.data[:len(pixels)] = pixels TypeError: bad argument type for built-in operation
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.
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:
work in progress patch to fix use-after-free
what the corruption looks like
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:
There is nothing of interest in the debug output:
delta: found empty bucket 0 delta: client options={'bucket': 0, 'lz4': 1, 'store': 2, 'rgb_format': 'BGRX'} (for region (0, 0, 511, 316))
delta: storing sequence 2 in bucket 0
Stumped.
Things I have ruled out, I think, by turning it off (either with switches or modifying the code):
rgb_reformat
the pixel order
Some minor tweaks and improvements in r9034, r9035.
the patch I am using for debugging
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.
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)
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
0.16 enables memoryview by default in r9223, so checking the performance is becoming more important.
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).
Some related bugs with link to the fixes: #863, #866
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..
As per ticket:866#comment:8, the proxy server with nvenc and nvenc standalone should be tested more thoroughly.
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.
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.
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)
Support for old style buffers will be dropped in 0.18: #1073
No problems found in 0.16 with the new buffers. Closing.
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/465