Avoid blocking all threads when decoding AVIF image - #9930
Conversation
0b3b624 to
3fd310f
Compare
fallenmi
left a comment
There was a problem hiding this comment.
Codex-assisted review of exact head 3fd310f710a54146d8bf2bd030a1833371815099. I found one blocking same-decoder concurrency regression; details and the reproducer results are in the inline comment. The current upstream matrix is green, but it does not exercise concurrent get_frame() calls on one decoder instance. No other findings.
| return NULL; | ||
| } | ||
|
|
||
| Py_BEGIN_ALLOW_THREADS; |
There was a problem hiding this comment.
Releasing the GIL here allows two Python threads to enter avifDecoderNthImage() concurrently on the same AvifDecoder. That routine mutates shared diagnostics, imageIndex, tile/codec state, and decoder->image; a seek/reset in one call can destroy codec state while another call is decoding. I compiled exact base e41083f383c9cd3db95de52564cc0b6452313d4a, head 3fd310f710a54146d8bf2bd030a1833371815099, and GitHub merge e93744326a73993b88b667a6c597d678bd74182a against Pillow's pinned libavif 1.4.2 with dav1d 1.5.3 and the libyuv fast path, then ran the bundled star.avifs through one shared decoder from two threads. Across ten subprocess trials, base completed 10/10 with zero mismatches, while head crashed 10/10 (8 SIGSEGV, 2 SIGBUS) and merge crashed 10/10 (8 SIGSEGV, 2 SIGBUS). Please add per-decoder serialization covering avifDecoderNthImage() through consumption/conversion of the decoder-owned image, and add a concurrency regression test. Locking only avifDecoderNthImage() is insufficient because decoder->image remains shared through avifImageYUVToRGB().
There was a problem hiding this comment.
I'm not really sure how to go about this...
I don't see any cross-platform mutex primitives being used anywhere in Pillow, there is just MUTEX_LOCK() and MUTEX_UNLOCK() macros which however are based on PyMutex which is python 3.13+ exclusive, and these macros are only defined when running a python build with GIL entirely disabled, so these cannot be used to enforce a mutex in either a python version prior to 3.13 or in a python build with GIL enabled (most builds).
Am I supposed to make my own mutex primitives and ensure they'll work on a bunch of exotic platforms?
And, I don't see this being a concern anywhere else in Pillow either: WebP decoder does the same thing, releases the GIL (ImagingSectionEnter() which runs PyEval_SaveThread()) around a WebPAnimDecoderGetNext() which uses shared context (469db51), and libImaging's _decode() does the same with ImagingSectionEnter() which just releases the GIL for all other format decoders with seemingly no mutexes or thread safety...
Apologies if I'm missing something, I'm happy to make changes if they are necessary but currently I'm more confused than anything.
There was a problem hiding this comment.
Wait, you're not even from the Pillow team?
There was a problem hiding this comment.
Wait, you're not even from the Pillow team?
Don't worry we are here watching 😄 Thank you for working on this!
There was a problem hiding this comment.
ran the bundled star.avifs through one shared decoder from two threads
Yeah, I'd say "don't do that" 😄
We could document it's not safe to share the internal decoder objects between threads, but someone who manually constructs _avif.AVIFDecoder() should already know they're playing with sharp objects...
|
@fdintino as the original author of AvifImagePlugin, would you like to give you seal of approval? |
fdintino
left a comment
There was a problem hiding this comment.
This makes sense to me, good catch.
|
Thanks. |
Allow threads during AVIF decode as it can be a relatively lengthy operation and, from what I understood of the code, other Python threads executing in the meantime shouldn't cause issues (there's just
decoder->bufferwhich is provided by Python code, but it is owned by this class for its whole lifetime, there should be no side effects from this change).I develop a Dear ImGui based app where I use a thread to decode images into raw pixels to give to OpenGL, and with 4K AVIF images it stutters to the point of being unusable while decoding is happening (from 60 FPS normally to 1-2 FPS during decode), which can be up to 5-10 seconds in a row in some cases. With this patch, everything is back to being smooth.
Passed
selfteston my machine, and I did not observe any other obvious issue using the decoder with this patch.