Strengthen calls to avcodec_decode_video2

[totaam]

Issue migrated from trac ticket # 146

component: client | priority: major | resolution: fixed

2012-06-14 15:39:11: ahuillet created the issue

---

Hello,

 - @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than
 - the actual read bytes because some optimized bitstream readers read 32 or 64
 - bits at once and could read over the end.

We don't currently do that, and this could theoretically yield segfaults.

While we're at it, the input buffer should be aligned on a 32bit boundary, and it should end with a 0.

[totaam]

2012-06-22 09:56:24: antoine uploaded file xpra-decompress-padded-nogil.patch (2.2 KiB)

copy the input buffer and pad it with zeroes, allows us to drop the gil

[totaam]

2012-06-22 09:59:31: antoine changed status from new to assigned

[totaam]

2012-06-22 09:59:31: antoine changed owner from antoine to ahuillet

[totaam]

2012-06-22 09:59:31: antoine commented

---

Please review the patch above (replacing 32 with 8... bits vs bytes, 64 vs 32!)

Looks fine to me, let's also try to get some numbers using the test script, I suspect this will improve parallelism which may be slightly detrimental to "client UI thread latency" (since this code runs in the UI thread)

Padding all pixel packets would be much more costly and impractical.

[totaam]

2012-06-22 20:53:24: antoine commented

---

We would need to use posix_memalign to ensure the memory is aligned:

 int posix_memalign (void **memptr, size_t alignment, size_t size)

What does it need to be aligned to? 32bit? alignment=4?

[totaam]

2012-06-22 21:34:34: antoine changed status from assigned to accepted

[totaam]

2012-06-22 21:34:34: antoine changed owner from ahuillet to antoine

[totaam]

2012-06-22 21:34:34: antoine commented

---

from ahuillet on irc: "align to 64 bits, align to sizeof(void *) - that's the best"

[totaam]

2012-06-25 11:00:22: antoine uploaded file gil-vs-nogil-FPS.svg (43.9 KiB)

comparing FPS before and after patch

[totaam]

2012-06-25 11:04:55: antoine uploaded file gil-vs-nogil-CPU.svg (48.4 KiB)

comparing client CPU usage before and after patch

[totaam]

2012-06-25 11:06:40: antoine changed status from accepted to closed

[totaam]

2012-06-25 11:06:40: antoine changed resolution from ** to fixed

[totaam]

2012-06-25 11:06:40: antoine commented

---

applied in r973

As can be seen here:
[[Image(https://www.xpra.org/trac/raw-attachment/ticket/146/gil-vs-nogil-FPS.svg)]]
We can push roughly the same number of frames per second, with the exception of the 'gtkperf' test (which is pathological) and 'eruption' (not sure why).

More importantly, we do not adversely affect CPU usage, which is mostly unchanged (user CPU + system CPU shown):
[[Image(https://www.xpra.org/trac/raw-attachment/ticket/146/gil-vs-nogil-CPU.svg)]]

[totaam]

2012-06-25 11:12:52: antoine uploaded file gil-vs-nogil-client-latency.svg (40.4 KiB)

comparing client UI thread latency before and after patch

[totaam]

2012-06-25 11:18:56: antoine commented

---

Also, since we don't hold the gil as much as before, the client UI thread latency is improved in most cases (except for gtkperf - which can be ignored as it creates many small and short-lived windows, and glxgears - unsure why):
[[Image(https://www.xpra.org/trac/raw-attachment/ticket/146/gil-vs-nogil-client-latency.svg)]]

[totaam]

2012-06-25 11:20:10: antoine uploaded file x264-gil-vs-nogil.csv (6.1 KiB)

sample from the test results in raw csv format

[totaam]

2012-07-04 09:17:45: antoine commented

---

Need to fix win32:

codec.obj : error LNK2019: \
    unresolved external symbol _posix_memalign referenced in function \
    ___pyx_pf_4xpra_4x264_5codec_7Decoder_6decompress_image_to_rgb

[totaam]

2012-07-04 09:17:45: antoine

[totaam]

2012-07-04 09:25:02: antoine commented

---

Here is the win32 equivallent called _aligned_malloc