fix(ui): guard preview resize against empty camera frames#1748
Open
Gujiassh wants to merge 2 commits intohacksider:mainfrom
Open
fix(ui): guard preview resize against empty camera frames#1748Gujiassh wants to merge 2 commits intohacksider:mainfrom
Gujiassh wants to merge 2 commits intohacksider:mainfrom
Conversation
Contributor
Reviewer's GuideRefactors image preview resizing into a dedicated helper module with robust guards for invalid/empty frames and adds regression tests to prevent crashes when camera frames are missing or preview dimensions are invalid. Sequence diagram for guarded preview resize during webcam frame handlingsequenceDiagram
participant UI as UIWebcamPreview
participant Camera as CameraFeed
participant Sizing as ImageSizing_fit_image_to_size
participant GPU as GpuProcessing_gpu_resize
UI->>Camera: capture_frame()
Camera-->>UI: image_frame (may be None or empty)
UI->>Sizing: fit_image_to_size(image_frame, target_width, target_height)
alt image is None or missing shape
Sizing->>Sizing: _fallback_frame(target_width, target_height)
Sizing-->>UI: black_fallback_frame
else image.size == 0 or invalid shape
Sizing->>Sizing: _fallback_frame(target_width, target_height)
Sizing-->>UI: black_fallback_frame
else target_width <= 0 or target_height <= 0
Sizing-->>UI: original_image_frame
else valid frame and target size
Sizing->>GPU: gpu_resize(image_frame, dsize=new_size)
GPU-->>Sizing: resized_frame
Sizing-->>UI: resized_frame
end
UI->>UI: render_preview(resized_or_fallback_frame)
Class diagram for image sizing helper module and GPU dependencyclassDiagram
class image_sizing_module {
<<module>>
}
class ImageSizing {
+_fallback_frame(width int, height int) np_ndarray
+fit_image_to_size(image np_ndarray_optional, width int_optional, height int_optional, fallback_size Tuple_int_int) np_ndarray
}
class GpuProcessing {
+gpu_resize(image np_ndarray, dsize Tuple_int_int) np_ndarray
}
image_sizing_module ..> ImageSizing : defines
ImageSizing ..> GpuProcessing : uses gpu_resize
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
fit_image_to_size, when bothwidthandheightareNoneyou returnimageas-is, which can beNonedespite the return type beingnp.ndarray; consider either updating the type annotation or normalizingNoneto a fallback frame for consistency with the other branches. - The test module stubs
numpyandmodules.gpu_processingdirectly insys.modules, which may leak into other tests that import these modules; you might want to confine these fakes via a test-only package or use patching/fixtures so they are scoped to this test file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fit_image_to_size`, when both `width` and `height` are `None` you return `image` as-is, which can be `None` despite the return type being `np.ndarray`; consider either updating the type annotation or normalizing `None` to a fallback frame for consistency with the other branches.
- The test module stubs `numpy` and `modules.gpu_processing` directly in `sys.modules`, which may leak into other tests that import these modules; you might want to confine these fakes via a test-only package or use patching/fixtures so they are scoped to this test file.
## Individual Comments
### Comment 1
<location path="tests/test_image_sizing.py" line_range="32-41" />
<code_context>
+sys.modules.setdefault("numpy", fake_numpy)
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid persistent global monkey-patching of numpy/cv2 to reduce cross-test side effects
This test replaces `numpy`, `modules.gpu_processing`, and `cv2` in `sys.modules` at import time, which can leak into other tests that rely on the real modules as the suite grows.
Use `unittest.mock.patch` (or a pytest fixture/context manager) to patch these modules only for the scope of this module or individual tests, ensuring they are restored afterward and avoiding cross-test interference.
Suggested implementation:
```python
setattr(fake_numpy, "zeros", fake_zeros)
setattr(fake_numpy, "empty", fake_empty)
setattr(fake_numpy, "ndarray", FakeArray)
```
To avoid persistent global monkey-patching and scope the fakes to this test module (or to each test), you should:
1. Add the necessary imports at the top of `tests/test_image_sizing.py`:
```python
import sys
from unittest import mock
```
2. Wrap the import of the code under test in a `mock.patch.dict` context so that `numpy`, `modules.gpu_processing`, and `cv2` are patched only while that import happens, and then automatically restored:
```python
with mock.patch.dict(
sys.modules,
{
"numpy": fake_numpy,
"modules.gpu_processing": fake_gpu_processing,
"cv2": fake_cv2,
},
clear=False,
):
import modules.image_sizing as image_sizing
```
This should replace any existing plain `import modules.image_sizing` (or similar) at module scope in this test file.
3. If some tests import other modules that also need these fakes, you can instead use a pytest fixture to apply the patch per-test:
```python
import pytest
from unittest import mock
@pytest.fixture(autouse=True)
def _patch_image_deps():
with mock.patch.dict(
sys.modules,
{
"numpy": fake_numpy,
"modules.gpu_processing": fake_gpu_processing,
"cv2": fake_cv2,
},
clear=False,
):
yield
```
Place this fixture after the definitions of `fake_numpy`, `fake_gpu_processing`, and `fake_cv2`. Remove any remaining `sys.modules.setdefault("modules.gpu_processing", fake_gpu_processing)` and `sys.modules.setdefault("cv2", fake_cv2)` lines, since the fixture/patch will now manage those modules' lifetimes correctly.
These changes ensure that `numpy`, `modules.gpu_processing`, and `cv2` are only patched for the scope of these tests, and are restored afterward, avoiding cross-test interference.
</issue_to_address>
### Comment 2
<location path="tests/test_image_sizing.py" line_range="67-71" />
<code_context>
+
+ self.assertEqual(resized.shape[:2], (25, 50))
+
+ def test_returns_fallback_frame_for_none_input(self) -> None:
+ resized = fit_image_to_size(None, 320, 240)
+
+ self.assertEqual(resized.shape, (240, 320, 3))
+ self.assertEqual(resized.dtype, fake_numpy.uint8)
+
+ def test_returns_fallback_frame_for_empty_input(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the fallback frame content is black to fully capture the regression requirement
The PR description calls out a "safe black fallback frame", but the tests only assert `shape` and `dtype`, so they would still pass if the frame weren’t black.
Since `FakeArray` doesn’t store data, you could extend it with a `fill_value` (default 0) and have `_fallback_frame` use that so you can assert on pixel values here. If you’d rather keep `FakeArray` minimal, at least clarify in the test name/docstring that we intentionally don’t assert on pixel values so this gap is explicit.
Suggested implementation:
```python
def test_returns_fallback_frame_for_none_input(self) -> None:
resized = fit_image_to_size(None, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
# Ensure that the "safe fallback" is actually a black frame
self.assertEqual(resized.fill_value, 0)
```
```python
def test_returns_fallback_frame_for_empty_input(self) -> None:
image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
# Ensure that the "safe fallback" is actually a black frame
self.assertEqual(resized.fill_value, 0)
```
To make these tests pass and fully implement the "safe black fallback frame" requirement, you will also need to:
1. Extend the `FakeArray` implementation (wherever it is defined, likely in a test helper module) to accept and store a `fill_value`:
- Add a `fill_value` attribute (defaulting to `0`) on construction.
- Ensure `fake_zeros` / `fake_empty` or any factory used to produce the fallback frame can pass a `fill_value` through to `FakeArray`.
2. Update `_fallback_frame` (or the function responsible for constructing the fallback frame used by `fit_image_to_size`) so that:
- It constructs a `FakeArray` (or equivalent) with `fill_value=0` to represent a black frame.
- If there is any simulated access to pixel data (e.g. `__getitem__`), it should return `fill_value` so this invariant is maintained.
3. Optionally, add a small unit test for `FakeArray` itself (if that pattern is used elsewhere) to assert that `fill_value` is set correctly and used consistently.
</issue_to_address>
### Comment 3
<location path="tests/test_image_sizing.py" line_range="73-86" />
<code_context>
+ self.assertEqual(resized.shape, (240, 320, 3))
+ self.assertEqual(resized.dtype, fake_numpy.uint8)
+
+ def test_returns_fallback_frame_for_empty_input(self) -> None:
+ image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8)
+
+ resized = fit_image_to_size(image, 320, 240)
+
+ self.assertEqual(resized.shape, (240, 320, 3))
+ self.assertEqual(resized.dtype, fake_numpy.uint8)
+
+ def test_preserves_original_frame_when_target_size_is_invalid(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for non-3D or degenerate shapes (e.g. 1D/2D arrays) that trigger the len(shape) < 2 branch
The `fit_image_to_size` logic includes `if image.size == 0 or len(image.shape) < 2: return _fallback_frame(...)`. Your new test covers the `size == 0` case but not `len(image.shape) < 2`.
Please add tests for 1D and scalar inputs (e.g. `fake_empty((10,), dtype=...)` and `fake_empty((), dtype=...)`) and assert that `fit_image_to_size` returns the correct fallback frame shape/dtype for those cases.
```suggestion
def test_returns_fallback_frame_for_empty_input(self) -> None:
image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
def test_returns_fallback_frame_for_1d_input(self) -> None:
image = fake_empty((10,), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
def test_returns_fallback_frame_for_scalar_input(self) -> None:
image = fake_empty((), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
def test_preserves_original_frame_when_target_size_is_invalid(self) -> None:
image = fake_zeros((12, 18, 3), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 0, 240)
self.assertIs(resized, image)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+32
to
+41
| sys.modules.setdefault("numpy", fake_numpy) | ||
|
|
||
| fake_gpu_processing = types.ModuleType("modules.gpu_processing") | ||
|
|
||
|
|
||
| def fake_gpu_resize(src, dsize, fx=0.0, fy=0.0, interpolation=None): | ||
| if dsize and dsize != (0, 0): | ||
| width, height = dsize | ||
| else: | ||
| width = max(1, int(src.shape[1] * fx)) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Avoid persistent global monkey-patching of numpy/cv2 to reduce cross-test side effects
This test replaces numpy, modules.gpu_processing, and cv2 in sys.modules at import time, which can leak into other tests that rely on the real modules as the suite grows.
Use unittest.mock.patch (or a pytest fixture/context manager) to patch these modules only for the scope of this module or individual tests, ensuring they are restored afterward and avoiding cross-test interference.
Suggested implementation:
setattr(fake_numpy, "zeros", fake_zeros)
setattr(fake_numpy, "empty", fake_empty)
setattr(fake_numpy, "ndarray", FakeArray)To avoid persistent global monkey-patching and scope the fakes to this test module (or to each test), you should:
-
Add the necessary imports at the top of
tests/test_image_sizing.py:import sys from unittest import mock
-
Wrap the import of the code under test in a
mock.patch.dictcontext so thatnumpy,modules.gpu_processing, andcv2are patched only while that import happens, and then automatically restored:with mock.patch.dict( sys.modules, { "numpy": fake_numpy, "modules.gpu_processing": fake_gpu_processing, "cv2": fake_cv2, }, clear=False, ): import modules.image_sizing as image_sizing
This should replace any existing plain
import modules.image_sizing(or similar) at module scope in this test file. -
If some tests import other modules that also need these fakes, you can instead use a pytest fixture to apply the patch per-test:
import pytest from unittest import mock @pytest.fixture(autouse=True) def _patch_image_deps(): with mock.patch.dict( sys.modules, { "numpy": fake_numpy, "modules.gpu_processing": fake_gpu_processing, "cv2": fake_cv2, }, clear=False, ): yield
Place this fixture after the definitions of
fake_numpy,fake_gpu_processing, andfake_cv2. Remove any remainingsys.modules.setdefault("modules.gpu_processing", fake_gpu_processing)andsys.modules.setdefault("cv2", fake_cv2)lines, since the fixture/patch will now manage those modules' lifetimes correctly.
These changes ensure that numpy, modules.gpu_processing, and cv2 are only patched for the scope of these tests, and are restored afterward, avoiding cross-test interference.
Comment on lines
+67
to
+71
| def test_returns_fallback_frame_for_none_input(self) -> None: | ||
| resized = fit_image_to_size(None, 320, 240) | ||
|
|
||
| self.assertEqual(resized.shape, (240, 320, 3)) | ||
| self.assertEqual(resized.dtype, fake_numpy.uint8) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert that the fallback frame content is black to fully capture the regression requirement
The PR description calls out a "safe black fallback frame", but the tests only assert shape and dtype, so they would still pass if the frame weren’t black.
Since FakeArray doesn’t store data, you could extend it with a fill_value (default 0) and have _fallback_frame use that so you can assert on pixel values here. If you’d rather keep FakeArray minimal, at least clarify in the test name/docstring that we intentionally don’t assert on pixel values so this gap is explicit.
Suggested implementation:
def test_returns_fallback_frame_for_none_input(self) -> None:
resized = fit_image_to_size(None, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
# Ensure that the "safe fallback" is actually a black frame
self.assertEqual(resized.fill_value, 0) def test_returns_fallback_frame_for_empty_input(self) -> None:
image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8)
resized = fit_image_to_size(image, 320, 240)
self.assertEqual(resized.shape, (240, 320, 3))
self.assertEqual(resized.dtype, fake_numpy.uint8)
# Ensure that the "safe fallback" is actually a black frame
self.assertEqual(resized.fill_value, 0)To make these tests pass and fully implement the "safe black fallback frame" requirement, you will also need to:
-
Extend the
FakeArrayimplementation (wherever it is defined, likely in a test helper module) to accept and store afill_value:- Add a
fill_valueattribute (defaulting to0) on construction. - Ensure
fake_zeros/fake_emptyor any factory used to produce the fallback frame can pass afill_valuethrough toFakeArray.
- Add a
-
Update
_fallback_frame(or the function responsible for constructing the fallback frame used byfit_image_to_size) so that:- It constructs a
FakeArray(or equivalent) withfill_value=0to represent a black frame. - If there is any simulated access to pixel data (e.g.
__getitem__), it should returnfill_valueso this invariant is maintained.
- It constructs a
-
Optionally, add a small unit test for
FakeArrayitself (if that pattern is used elsewhere) to assert thatfill_valueis set correctly and used consistently.
Comment on lines
+73
to
+86
| def test_returns_fallback_frame_for_empty_input(self) -> None: | ||
| image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8) | ||
|
|
||
| resized = fit_image_to_size(image, 320, 240) | ||
|
|
||
| self.assertEqual(resized.shape, (240, 320, 3)) | ||
| self.assertEqual(resized.dtype, fake_numpy.uint8) | ||
|
|
||
| def test_preserves_original_frame_when_target_size_is_invalid(self) -> None: | ||
| image = fake_zeros((12, 18, 3), dtype=fake_numpy.uint8) | ||
|
|
||
| resized = fit_image_to_size(image, 0, 240) | ||
|
|
||
| self.assertIs(resized, image) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for non-3D or degenerate shapes (e.g. 1D/2D arrays) that trigger the len(shape) < 2 branch
The fit_image_to_size logic includes if image.size == 0 or len(image.shape) < 2: return _fallback_frame(...). Your new test covers the size == 0 case but not len(image.shape) < 2.
Please add tests for 1D and scalar inputs (e.g. fake_empty((10,), dtype=...) and fake_empty((), dtype=...)) and assert that fit_image_to_size returns the correct fallback frame shape/dtype for those cases.
Suggested change
| def test_returns_fallback_frame_for_empty_input(self) -> None: | |
| image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 320, 240) | |
| self.assertEqual(resized.shape, (240, 320, 3)) | |
| self.assertEqual(resized.dtype, fake_numpy.uint8) | |
| def test_preserves_original_frame_when_target_size_is_invalid(self) -> None: | |
| image = fake_zeros((12, 18, 3), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 0, 240) | |
| self.assertIs(resized, image) | |
| def test_returns_fallback_frame_for_empty_input(self) -> None: | |
| image = fake_empty((0, 0, 3), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 320, 240) | |
| self.assertEqual(resized.shape, (240, 320, 3)) | |
| self.assertEqual(resized.dtype, fake_numpy.uint8) | |
| def test_returns_fallback_frame_for_1d_input(self) -> None: | |
| image = fake_empty((10,), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 320, 240) | |
| self.assertEqual(resized.shape, (240, 320, 3)) | |
| self.assertEqual(resized.dtype, fake_numpy.uint8) | |
| def test_returns_fallback_frame_for_scalar_input(self) -> None: | |
| image = fake_empty((), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 320, 240) | |
| self.assertEqual(resized.shape, (240, 320, 3)) | |
| self.assertEqual(resized.dtype, fake_numpy.uint8) | |
| def test_preserves_original_frame_when_target_size_is_invalid(self) -> None: | |
| image = fake_zeros((12, 18, 3), dtype=fake_numpy.uint8) | |
| resized = fit_image_to_size(image, 0, 240) | |
| self.assertIs(resized, image) |
Contributor
Author
|
Addressed review feedback on top of commit f7b5cb7. What I changed:
No review suggestion was intentionally left unaddressed in this update. Validation run:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fit_image_to_sizeagainstNone, empty frames, and invalid preview dimensions to avoid resize assertionsNoneinput, empty input, and invalid target size handlingFixes #1612.
Testing
python3 -m unittest tests.test_image_sizing