Repository navigation
perf: faster JPEG streaming (zero-copy VTK capture, 4:2:0 turbo-jpeg) - #65
Conversation
Thibault-Pelletier
left a comment
There was a problem hiding this comment.
@user27182 thank you for submitting this PR!
I added a few feedbacks on code organization so that these improvements don't leak into other code paths and target more directly turbo-jpeg.
| turbo_jpeg = None | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Remove test file.
This test is too coupled with the implementation and doesn't provide any additional information on wether the feature works or not / provides any actual speedup.
Instead, it could be interesting to have actual benchmarks in the tests with pytest-benchmark that can provide a way for developers to check the perfs before / after implementing changes (including this PR for reviewers on their local machine)
There was a problem hiding this comment.
I can remove it. The test isn't at all about performance, it's about conformance. The Pillow and turbo-jpeg encoders use different default encoding parameters, so currently on master Pillow and turbo-jpeg differ. This means that users can see subtle differences depending on which encoder is used. This test aims to ensure that turbo-jpeg's encoding matches Pillow's default.
But, like I said, I can remove it if you'd rather not assert anything as such. In fact, I can remove the entire feature (the second commit) if that's easier. I was mostly focused on Pillow performance, the turbo-jpeg stuff is a bonus, and will happily remove these changes and stick with Pillow-only changes if you prefer.
There was a problem hiding this comment.
You can remove this test, but instead you should add a proper pytest-benchmark test inside this repo (with the image encoder parameterized).
This way, the performances can be tested in a systematic way by developers when making changes to this part of the code.
The actual conformance tests should be checked by proper rendering + encoding and comparing to a baseline, but this is outside of the scope of your PR
There was a problem hiding this comment.
I don't mind adding tests, but pytest-benchmark is not currently a project dependency, and in my experience using these tools with GitHub runners is notoriously challenging due to large variations in test run times. Do you still want me to go ahead and try?
Another caveat is that, at least for your Linux CI, turbo-jpeg can't currently be tested, since CI installs the wrong (too old) version of libturbojpeg for GitHub Ubuntu runners (it installs libturbojpeg 2.1.5, but >=3.0 is needed for https://github.com/lilohuang/PyTurboJPEG), and so trying to load it generates a RuntimeError, which trame-rca then falls back to the pillow encoder.
(see pyvista/trame-pyvista#131 (comment) for details where this initially surfaced)
And so, I can add a benchmark, but on Linux at least it won't be for turbo-jpeg, it'll necessarily be for pillow. And hence the changes in question to turbo-jpeg encoding won't be visible.
The actual conformance tests should be checked by proper rendering + encoding and comparing to a baseline, but this is outside of the scope of your PR
I'm happy to try to add rendering tests, do you have an existing testing workflow for image regression testing? or is that the "outside the scope of your PR" part you're referring to?
There was a problem hiding this comment.
Yes, you can add a dev dependency for pytest-benchmark. The goal is to have benchmark tests that can be run locally by developers rather than checking the actual performances on the CI.
This way, if users report performances changes, it will be easier to bisect and use the perf test rather than manually reimplementing a new benchmark.
The benchmark should be parameterized for the available RCA images and can be marked skipped for import errors.
Rendering tests will be outside the scope of the PR.
| # Flip into a new RGBA buffer and return its RGB channels as a view: | ||
| # VTK's alpha is 0 on the background, and the encoders can read the | ||
| # 4-byte pixels in place (see encoders.img.rgbx_view) | ||
| np_image = np.ascontiguousarray(np_image[::-1])[..., :3] |
There was a problem hiding this comment.
This would be the wrong place to put this. If rgbx_view needs a contiguous array, then it can be put in the specific code and not leak here.
There was a problem hiding this comment.
I can drop this and simply do:
np_image = np_image[::-1, :, :3]Is that acceptable?
There was a problem hiding this comment.
There shouldn't be any changes to what the VTK side returns as this would break the current contract and break any other users accessing the data and expecting the current format.
If you need this change it would be better to move it inside rgbx_view instead. This way it will only afect this specific code path and not others.
On a side note, I will test your branch on real projects before accepting anything, but I'm concerned the changes in this file may be an hallucination and may flip the render window / flip the color space.
The tests in trame-rca are not remotely complete and just checking a grey cone doesn't inform if you have completely flipped what you wanted to display.
There was a problem hiding this comment.
I can move it.
I understand your concerns. This is why I don't blindly ask Claude to do things without my input and without review first. I only pushed two commits for this PR, but that's because my understanding is kitware does not typically use squash+merge, and so the commits should stand on their own, ready to be integrated as-is directly into the main branch.
And so locally my development branch initially had many commits, iterated upon by me after much back-and-forth with Claude, and then at the end I rewrote history and force-pushed just the two commits. There is lots of local benchmarking and visual testing, as well as testing with trame-pyvista downstream, that is simply not visible here. I also have specific code-review skills I run with Claude on the diff.
If you'd prefer, next time I can add commits without a force-push, and then you can squash+merge into main? What is the standard here for this repo?
There was a problem hiding this comment.
I like to have isolated commit (rather than a big squash), that way the release notes point to each step/goal and it is easier to track. But if a PR is small enough and does not tackle 2+ things at once, then it can be squashed.
For me the split you did in your commits were perfect.
There was a problem hiding this comment.
There shouldn't be any changes to what the VTK side returns as this would break the current contract and break any other users accessing the data and expecting the current format.
If you need this change it would be better to move it inside rgbx_view instead. This way it will only afect this specific code path and not others.
On a side note, I will test your branch on real projects before accepting anything, but I'm concerned the changes in this file may be an hallucination and may flip the render window / flip the color space.
The tests in trame-rca are not remotely complete and just checking a grey cone doesn't inform if you have completely flipped what you wanted to display.
I added image orientation tests as part of #66. That should help give confidence here that this PR doesn't break anything. Please review #66 first.
There was a problem hiding this comment.
Thanks @user27182, I reviewed #65 and added a small request on git history.
FYI, I tried rebasing the current PR on the other one and there will be a small conflict to address + failing tests due to a too strong coupling of the tests with the actual type of the encoded image.
You may want to relax the tests in #65 to avoid having to fix them just after in this PR.
There was a problem hiding this comment.
This would be the wrong place to put this. If rgbx_view needs a contiguous array, then it can be put in the specific code and not leak here.
rgbx_view doesn't need a contiguous array. It needs the 4-byte pixels to be in memory top row first, because that's how Pillow and libjpeg-turbo read a buffer in place. np.ascontiguousarray is just how the flip gets done, and master already flips here:
# master: copy the flipped rows back into VTK's own buffer
np_image[:] = np_image[::-1, :, :]
# this PR: copy the flipped rows into a new buffer
np_image = np.ascontiguousarray(np_image[::-1])Both make the rows top-down. This PR does it with one copy instead of two (NumPy buffers the overlapping in-place assignment through a temporary), and no longer writes into VTK's buffer.
There shouldn't be any changes to what the VTK side returns as this would break the current contract and break any other users accessing the data and expecting the current format.
Agreed that the existing contract should not be broken, and this PR does change the layout of the returned array, though not the values. To let the encoders read 4-byte pixels in place, it captures RGBA and returns the RGB channels as a view ([..., :3]). The shape, dtype, orientation and pixel values are the same as on master, but the array is no longer C-contiguous (4 bytes between pixels instead of 3).
That only matters to code that reads the array's raw memory as packed RGB, e.g. np.frombuffer(arr), arr.view(other_dtype), arr.ctypes.data, or C extensions that require contiguous input. For these cases, an explicit call to np.ascontiguousarray(arr) by user code restores the old layout. Nothing in trame-rca relies on this (the scheduler passes the array straight to the encoders), and the tests from #66 check that every encoder accepts non-contiguous layouts.
The layout change is what makes the zero-copy path possible: with packed RGB, rgbx_view always returns None and Pillow falls back to Image.fromarray, which repacks every pixel. If changing img_cols_rows's layout isn't acceptable, two options:
- Keep
img_cols_rowsexactly as on master, and add a separate method onVtkRemoteControlledAreathat returns the RGBA frame, which the scheduler would pass to the encoders when available. - Drop the zero-copy commit and keep only the 4:2:0 one. But this defeats the main purpose of the PR, which is to speed up the default Pillow encoder.
| pixel_format = TJPF_RGB | ||
| rgbx = rgbx_view(image) | ||
| if rgbx is not None: | ||
| # VtkRemoteControlledArea frame: read its RGBX buffer without a copy |
There was a problem hiding this comment.
Remove comment, uninformative
| if rgbx is not None: | ||
| # VtkRemoteControlledArea frame: read its RGBX buffer without a copy | ||
| image, pixel_format = rgbx, TJPF_RGBX | ||
| # 4:2:0 like Pillow; PyTurboJPEG's default 4:2:2 gives larger, slower frames |
There was a problem hiding this comment.
Remove comment, uninformative
| def rgbx_view(image: NDArray) -> NDArray | None: | ||
| """ | ||
| Return the RGBX buffer behind an RGB view over 4-byte pixels, or None. | ||
| VtkRemoteControlledArea returns such a view so the encoders can skip a copy. |
There was a problem hiding this comment.
I added another comment in the vtk_rca.py, but since the RGBX contiguous view is only required by this function, please move the conversion scope inside this function directly and remove this comment
There was a problem hiding this comment.
Just to confirm, you want this function moved to vtk_rca.py, is that correct?
There was a problem hiding this comment.
No, I meant that this function should be moved to the turbo_jpeg.py file.
If your change provides any performance update to the PIL code path, then it can stay here, but if not, I would rather this stays an implementation detail of the turbo_jpeg encoding to avoid any unwanted side effects in the PIL case.
There was a problem hiding this comment.
rgbx_view applies to both encoders though. For simplicity, if you prefer, I can make this PR Pillow-only, and drop all turbo-jpeg changes, and move rgbx_view into pil.py instead. That should make the implementation and tests a lot clearer in scope.
There was a problem hiding this comment.
My bad, I had missread the commit messages.
I still think the changes should be scoped to pil and turbo_jpeg encodings as any other changes to the VTK RCA img to row may break the current contract of what the method returns.
So this method can stay here, the comment still needs to be updated.
9527216 to
278e889
Compare
|
I removed the turbo-jpeg 4:2:0 and comments as per review feedback. I also removed |
|
@Thibault-Pelletier I think I'm good with the current code change? What about you? |
|
Current changes look good to me. Due to timezone diff, I'll let @user27182 rebase / adress the merge conflict and @jourdain you can merge whenever the CI is green 👍 |
Capture RGBA from vtkWindowToImageFilter and flip it into a new buffer, returning its RGB channels as a view. Pillow and PyTurboJPEG can then read the 4-byte pixels in place (Image.frombuffer "RGBX" / TJPF_RGBX) instead of Pillow copying the frame via Image.fromarray. The returned array is still a top-down RGB array, so other RCAs, the non-JPEG formats and the multiprocessing path are unchanged: VTK's alpha (0 on the background) never reaches the encoders, and output is byte-identical to before for jpeg, turbo-jpeg, png, webp and avif. The flip no longer writes back into the VTK-owned buffer.
PyTurboJPEG defaults to 4:2:2 while Pillow uses 4:2:0, so turbo-jpeg frames were larger and slower to encode than jpeg ones. With 4:2:0 both encoders produce frames of the same size.
278e889 to
7d30f8a
Compare
|
I pulled these numbers directly from the new CI benchmarks. Values are not comparable across runs due to different hardware, but we can see that Pillow was a bit faster than turbo-jpeg before this PR (#65). And now
Median encode time of one 1920x1080 frame at quality 80, from the |
|
Thanks again for working on this. It looks good to me 👍 |
Two speedups for JPEG image streaming, one commit each:
VtkRemoteControlledAreacaptures RGBA and returns the RGB channels as a view over that buffer. The Pillow and PyTurboJPEG encoders detect the view and read the pixels in place, instead of Pillow copying every frame throughImage.fromarray.img_cols_rowsstill returns a top-down RGB array, and output is byte-identical tomasterfor all formats.turbo-jpegframes about 16% smaller and faster to encode.Benchmark
Capture + encode per 1920x1080 frame (Apple Silicon, rendering excluded):
jpeg(Pillow)turbo-jpegturbo-jpegframemasterTo reproduce (from a trame-rca checkout):
bench_rca.py
For real
turbo-jpegnumbers, alsopip install PyTurboJPEGwith libjpeg-turbo >= 3.0 on the system; otherwise it falls back to Pillow (the script prints which one it used).🤖 Generated with Claude Code