#465 closed enhancement (fixed)
improve picture buffer handling
Reported by: | Antoine Martin | Owned by: | alas |
---|---|---|---|
Priority: | major | Milestone: | 0.16 |
Component: | core | Version: | |
Keywords: | Cc: |
Description (last modified by )
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:
- Use the new buffer API
- We should use something better and allow read-write access if the underlying buffer allows it (non reference frames), or use a copy-on-write mechanism
- Try to fix
OpenCL
? (if possible..) see r4281 and r5255, question to thePyOpenCL
mailing list here: read_only images, answer 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?) - Carry a flag telling us if the buffer is read-only or not?
- Carry a flag telling us if the buffer is thread safe or not? (so we know when to free the image from the UI thread - better than r4963)
Attachments (8)
Change History (40)
comment:1 Changed 7 years ago by
Description: | modified (diff) |
---|---|
Owner: | changed from Antoine Martin to Antoine Martin |
Status: | new → assigned |
comment:2 Changed 7 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 7 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 7 years ago by
Changed 7 years ago by
Attachment: | newbuffer-vpx-opengl.patch added |
---|
uses the new buffer API for decoding vpx via dec_vpx and tries to upload to PyOpenGL without copying
comment:5 Changed 7 years ago by
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:
- the memoryview code is Python 2.7 only, so needs to be moved to a common location so we can more easily re-use it from all the codecs
- csc modules expect buffers not memory views, so again they need a more generic way of getting pointers from data
clone_pixel_data
barfs that we cannot modify the view..
etc.
Some pointers:
- The new-style Py_buffer struct
- buffers.pxd example new-buffer cython code
- python docs: memoryview and typed memoryviews
- this PyMemoryView_FromBuffer example put me on the right track
- What is the recommended way of allocating memory for a typed memory view?
- python memory allocation
- How to wrap a C pointer and length in a new-style buffer object in Cython?
Changed 7 years ago by
Attachment: | newbuffer-v2.patch added |
---|
updated patch using the new buffer API, works but leaks memory..
Changed 7 years ago by
Attachment: | newbuffer-pyopengl-leaks.patch added |
---|
enables memoryview direct memory copy with pyopengl for memoryviews, str and buffers (indirectly) - all leak
comment:6 Changed 7 years ago by
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.
Changed 7 years ago by
Attachment: | gl-memoryview-nocopy.patch added |
---|
zerocopy patch for pyopengl texture upload
comment:7 Changed 7 years ago by
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 ffmpeg mailing list: 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."
comment:8 Changed 7 years ago by
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:
- read-only: decide if we disable current versions of opencl on the clientside (not a big loss), or if we keep the buffer read-write for now
- fix dec_avcodec2 (big)
- add locking (noop when not needed) so we can re-use and free buffers without causing crashes (vpx?)
comment:9 Changed 7 years ago by
comment:10 Changed 7 years ago by
comment:11 Changed 7 years ago by
Milestone: | future → 0.14 |
---|
I think this causes random crashes even with python 2.7 when I enable it, deferring it.
comment:13 Changed 6 years ago by
Also related: #717 had to disable zero copy opengl buffer upload..
Changed 6 years ago by
Attachment: | memoryview-encode.patch added |
---|
changes that may be needed for memoryview support encoding side
comment:14 Changed 6 years ago by
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:
- with webp:
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
- with nvenc:
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
comment:15 Changed 6 years ago by
Priority: | minor → critical |
---|
comment:16 Changed 6 years ago by
Priority: | critical → blocker |
---|
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:
- it's ugly (would be better to move the restride code to image.pyx)
- it doesn't actually fix (all?) the visual corruption I am seeing..
Changed 6 years ago by
Attachment: | image-pixels-reallocate-fix.patch added |
---|
work in progress patch to fix use-after-free
comment:17 Changed 6 years ago by
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:
- server:
delta: found empty bucket 0 delta: client options={'bucket': 0, 'lz4': 1, 'store': 2, 'rgb_format': 'BGRX'} (for region (0, 0, 511, 316))
- client:
delta: storing sequence 2 in bucket 0
Stumped.
comment:18 Changed 6 years ago by
Things I have ruled out, I think, by turning it off (either with switches or modifying the code):
- lz4 / lzo / zlib compression stage
- restride stage
- verified we don't
rgb_reformat
the pixel order - buckets getting cleared
- tried copying the pixels instead of referencing the memoryview
- cyxor code: switching to the old numpy also gives visual corruption, and if anything, more reliably! (why?)
- all delta encodings are affected (not just rgb)
comment:19 Changed 6 years ago by
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.
comment:20 Changed 6 years ago by
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)
comment:21 Changed 6 years ago by
Owner: | changed from Antoine Martin to alas |
---|---|
Priority: | blocker → major |
Status: | assigned → new |
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
comment:22 Changed 6 years ago by
0.16 enables memoryview by default in r9223, so checking the performance is becoming more important.
comment:23 Changed 6 years ago by
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).
comment:25 Changed 6 years ago by
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..
comment:26 Changed 6 years ago by
As per ticket:866#comment:8, the proxy server with nvenc and nvenc standalone should be tested more thoroughly.
comment:27 Changed 6 years ago by
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.
comment:28 Changed 6 years ago by
Milestone: | 0.15 → 0.16 |
---|
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.
comment:29 Changed 6 years ago by
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)
comment:31 Changed 5 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
No problems found in 0.16 with the new buffers. Closing.
comment:32 Changed 6 weeks ago by
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/465
r5498 replaces r4963 with a much cleaner solution