[codex] Move preview pixmap creation to UI thread - #107
Conversation
📝 WalkthroughWalkthroughA new ChangesPreview Image Conversion Module and Widget Refactor
Sequence Diagram(s)sequenceDiagram
participant PreviewWorker
participant imagebuf_to_preview_image
participant PreviewWidget
participant preview_image_to_pixmap
rect rgba(70, 130, 180, 0.5)
Note over PreviewWorker: Worker thread
PreviewWorker->>imagebuf_to_preview_image: ImageBuf
imagebuf_to_preview_image-->>PreviewWorker: PreviewImageData
PreviewWorker-->>PreviewWidget: preview_ready(PreviewImageData)
end
rect rgba(60, 179, 113, 0.5)
Note over PreviewWidget: UI thread
PreviewWidget->>preview_image_to_pixmap: PreviewImageData
preview_image_to_pixmap-->>PreviewWidget: QPixmap
PreviewWidget->>PreviewWidget: store QPixmap, repaint
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderkit/ui/preview_image.py (2)
39-42: ⚡ Quick winRedundant dtype check since
get_pixels(oiio.FLOAT)always returns float32.Line 24 calls
buf.get_pixels(oiio.FLOAT), which always returns anp.float32array regardless of the buffer's internal format. Therefore, the conditionif image.dtype != np.uint8:on line 39 will always beTrue, making the check redundant. Additionally, theastype(np.float32, copy=False)on line 40 is a no-op sinceimageis already float32.While this defensive code doesn't cause incorrect behavior, it adds unnecessary complexity.
♻️ Simplify the conversion logic
- if image.dtype != np.uint8: - image_f32 = image.astype(np.float32, copy=False) - image = np.clip(image_f32, 0.0, 1.0) - image = (image * np.float32(255.0)).astype(np.uint8) + # get_pixels(FLOAT) returns float32 in [0,1] for typical images, but HDR can exceed + image = np.clip(image, 0.0, 1.0) + image = (image * np.float32(255.0)).astype(np.uint8)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/ui/preview_image.py` around lines 39 - 42, Since buf.get_pixels(oiio.FLOAT) always returns a np.float32 array, the condition `if image.dtype != np.uint8:` will always evaluate to True, making the check redundant. Additionally, the astype(np.float32, copy=False) call is a no-op since image is already float32. Remove the redundant dtype check and the unnecessary astype call, keeping only the np.clip operation and the final conversion to uint8 via astype(np.uint8). This simplifies the conversion logic from a conditional block to a direct float-to-uint8 conversion path.
72-89: 💤 Low valueConsider testing the error paths in
_qimage_format.The fallback logic for Qt format resolution (lines 80-89) handles compatibility across Qt versions, but the error path at line 88 ("Qt image format is unavailable") is not tested. While this is unlikely to occur in normal operation, testing it would ensure the error message is helpful and the function fails gracefully.
Add a test that mocks
QImageto returnNonefor bothFormatand the direct attribute, verifying theValueErroris raised with the expected message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/ui/preview_image.py` around lines 72 - 89, Add a test case for the `_qimage_format` function that verifies the error path is properly handled when Qt image format is unavailable. Mock the QImage class to return None for both the Format attribute lookup and the direct format attribute lookup (for unsupported channel values, test with channels=3 or channels=4). The test should assert that a ValueError is raised with the expected message "Qt image format is unavailable: {name}" to ensure the function fails gracefully and provides a helpful error message when neither fallback path succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderkit/ui/preview_image.py`:
- Around line 29-30: Add a test case for the `imagebuf_to_preview_image()`
function that covers the 1D-to-3D reshape code path. Create a test where
`buf.get_pixels()` returns a 1D array with a length equal to height × width ×
nchannels, then verify that the function correctly reshapes it into a 3D array
with dimensions (spec.height, spec.width, spec.nchannels). This will exercise
the currently untested reshape logic at lines 29-30 and ensure it handles the
edge case correctly.
---
Nitpick comments:
In `@src/renderkit/ui/preview_image.py`:
- Around line 39-42: Since buf.get_pixels(oiio.FLOAT) always returns a
np.float32 array, the condition `if image.dtype != np.uint8:` will always
evaluate to True, making the check redundant. Additionally, the
astype(np.float32, copy=False) call is a no-op since image is already float32.
Remove the redundant dtype check and the unnecessary astype call, keeping only
the np.clip operation and the final conversion to uint8 via astype(np.uint8).
This simplifies the conversion logic from a conditional block to a direct
float-to-uint8 conversion path.
- Around line 72-89: Add a test case for the `_qimage_format` function that
verifies the error path is properly handled when Qt image format is unavailable.
Mock the QImage class to return None for both the Format attribute lookup and
the direct format attribute lookup (for unsupported channel values, test with
channels=3 or channels=4). The test should assert that a ValueError is raised
with the expected message "Qt image format is unavailable: {name}" to ensure the
function fails gracefully and provides a helpful error message when neither
fallback path succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb62f7a1-8326-4442-85df-dfb4c6fa4734
📒 Files selected for processing (3)
src/renderkit/ui/preview_image.pysrc/renderkit/ui/widgets.pytests/test_ui_widgets.py
| if image.ndim == 1: | ||
| image = image.reshape((spec.height, spec.width, spec.nchannels)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for tests that exercise the 1D reshape path
rg -n 'ndim.*==.*1' --type=py tests/Repository: Ahmed-Hindy/renderkit
Length of output: 286
🏁 Script executed:
# First, let's understand the preview_image.py file and what function contains lines 29-30
cat -n src/renderkit/ui/preview_image.py | head -40Repository: Ahmed-Hindy/renderkit
Length of output: 1508
🏁 Script executed:
# Check if there are tests specifically for preview_image.py
fd -e py tests/ | xargs grep -l "preview_image"Repository: Ahmed-Hindy/renderkit
Length of output: 397
🏁 Script executed:
# Look at the test files with ndim==1 to see if they test preview_image
cat -n tests/test_integration_real_files.py | sed -n '85,135p'Repository: Ahmed-Hindy/renderkit
Length of output: 2491
🏁 Script executed:
# Search for tests that import or use imagebuf_to_preview_image
rg -n "imagebuf_to_preview_image" tests/Repository: Ahmed-Hindy/renderkit
Length of output: 815
🏁 Script executed:
# Check if there are any tests for preview_image module
rg -n "from.*preview_image|import.*preview_image" tests/Repository: Ahmed-Hindy/renderkit
Length of output: 134
🏁 Script executed:
# Look at test_color_space.py around line 40
cat -n tests/test_color_space.py | sed -n '30,50p'Repository: Ahmed-Hindy/renderkit
Length of output: 830
🏁 Script executed:
# Look at the existing preview_image tests to see if they cover 1D arrays
cat -n tests/test_ui_widgets.py | sed -n '95,180p'Repository: Ahmed-Hindy/renderkit
Length of output: 3431
Add test coverage for 1D array reshape in imagebuf_to_preview_image().
The 1D-to-3D reshape logic (lines 29–30) is not exercised by the existing test suite. Current tests for imagebuf_to_preview_image() only pass 3D pixel arrays, leaving this code path untested. The team's use of the same reshape pattern in other modules (e.g., _buf_to_array() in test_color_space.py) confirms this is a real edge case. Add a test case where buf.get_pixels() returns a 1D array to ensure the reshape works correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderkit/ui/preview_image.py` around lines 29 - 30, Add a test case for
the `imagebuf_to_preview_image()` function that covers the 1D-to-3D reshape code
path. Create a test where `buf.get_pixels()` returns a 1D array with a length
equal to height × width × nchannels, then verify that the function correctly
reshapes it into a 3D array with dimensions (spec.height, spec.width,
spec.nchannels). This will exercise the currently untested reshape logic at
lines 29-30 and ensure it handles the edge case correctly.
2 participants
Summary
PreviewWorkeremit neutralPreviewImageDatainstead of constructingQPixmapin the worker threadQPixmapinPreviewWidgetbefore display/export/fullscreen useRoot cause
PreviewWorker.run()delegated ImageBuf preparation to the shared frame pipeline, but still owned NumPy-to-QImage/QPixmapconversion inline. That kept a GUI-facingQPixmapcreation step inside the worker thread and made the worker/UI boundary blurrier than the shared prep cleanup intended.Validation
uv --system-certs sync --extra devuv --system-certs run pytest tests/test_ui_widgets.pyuv --system-certs run ruff check src/renderkit/ui/widgets.py src/renderkit/ui/preview_image.py tests/test_ui_widgets.pySummary by CodeRabbit
Refactor
Tests