xpra icon
Bug tracker and wiki

Opened 4 years ago

Closed 16 months ago

#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 Antoine Martin)

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 the PyOpenCL 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)

newbuffer-vpx-opengl.patch (4.6 KB) - added by Antoine Martin 3 years ago.
uses the new buffer API for decoding vpx via dec_vpx and tries to upload to PyOpenGL without copying
newbuffer-v2.patch​ (13.8 KB) - added by Antoine Martin 3 years ago.
updated patch using the new buffer API, works but leaks memory..
newbuffer-pyopengl-leaks.patch (4.0 KB) - added by Antoine Martin 3 years ago.
enables memoryview direct memory copy with pyopengl for memoryviews, str and buffers (indirectly) - all leak
gl-memoryview-nocopy.patch (3.5 KB) - added by Antoine Martin 3 years ago.
zerocopy patch for pyopengl texture upload
memoryview-encode.patch (5.0 KB) - added by Antoine Martin 2 years ago.
changes that may be needed for memoryview support encoding side
image-pixels-reallocate-fix.patch (3.3 KB) - added by Antoine Martin 2 years ago.
work in progress patch to fix use-after-free
xterm-xor-corruption.png (5.2 KB) - added by Antoine Martin 2 years ago.
what the corruption looks like
xor-debug.patch (11.3 KB) - added by Antoine Martin 2 years ago.
the patch I am using for debugging

Download all attachments as: .zip

Change History (39)

comment:1 Changed 4 years ago by Antoine Martin

Description: modified (diff)
Owner: changed from Antoine Martin to Antoine Martin
Status: newassigned

comment:2 Changed 3 years ago by Antoine Martin

Description: modified (diff)

comment:3 Changed 3 years ago by Antoine Martin

Description: modified (diff)

comment:4 Changed 3 years ago by Antoine Martin

r5498 replaces r4963 with a much cleaner solution

Changed 3 years ago by Antoine Martin

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 3 years ago by 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:

  • 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:

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

Changed 3 years ago by Antoine Martin

Attachment: newbuffer-v2.patch​ added

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

Changed 3 years ago by Antoine Martin

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

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

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

Changed 3 years ago by Antoine Martin

Attachment: gl-memoryview-nocopy.patch added

zerocopy patch for pyopengl texture upload

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

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

comment:8 Changed 3 years ago by 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:

  • 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 3 years ago by Antoine Martin

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

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

comment:11 Changed 3 years ago by Antoine Martin

Milestone: future0.14

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

comment:13 Changed 3 years ago by Antoine Martin

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

Changed 2 years ago by Antoine Martin

Attachment: memoryview-encode.patch added

changes that may be needed for memoryview support encoding side

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

  • 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 2 years ago by Antoine Martin

Priority: minorcritical

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.

comment:16 Changed 2 years ago by Antoine Martin

Priority: criticalblocker

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..
Last edited 2 years ago by Antoine Martin (previous) (diff)

Changed 2 years ago by Antoine Martin

work in progress patch to fix use-after-free

Changed 2 years ago by Antoine Martin

Attachment: xterm-xor-corruption.png added

what the corruption looks like

comment:17 Changed 2 years ago by 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:

  • 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 2 years ago by Antoine Martin

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)

Some minor tweaks and improvements in r9034, r9035.

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

Changed 2 years ago by Antoine Martin

Attachment: xor-debug.patch added

the patch I am using for debugging

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

comment:20 Changed 2 years ago by 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)

comment:21 Changed 2 years ago by Antoine Martin

Owner: changed from Antoine Martin to alas
Priority: blockermajor
Status: assignednew

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

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

comment:22 Changed 2 years ago by Antoine Martin

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

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

comment:23 Changed 2 years ago by 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).

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

comment:24 Changed 2 years ago by Antoine Martin

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

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

comment:26 Changed 2 years ago by Antoine Martin

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

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

comment:28 Changed 2 years ago by Antoine Martin

Milestone: 0.150.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 23 months ago by 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)

comment:30 Changed 18 months ago by Antoine Martin

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

comment:31 Changed 16 months ago by Antoine Martin

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.