[eseifert]

When the image had to be regenerated, the new buffer was stored on the producer and the lock released before the buffer was filled, and every frame copied the cached image without holding the lock. With frame threading, another thread could copy a partly filled image, or replace the cache for a different size or format and free the buffer that was being filled or copied. Keep the producer locked until the image is filled and copied.

Reproduced with ASan: 8 threads requesting frames of one color producer with alternating size and format gave heap-use-after-free on master in 2 runs; 0 errors and 0 wrong frames in 2 runs with the fix. The XML from #920 (yuv422 color track under a video, composite, real_time=-3, the video replaced by a local clip) rendered frames 2 and 3 wrong on master in 30 of 30 runs and matched the real_time=1 render in 30 of 30 runs with the fix.

Fixes #920

[bmatherly]

I see what you mean. But this change defeats any opportunity for parallelism and effectively serializes the get_image function. I wonder if we should consider a read-write lock for this situation: If the image needs to be regenerated, then all threads are blocked until the regeneration completes. But if threads only need to read, they can run in parallel.

[eseifert]

Yes, you’re right, I should have benchmarked it. I just measured it and rendering 1000 black 4K rgba frames to the null consumer takes 1.5 s on master with real_time=-8 and 2.7 s with this change, which is even slower than a single thread (2.1 s). So the lock must not cover the copy.

I see these options:

1. A read-write lock as you suggest: check and copy under a read lock; if the cache doesn't match, take the write lock, check again (a read lock can't be upgraded) and regenerate. Alternating requests for different sizes or formats, e.g. preview scaling and a second consumer, would still serialize.

2. Reference count the cached image: keep it in its own mlt_properties, and under the existing mutex only validate/regenerate it and take a reference. Copy outside the lock, then release the reference, so a replaced image is freed by its last reader. Copies never wait for each other or for a regeneration, and no new lock type is needed.

3. Drop the shared cache, except perhaps for the checkerboard, and fill each frame's buffer directly. Filling a solid color should cost about the same as copying it, so the cache may not buy much, and no lock is needed for the image.

Just shrinking the locked section probably isn't enough, because another thread can free the buffer during the copy. I’m happy to try another implementation, whichever you prefer. I’d lean towards 2 or 3 (after measuring the fill against the copy), but a read-write lock is fine with me too.

[bmatherly]

Oh boy, we could really blow the scope up on this one!

#1 - this is actually my preferred method to start with. Let's just fix the problem in front of us, test it to make sure it doesn't cause a major performance regression for the nominal use case, and then move on - or implement future optimizations in another PR.

#2 - I like this approach. But it sounds like a lot of accounting work for an infrequent occurrence. We only expect the image to be rebuilt when the user triggers a color change, or a consumer resolution change. Both user initiated actions.

#3 - I am also interested in this idea. But before we even test it, we should rewrite all the backwards while loops with modern for loops so the compiler can do its vectorization magic. Without that, the comparison would be unfair. If you are motivated and curious, you are welcome to go for this. But it isn't fair of me to ask you to do that when #1 is an option.

_Note: there is one exception to my comment above about this being an infrequent occurrence: when there certain filters ahead of this producer (composite, affine, etc), they may ask for a different resolution for every frame. In that case, #3 would be the very best option.

[ddennedy]

What is the use case that leads to this?

[eseifert]

What is the use case that leads to this?

I hit the issue when exporting from Kdenlive with "Parallel processing" enabled in the render dialog, which renders with real_time=-4 by default. I then found #920 and started digging. I checked the issue with a Kdenlive-like timeline (black background track, a gap, then a semi-transparent color clip, qtblend compositing, 1080p) and with 4 threads, 8 of 20 renders on master had wrong frames at the start and where the color clip begins, and none of 20 with this change. In the wrong frames, the bottom part of the image was missing because another thread copied it while it was still being filled: the background was transparent instead of opaque black, and in a normal yuv420p export the bottom rows of the first color clip frame were sometimes black, a one-frame flash. #920 is the same problem with composite.

[eseifert]

Oh boy, we could really blow the scope up on this one!

Yes 😁

#3 - I am also interested in this idea. But before we even test it, we should rewrite all the backwards while loops with modern for loops so the compiler can do its vectorization magic. Without that, the comparison would be unfair. If you are motivated and curious, you are welcome to go for this. But it isn't fair of me to ask you to do that when #1 is an option.

_Note: there is one exception to my comment above about this being an infrequent occurrence: when there certain filters ahead of this producer (composite, affine, etc), they may ask for a different resolution for every frame. In that case, #3 would be the very best option.

I already started implementing option 3 when I first replied, just to see if it would work and I’m also interested in what the row-copy method could gain. I’m pretty much done with the implementation and I’m working on a benchmark right now, so we can compare it with other options.

[eseifert]

Converting to draft until the final solution is pushed. Option 3 will go to a new PR draft for comparison.