Fix some minor bugs#33
Conversation
WalkthroughAdds EXIF orientation handling/sanitization across decode/load/save, optional OpenCV detection, analysis-driven highlight/shadow recovery with caching, a gate-style preview refresh pipeline, histogram/preview UI sync updates, robust save error handling, many tests/tools, and bumps version to 1.5.2. Changes
Sequence Diagram(s)sequenceDiagram
participant QML as QML / User
participant Provider as ImageProvider / UIState
participant App as AppController
participant Editor as ImageEditor
participant Prefetch as Prefetcher
participant EXIF as EXIF/Orientation
QML->>Provider: request preview (id[/generation])
Provider->>App: forward request / select source
App->>Editor: get_preview_data_cached()
Editor->>Prefetch: decode image (if needed)
Prefetch->>EXIF: read exif_obj
Prefetch-->>Editor: decoded buffer
Editor->>EXIF: apply_exif_orientation(buffer, exif_obj)
EXIF-->>Editor: oriented buffer
Editor->>Editor: _analyze_highlight_state()
Editor-->>Provider: preview + highlightState
Provider->>QML: emit highlightStateChanged
sequenceDiagram
participant QML as QML / User
participant App as AppController
participant Editor as ImageEditor
participant Provider as ImageProvider
participant Cache as Preview Cache
QML->>App: rotate_image_cw()
App->>Editor: rotate_image_cw()
Editor->>Cache: invalidate/refresh preview (_edits_rev)
Editor->>Editor: _apply_edits() → _apply_highlights_shadows()
Editor-->>Provider: new preview + highlightState
Provider->>QML: emit highlightStateChanged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
29-29: Move OpenCV to optional dependencies to match architectural support.The codebase has been refactored to support optional OpenCV (as stated in ChangeLog.md) with
faststack.imaging.optional_depsproviding graceful fallback when OpenCV is unavailable. However,opencv-pythonis still listed as a required dependency in pyproject.toml. Move it to[project.optional-dependencies]to align the dependency configuration with the implemented architecture.
🤖 Fix all issues with AI agents
In `@ChangeLog.md`:
- Line 8: Update the inconsistent heading levels under the 1.5.2 release section
in ChangeLog.md: change the subheadings currently using '####' (e.g., the '####
Added', '#### Changed', and '#### Fixed' headings) to '###' so they match the
other release sections' h3 hierarchy and maintain a correct h2 → h3 → h4
structure.
In `@faststack/app.py`:
- Around line 771-803: Remove unused variables and the stray f-string: drop the
unused backup_path from the unpacking of result and remove the found_index local
(it’s assigned but never used) in the block around saved_path handling; update
the unpack to only capture saved_path and adjust any subsequent references to
found_index (none expected) to use new_index when needed. Also locate and remove
the stray f-string with no placeholders near the same area (around line ~816) so
there are no empty f-strings left in the function; keep all path-resolution
logic using saved_path, new_index, self.image_files, and img.path.resolve
unchanged.
In `@faststack/imaging/math_utils.py`:
- Around line 128-131: The elif branch is dead because max_rgb is already
defined when pre_exposure_linear is None; remove the redundant elif and simply
set max_rgb only when pre_exposure_linear is not None (i.e., keep the if
pre_exposure_linear is not None: max_rgb = rgb_linear.max(axis=2) and delete the
elif 'max_rgb' not in locals() branch), ensuring the code relies on the earlier
definition of max_rgb when pre_exposure_linear is None; reference variables:
pre_exposure_linear, max_rgb, rgb_linear.
- Around line 156-164: Remove the unused parameter k from the
_highlight_recover_linear signature and its docstring reference, and update all
callers to stop passing k; specifically, edit the function
_highlight_recover_linear to delete the k: float = 6.0 parameter and any
docstring text like "Shoulder curve steepness", then modify every call site that
currently passes k=... (e.g., calls from editor.py and tests in
test_highlight_recovery.py) to omit that argument so behavior remains unchanged.
In `@faststack/imaging/orientation.py`:
- Around line 35-76: The function apply_orientation_to_np returns early for
orientation <= 1 and for the else branch without enforcing C-contiguity contrary
to its docstring; update those return paths to wrap the returned array with
np.ascontiguousarray(...) so the function always returns a C-contiguous numpy
array (apply to the early return for orientation <= 1 and the final else branch,
leaving the post-transformation contiguity check for transformed results
intact).
In `@faststack/imaging/prefetch.py`:
- Around line 568-592: Remove the unused timer variable t_after_orient and
replace the broad "except Exception as e" with a narrow set of expected errors
around applying EXIF and opening the image (e.g., OSError,
PILImage.UnidentifiedImageError, AttributeError) so programmer errors aren't
swallowed; keep the try/except only covering the calls to apply_exif_orientation
and PILImage.open/getexif (references: apply_exif_orientation, exif_obj,
PILImage.open, img.getexif), and ensure the subsequent re-establishment of h, w,
buffer, bytes_per_line, and mv always runs regardless of whether EXIF
application succeeded.
In `@faststack/qml/ImageEditorDialog.qml`:
- Line 150: The Detail and Effects sections are using the wrong models:
currently the Repeater in the Detail header uses effectsModel and the Effects
header uses detailModel; fix by swapping the model bindings so the Detail
section's Repeater uses detailModel and the Effects section's Repeater uses
effectsModel (both delegates remain editSlider). Locate the Repeater instances
that reference effectsModel/detailModel (one at the shown diff and the other
around the second occurrence mentioned) and update their model properties
accordingly or rename headers if the intent was reversed.
In `@faststack/tests/reproduce_exif_bug.py`:
- Around line 5-8: Replace the hardcoded Windows path and global cv2 mock:
compute and insert a project-root path relative to __file__ instead of
r"c:\code\faststack" and remove the module-level sys.modules['cv2'] =
MagicMock() assignment; instead, in the test class implement setUp and tearDown
(or use a context manager) to call patch.dict(sys.modules, {'cv2': MagicMock()})
so the mock is scoped to each test, and restore sys.path changes there as well;
look for the module-level sys.path.insert and sys.modules['cv2'] usages and
update them to use __file__, setUp/tearDown, patch.dict, and MagicMock.
In `@faststack/tests/test_editor_error_handling.py`:
- Around line 16-34: The tests assume ImageEditor.load_image raises OSError and
save_image propagates PermissionError, but the implementation of
ImageEditor.load_image returns False on failure and ImageEditor.save_image wraps
exceptions in RuntimeError; update the tests to match this contract: change the
load-image test to assert that editor.load_image("non_existent_file.jpg")
returns False (and not raise), and update the save-image test to expect a
RuntimeError (containing the underlying permission message if needed) instead of
PermissionError; refer to ImageEditor.load_image and ImageEditor.save_image when
locating the behavior to align tests with the implementation.
In `@faststack/tests/test_exif_compat.py`:
- Around line 124-127: The test currently patches self.editor._write_tiff_16bit
as mock_tiff but never uses it, causing unused-mock lint noise; remove the
unused patch binding by deleting "patch.object(self.editor, '_write_tiff_16bit')
as mock_tiff" from the with-statement so only needed mocks
(sanitize_exif_orientation, create_backup_file, PIL.Image.fromarray) remain.
- Around line 38-47: The test test_missing_load_method is not actually removing
load() because MagicMock recreates attributes; change the mocked Exif to forbid
a load attribute (e.g., create the mock with spec_set=[] or a simple object
without load) so hasattr(mock_exif_instance, "load") returns False and the code
path in _get_sanitized_exif_bytes falls back to editor.original_image.getexif;
update the patch that returns mock_exif_instance (patch 'PIL.Image.Exif') and
ensure original_image.getexif is a MagicMock to assert it was called.
In `@faststack/tests/test_generation_aware_preview.py`:
- Line 52: Remove the unused variable assignment "img =
self.provider.requestImage(\"0/5\", None, None)" in the test (or replace with a
throwaway `_ =` if you prefer clarity) so the call remains but the unused value
does not trigger lint; locate the call in test_generation_aware_preview.py where
self.provider.requestImage("0/5", None, None) is invoked and delete the
left-hand assignment.
In `@faststack/tests/test_highlight_recovery.py`:
- Around line 197-212: The main test runner omits calling
test_source_clipping_detection() and wraps all tests in a broad try/except which
suppresses exceptions; update the main-guard to include a call to
test_source_clipping_detection() alongside the other tests (e.g., after
test_headroom_shoulder()) and remove the outer try/except so failures propagate
naturally (do not catch Exception around the test invocations).
In `@faststack/tests/test_highlight_state_normalization.py`:
- Around line 14-33: Rename the duplicate test function so both scenarios run
(e.g., keep one as test_highlight_state_normalization_standard and rename the
other to test_highlight_state_normalization_legacy_keys), ensure the setup uses
self.mock_editor._last_highlight_state with legacy keys (headroom_pct,
clipped_pct, near_white_pct) in one test and canonical keys (headroom_pct,
source_clipped_pct, current_nearwhite_pct) in the other, and add assertions that
the provider/normalizer returns expected values for canonical keys when fed
legacy keys (e.g., source_clipped_pct and current_nearwhite_pct resolve to 0.0
or the expected mapped values) so both behaviors are covered; update any test
docstrings accordingly and remove the stub duplicate.
In `@faststack/tests/test_highlights_responsiveness.py`:
- Around line 34-44: The test currently only prints values and has no
assertions; update test_highlights_responsiveness.py to assert the expected
behavior using the existing variables (steps, vals, out and computed diff = inp
- outp): add assertions that for indices where vals[i] < 0.75 (the pivot
referenced in comments) diff == 0 (or nearly zero with a small tolerance like
1e-6) and for a set of lower brightness inputs verify diff > 0.01 to ensure
responsiveness; keep the loop that computes inp/outp/diff but replace/augment
prints with assert statements that use exact or tolerance comparisons so the
test will fail on regressions.
In `@faststack/tests/test_highlights_v2.py`:
- Around line 39-65: The test_analysis_decoupling test contains an unused local
variable srgb which ruff flags; remove the unused srgb declaration from the test
or, if it was intended to feed the analysis path, pass it into the call that
triggers analysis (e.g., provide srgb to editor._apply_edits or to whatever
ImageEditor method that consumes an sRGB input) so that the variable is actually
used; update assertions only if the analysis input changes; relevant symbols:
test_analysis_decoupling, srgb, editor._apply_edits, ImageEditor,
_initial_edits, and _last_highlight_state.
In `@faststack/tests/test_prefetch_logic.py`:
- Around line 1-7: The test imports use Future in a type/spec at spec=Future but
never imports it, causing a NameError at runtime; fix by adding an import for
concurrent.futures.Future (or from concurrent.futures import Future) at the top
of faststack/tests/test_prefetch_logic.py so the spec=Future reference in the
test (around the Prefetcher-related mocks) resolves correctly.
In `@faststack/ui/provider.py`:
- Around line 823-829: Update the docstring for the highlight analysis return
value in provider.py to match the actual keys returned by the function: replace
references to clipped_pct and near_white_pct with source_clipped_pct and
current_nearwhite_pct (keep headroom_pct as-is), and adjust the descriptions to
refer to the SOURCE image for source_clipped_pct and the processed/current state
for current_nearwhite_pct so the docstring matches the returned dict keys from
this highlight analysis routine.
In `@tests/test_highlights_v2.py`:
- Line 3: The test imports _apply_headroom_shoulder from the wrong module;
update the import in tests/test_highlights_v2.py to import
_apply_headroom_shoulder from its defining module faststack.imaging.math_utils
(keep ImageEditor import as-is if needed) so the test references the symbol
_apply_headroom_shoulder from the correct source module for maintainability and
consistency with test_rolloff.py.
🧹 Nitpick comments (23)
faststack/check_scipy.py (1)
1-6: Consider relocating this utility script.This appears to be a developer utility for checking SciPy availability. Placing it directly in
faststack/means it could be inadvertently imported as part of the package. Consider:
- Moving to a
scripts/ortools/directory outside the main package- Adding a leading underscore (
_check_scipy.py) to indicate internal use- Excluding it from package distribution in
pyproject.tomlfaststack/test_pil_blur.py (1)
1-28: Test file placed outside test directory.This test file is located in
faststack/rather thanfaststack/tests/. This means:
- It won't be discovered by pytest (per
testpaths = ["faststack/tests"]in pyproject.toml)- It will be included in the distributed package unnecessarily
Consider moving to
faststack/tests/test_pil_blur.pyand adapting to use pytest/unittest conventions.faststack/tests/manual_test_error_handling.py (1)
83-89: Minor style: split multi-statement lines.Lines 84 and 86 have multiple statements on one line, which reduces readability.
Suggested fix
success = True - if not test_load_image_raises(): success = False + if not test_load_image_raises(): + success = False print("-" * 20) - if not test_save_image_raises(): success = False + if not test_save_image_raises(): + success = Falsefaststack/imaging/prefetch.py (1)
467-469: Remove the leftoverpassplaceholder.
Orientation is handled later, so this block can be dropped to avoid confusion.🧹 Proposed cleanup
- # EXIF orientation correction - - passfaststack/tests/test_editor.py (1)
60-67: Consider using centralizedHAS_OPENCVfor consistency.The OpenCV guard works correctly, but this PR introduces
faststack/imaging/optional_deps.pywith a centralizedHAS_OPENCVflag. For consistency across the codebase, consider using:from faststack.imaging.optional_deps import HAS_OPENCV if not HAS_OPENCV: self.skipTest("OpenCV not installed, skipping texture test")That said, the local import approach is also valid for test isolation purposes.
faststack/config.py (1)
39-43: Consider extractingversion_sort_keyto avoid duplication.The same function is duplicated in
faststack/tests/test_version_sort.py(lines 6-10). If the test is meant to verify this exact logic, consider importing from a shared location or having the test import fromfaststack.config.faststack/tests/test_highlights_responsiveness.py (1)
12-12: Prefer relative import or proper package installation oversys.pathmanipulation.Using
sys.path.appendis fragile. Consider running tests withpython -m pytestfrom the repo root or ensuring the package is installed in development mode (pip install -e .).faststack/tests/test_rolloff.py (1)
1-5: Remove unused imports.
MagicMock,patch, andsysare imported but never used in this test file. The comments on lines 6-7 mention mocking but no mocking is actually performed since_apply_headroom_shoulderis a pure math function.🧹 Proposed cleanup
import unittest -from unittest.mock import MagicMock, patch import numpy as np -import sys - -# We check for modules that might be missing and mock them if needed -# inside the test setup to avoid import errors at module level.faststack/tests/test_sensitivity.py (3)
5-6: Consider using a proper test discovery mechanism instead ofsys.pathmanipulation.Direct
sys.pathmanipulation can cause import issues and is fragile. Consider running tests viapython -m pytestorpython -m unittestfrom the project root, which handles path resolution automatically.
10-45: Consider converting to unittest.TestCase for consistency.This test uses print statements and bare
assertfor validation, which differs from other test files in this PR that use theunittestframework. Usingunittest.TestCaseprovides better test discovery, structured assertions, and consistent output formatting.♻️ Example conversion
import unittest import numpy as np from faststack.imaging.editor import ImageEditor class TestContrastSaturationSensitivity(unittest.TestCase): def test_contrast_at_1_0(self): editor = ImageEditor() arr = np.zeros((100, 100, 3), dtype=np.float32) arr[:, :50, 0] = 0.8 arr[:, 50:, 1] = 0.8 editor.float_preview = arr edits = editor._initial_edits() edits['contrast'] = 1.0 out = editor._apply_edits(arr.copy(), edits=edits) self.assertAlmostEqual(out[0, 0, 0], 0.92, delta=0.01) def test_saturation_at_1_0(self): editor = ImageEditor() arr = np.zeros((100, 100, 3), dtype=np.float32) arr[:, :50, 0] = 0.8 editor.float_preview = arr edits = editor._initial_edits() edits['saturation'] = 1.0 out = editor._apply_edits(arr.copy(), edits=edits) self.assertAlmostEqual(out[0, 0, 0], 1.0804, delta=0.01) if __name__ == "__main__": unittest.main()
47-52: Catching bareExceptionhides unexpected failures.Per the static analysis hint, catching
Exceptioncan mask programming errors. Since this is test scaffolding, a bare exception may be acceptable for quick diagnostics, but it makes debugging harder if an unexpected error occurs.🧹 Narrow the exception type or log the traceback
if __name__ == "__main__": try: test_contrast_saturation_sensitivity() - except Exception as e: - print(f"Test failed: {e}") + except AssertionError as e: + print(f"Test assertion failed: {e}") + sys.exit(1) + except ImportError as e: + print(f"Import failed: {e}") sys.exit(1)faststack/tests/test_raw_pipeline.py (1)
88-97: String-based method name assertion is fragile.Line 95 uses
'_on_develop_finished' in str(callback.func)to verify the callback. This could break if the method is renamed or if the string representation changes. Consider using a more robust check.♻️ More robust callback verification
# callback is functools.partial(self._on_develop_finished, False, err_msg) # For a bound method, callback.func is the method - self.assertTrue(hasattr(callback, 'func')) - self.assertTrue('_on_develop_finished' in str(callback.func)) + self.assertTrue(hasattr(callback, 'func')) + self.assertEqual(callback.func.__name__, '_on_develop_finished') self.assertEqual(callback.args[0], False) # Success = False self.assertIn("timed out", callback.args[1]) # Msgfaststack/tests/mini_test.py (1)
1-47: Consider converting to unittest.TestCase for consistency with other test files.This diagnostic script uses print-based PASS/FAIL output and bare exception handlers. While functional for quick validation, converting to
unittest.TestCasewould:
- Enable test discovery via standard runners
- Provide structured assertion methods
- Align with other test files in this PR
The bare
Exceptioncatches (lines 19, 39, 45) flagged by static analysis are acceptable here since they serve diagnostic purposes in a test script.faststack/tests/test_cache_invalidation.py (3)
18-26: Use a temporary directory instead of a relative path.The hardcoded path
tests/dummy_images_cacheassumes the test runs from the repository root. This can fail in different test runners or CI environments. Usetempfile.mkdtemp()for reliable cleanup.♻️ Use tempfile for test isolation
+import tempfile + def test_cache_stability(): """Verify that cache hash remains stable when reloading the same unmodified file.""" - # Setup dummy image - test_dir = Path("tests/dummy_images_cache") - test_dir.mkdir(parents=True, exist_ok=True) + # Setup dummy image in temp directory + test_dir = Path(tempfile.mkdtemp(prefix="faststack_cache_test_")) img_path = test_dir / "test_cache.jpg"
49-66: Use assertions instead of print-based PASS/FAIL.The test prints PASS/FAIL but doesn't raise exceptions on failure, so test runners won't detect failures. Lines 49-52 and 63-66 should use
assertor unittest assertions.♻️ Add assertions for test runner compatibility
if hash1 == hash2: print("PASS: Hash is stable across reloads.") else: - print("FAIL: Hash changed across reloads (unnecessary invalidation).") + # Note: Current known behavior - hash changes due to id() usage + print("INFO: Hash changed across reloads (known behavior with id()-based hashing).") # ... if hash3 != hash2: print("PASS: Hash changed after mtime update.") else: - print("FAIL: Hash did NOT change after mtime update.") + raise AssertionError("FAIL: Hash did NOT change after mtime update.")
68-72: Avoid bareexceptclause.Per static analysis, the bare
except:at line 71 can hide errors. Use a specific exception type or at minimum log the error.🧹 Proposed fix
# Cleanup try: shutil.rmtree(test_dir) - except: - pass + except OSError as e: + print(f"Warning: Cleanup failed: {e}")faststack/tests/test_fallback_blur.py (1)
5-5: Remove unused import.
PIL.Imageis imported but never used in this test file.🧹 Remove unused import
import unittest import numpy as np from unittest.mock import patch, MagicMock -from PIL import Imagefaststack/imaging/editor.py (6)
145-174: Consider using underscore prefix for unused variables.At line 146,
handware unpacked but never used. Per static analysis hint and Python conventions, prefix with underscore:Suggested fix
- h, w, c = arr.shape + _h, _w, c = arr.shape
590-594: Unused variablesnwandnh.These variables are unpacked but never used since
np.stackinfers the shape automatically.Suggested fix
# Merge back # Assume all channels rotated to same size - nw, nh = channels[0].size new_arr = np.stack([np.array(ch) for ch in channels], axis=-1) return new_arr
609-612: Remove unused variableis_export.
is_exportis assigned but never used; the code usesfor_exportdirectly throughout.Suggested fix
- is_export = for_export - # Alias arr = img_arr
1092-1095: Consider defining a custom exception or shortening the message.Per static analysis (TRY003), long exception messages are better placed in a custom exception class. However, this is a minor style issue and the current implementation is functional.
1203-1204: Minor typo in comment."If caller passed analysis_state, usage that" should be "use that".
Suggested fix
- # If caller passed analysis_state, usage that. + # If caller passed analysis_state, use that.
1571-1573: Redundant exception object inlog.exceptioncall.
log.exception()automatically includes the exception information in the log output, so including{e}in the message is redundant.Suggested fix
except Exception as e: - log.exception(f"Failed to save {self.current_filepath}: {e}") + log.exception(f"Failed to save {self.current_filepath}") raise RuntimeError(f"Save failed: {str(e)}") from e
| saved_path, backup_path = result | ||
|
|
||
| # If we overwrote the current file, we need to refresh checks | ||
| # But usually we save as a new file or overwrite. | ||
| # Use get_active_edit_path logic? ImageEditor handles correct path logic? | ||
| # ImageEditor.save_image saves to self.current_filepath | ||
| # --- Restore Old Behavior --- | ||
|
|
||
| # Invalidate cache for this file | ||
| self.display_generation += 1 | ||
| self.image_cache.clear() # Brute force clear to ensure fresh load | ||
| # 1. Close Editor UI | ||
| self.ui_state.isEditorOpen = False | ||
|
|
||
| # 2. Clear Editor State (release memory) | ||
| self.image_editor.clear() | ||
|
|
||
| # 3. Refresh List (to see new file or updated timestamp) | ||
| self.refresh_image_list() | ||
|
|
||
| # 4. Find and Select the saved image | ||
| new_index = self.current_index # Default to keeping selection if not found | ||
|
|
||
| # Try to find by exact path match | ||
| found_index = -1 | ||
| if saved_path: | ||
| try: | ||
| target_resolve = saved_path.resolve() | ||
| for i, img in enumerate(self.image_files): | ||
| try: | ||
| # Robust path comparison | ||
| if img.path.resolve() == target_resolve: | ||
| new_index = i | ||
| found_index = i | ||
| break | ||
| except (OSError, RuntimeError): | ||
| # Fallback to string compare | ||
| if str(img.path) == str(saved_path): | ||
| new_index = i | ||
| found_index = i |
There was a problem hiding this comment.
Clean up a few unused vars / stray f-string.
backup_path and found_index aren’t used, and the f-string has no placeholders.
🧹 Proposed fix
- saved_path, backup_path = result
+ saved_path, _backup_path = result
...
- found_index = -1
...
- self.update_status_message(f"Image saved")
+ self.update_status_message("Image saved")Also applies to: 816-816
🧰 Tools
🪛 Ruff (0.14.13)
771-771: Unpacked variable backup_path is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
803-803: Local variable found_index is assigned to but never used
Remove assignment to unused variable found_index
(F841)
🤖 Prompt for AI Agents
In `@faststack/app.py` around lines 771 - 803, Remove unused variables and the
stray f-string: drop the unused backup_path from the unpacking of result and
remove the found_index local (it’s assigned but never used) in the block around
saved_path handling; update the unpack to only capture saved_path and adjust any
subsequent references to found_index (none expected) to use new_index when
needed. Also locate and remove the stray f-string with no placeholders near the
same area (around line ~816) so there are no empty f-strings left in the
function; keep all path-resolution logic using saved_path, new_index,
self.image_files, and img.path.resolve unchanged.
| if pre_exposure_linear is not None: | ||
| max_rgb = rgb_linear.max(axis=2) | ||
| elif 'max_rgb' not in locals(): | ||
| max_rgb = rgb_linear.max(axis=2) |
There was a problem hiding this comment.
Dead code: the elif branch can never execute.
If pre_exposure_linear is not None, line 129 defines max_rgb. If pre_exposure_linear is None, line 106 already defined max_rgb. Therefore, the condition 'max_rgb' not in locals() at line 130 is always False.
Proposed fix
# 2. Current Near-White Statistics (for Pivot Nudging)
# This drives the "micro-contrast feel" based on how bright the image IS NOW.
# Re-calculate max_rgb if we didn't do it earlier (optimization)
if pre_exposure_linear is not None:
max_rgb = rgb_linear.max(axis=2)
- elif 'max_rgb' not in locals():
- max_rgb = rgb_linear.max(axis=2)
+ # else: max_rgb was already computed at line 106🤖 Prompt for AI Agents
In `@faststack/imaging/math_utils.py` around lines 128 - 131, The elif branch is
dead because max_rgb is already defined when pre_exposure_linear is None; remove
the redundant elif and simply set max_rgb only when pre_exposure_linear is not
None (i.e., keep the if pre_exposure_linear is not None: max_rgb =
rgb_linear.max(axis=2) and delete the elif 'max_rgb' not in locals() branch),
ensuring the code relies on the earlier definition of max_rgb when
pre_exposure_linear is None; reference variables: pre_exposure_linear, max_rgb,
rgb_linear.
| def _highlight_recover_linear( | ||
| rgb_linear: np.ndarray, | ||
| amount: float, | ||
| *, | ||
| pivot: float = 0.7, | ||
| k: float = 6.0, | ||
| chroma_rolloff: float = 0.15, | ||
| headroom_ceiling: float = 1.0, | ||
| ) -> np.ndarray: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the complete function definition and check if k is used
rg -n '_highlight_recover_linear' --type py -A 80 faststack/imaging/math_utils.py | head -100Repository: AlanRockefeller/faststack
Length of output: 4110
🏁 Script executed:
# Check all call sites of _highlight_recover_linear across the codebase
rg -n '_highlight_recover_linear\(' --type pyRepository: AlanRockefeller/faststack
Length of output: 1098
🏁 Script executed:
# Also check the specific line in editor.py mentioned in the review
rg -n '_highlight_recover_linear' faststack/ --type py -B 2 -A 2Repository: AlanRockefeller/faststack
Length of output: 4339
🏁 Script executed:
# Check the actual call to _highlight_recover_linear in editor.py around line 1343
sed -n '1340,1355p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 690
🏁 Script executed:
# Get more context around the editor.py call site (lines 1340-1360)
sed -n '1340,1360p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 882
Remove unused parameter k from function signature.
The parameter k is declared with a default value and documented as "Shoulder curve steepness," but is never used in the function body. The implementation uses a simple linear recovery approach without any shoulder curve logic. Multiple callers in test_highlight_recovery.py and editor.py pass k=6.0 or k=k explicitly, but it has no effect. Remove the parameter and its docstring reference to eliminate confusion.
Recommended change
def _highlight_recover_linear(
rgb_linear: np.ndarray,
amount: float,
*,
pivot: float = 0.7,
- k: float = 6.0,
chroma_rolloff: float = 0.15,
headroom_ceiling: float = 1.0,
) -> np.ndarray:
"""Apply highlight recovery using brightness-based rescaling to preserve hue.Update all call sites in editor.py (line 1343) and all test calls in test_highlight_recovery.py (lines 38, 63, 78, 102, 116, 185, 190) to remove the k= argument.
🧰 Tools
🪛 Ruff (0.14.13)
161-161: Unused function argument: k
(ARG001)
🤖 Prompt for AI Agents
In `@faststack/imaging/math_utils.py` around lines 156 - 164, Remove the unused
parameter k from the _highlight_recover_linear signature and its docstring
reference, and update all callers to stop passing k; specifically, edit the
function _highlight_recover_linear to delete the k: float = 6.0 parameter and
any docstring text like "Shoulder curve steepness", then modify every call site
that currently passes k=... (e.g., calls from editor.py and tests in
test_highlight_recovery.py) to omit that argument so behavior remains unchanged.
| def apply_orientation_to_np(buffer: np.ndarray, orientation: int) -> np.ndarray: | ||
| """Apply EXIF orientation transformation to a numpy image buffer. | ||
|
|
||
| Args: | ||
| buffer: Image as numpy array (H, W, 3) RGB uint8 or float32 | ||
| orientation: Orientation value (1-8) | ||
|
|
||
| Returns: | ||
| Transformed numpy array. Guaranteed to be C-contiguous. | ||
| """ | ||
| if orientation <= 1: | ||
| return buffer | ||
|
|
||
| # Apply transformation based on orientation | ||
| if orientation == 2: | ||
| # Mirrored horizontally | ||
| result = np.fliplr(buffer) | ||
| elif orientation == 3: | ||
| # Rotated 180 degrees | ||
| result = np.rot90(buffer, k=2) | ||
| elif orientation == 4: | ||
| # Mirrored vertically | ||
| result = np.flipud(buffer) | ||
| elif orientation == 5: | ||
| # Mirrored horizontally then rotated 90 CCW | ||
| result = np.rot90(np.fliplr(buffer), k=1) | ||
| elif orientation == 6: | ||
| # Rotated 90 CW (270 CCW) | ||
| result = np.rot90(buffer, k=3) | ||
| elif orientation == 7: | ||
| # Mirrored horizontally then rotated 90 CW | ||
| result = np.rot90(np.fliplr(buffer), k=3) | ||
| elif orientation == 8: | ||
| # Rotated 90 CCW | ||
| result = np.rot90(buffer, k=1) | ||
| else: | ||
| return buffer | ||
|
|
||
| # Ensure result is C-contiguous after flip/rotate | ||
| if not result.flags['C_CONTIGUOUS']: | ||
| result = np.ascontiguousarray(result) | ||
| return result |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "orientation.py" -path "*/imaging/*" | head -5Repository: AlanRockefeller/faststack
Length of output: 103
🏁 Script executed:
cat -n ./faststack/imaging/orientation.py | head -100Repository: AlanRockefeller/faststack
Length of output: 3396
Enforce C-contiguity guarantee on all return paths.
The docstring promises the output is "Guaranteed to be C-contiguous," but the early returns for orientation <= 1 and invalid orientations bypass contiguity normalization. Callers relying on this contract could receive non-contiguous arrays.
Proposed fix
- if orientation <= 1:
- return buffer
+ if orientation <= 1:
+ return buffer if buffer.flags["C_CONTIGUOUS"] else np.ascontiguousarray(buffer)
@@
- else:
- return buffer
+ else:
+ return buffer if buffer.flags["C_CONTIGUOUS"] else np.ascontiguousarray(buffer)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def apply_orientation_to_np(buffer: np.ndarray, orientation: int) -> np.ndarray: | |
| """Apply EXIF orientation transformation to a numpy image buffer. | |
| Args: | |
| buffer: Image as numpy array (H, W, 3) RGB uint8 or float32 | |
| orientation: Orientation value (1-8) | |
| Returns: | |
| Transformed numpy array. Guaranteed to be C-contiguous. | |
| """ | |
| if orientation <= 1: | |
| return buffer | |
| # Apply transformation based on orientation | |
| if orientation == 2: | |
| # Mirrored horizontally | |
| result = np.fliplr(buffer) | |
| elif orientation == 3: | |
| # Rotated 180 degrees | |
| result = np.rot90(buffer, k=2) | |
| elif orientation == 4: | |
| # Mirrored vertically | |
| result = np.flipud(buffer) | |
| elif orientation == 5: | |
| # Mirrored horizontally then rotated 90 CCW | |
| result = np.rot90(np.fliplr(buffer), k=1) | |
| elif orientation == 6: | |
| # Rotated 90 CW (270 CCW) | |
| result = np.rot90(buffer, k=3) | |
| elif orientation == 7: | |
| # Mirrored horizontally then rotated 90 CW | |
| result = np.rot90(np.fliplr(buffer), k=3) | |
| elif orientation == 8: | |
| # Rotated 90 CCW | |
| result = np.rot90(buffer, k=1) | |
| else: | |
| return buffer | |
| # Ensure result is C-contiguous after flip/rotate | |
| if not result.flags['C_CONTIGUOUS']: | |
| result = np.ascontiguousarray(result) | |
| return result | |
| def apply_orientation_to_np(buffer: np.ndarray, orientation: int) -> np.ndarray: | |
| """Apply EXIF orientation transformation to a numpy image buffer. | |
| Args: | |
| buffer: Image as numpy array (H, W, 3) RGB uint8 or float32 | |
| orientation: Orientation value (1-8) | |
| Returns: | |
| Transformed numpy array. Guaranteed to be C-contiguous. | |
| """ | |
| if orientation <= 1: | |
| return buffer if buffer.flags["C_CONTIGUOUS"] else np.ascontiguousarray(buffer) | |
| # Apply transformation based on orientation | |
| if orientation == 2: | |
| # Mirrored horizontally | |
| result = np.fliplr(buffer) | |
| elif orientation == 3: | |
| # Rotated 180 degrees | |
| result = np.rot90(buffer, k=2) | |
| elif orientation == 4: | |
| # Mirrored vertically | |
| result = np.flipud(buffer) | |
| elif orientation == 5: | |
| # Mirrored horizontally then rotated 90 CCW | |
| result = np.rot90(np.fliplr(buffer), k=1) | |
| elif orientation == 6: | |
| # Rotated 90 CW (270 CCW) | |
| result = np.rot90(buffer, k=3) | |
| elif orientation == 7: | |
| # Mirrored horizontally then rotated 90 CW | |
| result = np.rot90(np.fliplr(buffer), k=3) | |
| elif orientation == 8: | |
| # Rotated 90 CCW | |
| result = np.rot90(buffer, k=1) | |
| else: | |
| return buffer if buffer.flags["C_CONTIGUOUS"] else np.ascontiguousarray(buffer) | |
| # Ensure result is C-contiguous after flip/rotate | |
| if not result.flags['C_CONTIGUOUS']: | |
| result = np.ascontiguousarray(result) | |
| return result |
🤖 Prompt for AI Agents
In `@faststack/imaging/orientation.py` around lines 35 - 76, The function
apply_orientation_to_np returns early for orientation <= 1 and for the else
branch without enforcing C-contiguity contrary to its docstring; update those
return paths to wrap the returned array with np.ascontiguousarray(...) so the
function always returns a C-contiguous numpy array (apply to the early return
for orientation <= 1 and the final else branch, leaving the post-transformation
contiguity check for transformed results intact).
| print("\nBrightness Levels (Linear 0.0 -> 1.0):") | ||
| print("Input -> Output (Diff)") | ||
| for i in range(steps): | ||
| inp = vals[i] | ||
| outp = out[0, i, 0] | ||
| diff = inp - outp | ||
| print(f"{inp:0.2f} -> {outp:0.4f} ({diff:0.4f})") | ||
|
|
||
| # The goal is to see significant changes (diff > 0.01) starting from lower levels | ||
| # Currently, with pivot 0.75, values below 0.75 should be unchanged (diff=0) | ||
|
|
There was a problem hiding this comment.
Test lacks assertions—only prints output.
This test runs through the highlight recovery logic but has no assert statements to validate behavior. As written, it will always pass regardless of output. The comments on lines 42-43 describe expected behavior that should be asserted.
💚 Proposed fix to add basic assertions
for i in range(steps):
inp = vals[i]
outp = out[0, i, 0]
diff = inp - outp
print(f"{inp:0.2f} -> {outp:0.4f} ({diff:0.4f})")
- # The goal is to see significant changes (diff > 0.01) starting from lower levels
- # Currently, with pivot 0.75, values below 0.75 should be unchanged (diff=0)
+ # Values below pivot should be mostly unchanged
+ # Higher values should show recovery effect
+ high_val_idx = steps - 2 # 0.9 linear
+ high_diff = vals[high_val_idx] - out[0, high_val_idx, 0]
+ self.assertGreater(high_diff, 0.01, "Highlight recovery should affect bright values")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print("\nBrightness Levels (Linear 0.0 -> 1.0):") | |
| print("Input -> Output (Diff)") | |
| for i in range(steps): | |
| inp = vals[i] | |
| outp = out[0, i, 0] | |
| diff = inp - outp | |
| print(f"{inp:0.2f} -> {outp:0.4f} ({diff:0.4f})") | |
| # The goal is to see significant changes (diff > 0.01) starting from lower levels | |
| # Currently, with pivot 0.75, values below 0.75 should be unchanged (diff=0) | |
| print("\nBrightness Levels (Linear 0.0 -> 1.0):") | |
| print("Input -> Output (Diff)") | |
| for i in range(steps): | |
| inp = vals[i] | |
| outp = out[0, i, 0] | |
| diff = inp - outp | |
| print(f"{inp:0.2f} -> {outp:0.4f} ({diff:0.4f})") | |
| # Values below pivot should be mostly unchanged | |
| # Higher values should show recovery effect | |
| high_val_idx = steps - 2 # 0.9 linear | |
| high_diff = vals[high_val_idx] - out[0, high_val_idx, 0] | |
| self.assertGreater(high_diff, 0.01, "Highlight recovery should affect bright values") |
🤖 Prompt for AI Agents
In `@faststack/tests/test_highlights_responsiveness.py` around lines 34 - 44, The
test currently only prints values and has no assertions; update
test_highlights_responsiveness.py to assert the expected behavior using the
existing variables (steps, vals, out and computed diff = inp - outp): add
assertions that for indices where vals[i] < 0.75 (the pivot referenced in
comments) diff == 0 (or nearly zero with a small tolerance like 1e-6) and for a
set of lower brightness inputs verify diff > 0.01 to ensure responsiveness; keep
the loop that computes inp/outp/diff but replace/augment prints with assert
statements that use exact or tolerance comparisons so the test will fail on
regressions.
| def test_analysis_decoupling(self): | ||
| """Verify analysis runs before adjustments and is cached via preview path.""" | ||
| editor = ImageEditor() | ||
| # Create a linear image with some headroom | ||
| linear = np.ones((100, 100, 3), dtype=np.float32) * 1.2 | ||
| # sRGB mock indicating some clipping (e.g. 255) | ||
| srgb = np.ones((100, 100, 3), dtype=np.uint8) * 255 | ||
|
|
||
| # Setup editor state to simulate the image being loaded | ||
| # We need this because _apply_edits works on self.float_image/preview logic usually, | ||
| # but one can pass arr. | ||
| # But _apply_edits updates _last_highlight_state. | ||
|
|
||
| # Run _apply_edits flow | ||
| edits = editor._initial_edits() | ||
| edits['highlights'] = -0.5 | ||
|
|
||
| # _apply_edits expects global self.float_image for some contexts? | ||
| # No, it takes img_arr arg. | ||
|
|
||
| editor._apply_edits(linear, edits=edits, for_export=False) | ||
|
|
||
| # Check cache | ||
| self.assertIsNotNone(editor._last_highlight_state) | ||
| # Note: update logic might use striding so check rough values | ||
| self.assertGreater(editor._last_highlight_state['headroom_pct'], 0.9) | ||
|
|
There was a problem hiding this comment.
Remove unused srgb fixture.
Ruff flags it as unused; drop it or pass it into the analysis path if that was the intent.
🧹 Proposed fix
- # sRGB mock indicating some clipping (e.g. 255)
- srgb = np.ones((100, 100, 3), dtype=np.uint8) * 255📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_analysis_decoupling(self): | |
| """Verify analysis runs before adjustments and is cached via preview path.""" | |
| editor = ImageEditor() | |
| # Create a linear image with some headroom | |
| linear = np.ones((100, 100, 3), dtype=np.float32) * 1.2 | |
| # sRGB mock indicating some clipping (e.g. 255) | |
| srgb = np.ones((100, 100, 3), dtype=np.uint8) * 255 | |
| # Setup editor state to simulate the image being loaded | |
| # We need this because _apply_edits works on self.float_image/preview logic usually, | |
| # but one can pass arr. | |
| # But _apply_edits updates _last_highlight_state. | |
| # Run _apply_edits flow | |
| edits = editor._initial_edits() | |
| edits['highlights'] = -0.5 | |
| # _apply_edits expects global self.float_image for some contexts? | |
| # No, it takes img_arr arg. | |
| editor._apply_edits(linear, edits=edits, for_export=False) | |
| # Check cache | |
| self.assertIsNotNone(editor._last_highlight_state) | |
| # Note: update logic might use striding so check rough values | |
| self.assertGreater(editor._last_highlight_state['headroom_pct'], 0.9) | |
| def test_analysis_decoupling(self): | |
| """Verify analysis runs before adjustments and is cached via preview path.""" | |
| editor = ImageEditor() | |
| # Create a linear image with some headroom | |
| linear = np.ones((100, 100, 3), dtype=np.float32) * 1.2 | |
| # Setup editor state to simulate the image being loaded | |
| # We need this because _apply_edits works on self.float_image/preview logic usually, | |
| # but one can pass arr. | |
| # But _apply_edits updates _last_highlight_state. | |
| # Run _apply_edits flow | |
| edits = editor._initial_edits() | |
| edits['highlights'] = -0.5 | |
| # _apply_edits expects global self.float_image for some contexts? | |
| # No, it takes img_arr arg. | |
| editor._apply_edits(linear, edits=edits, for_export=False) | |
| # Check cache | |
| self.assertIsNotNone(editor._last_highlight_state) | |
| # Note: update logic might use striding so check rough values | |
| self.assertGreater(editor._last_highlight_state['headroom_pct'], 0.9) |
🧰 Tools
🪛 Ruff (0.14.13)
45-45: Local variable srgb is assigned to but never used
Remove assignment to unused variable srgb
(F841)
🤖 Prompt for AI Agents
In `@faststack/tests/test_highlights_v2.py` around lines 39 - 65, The
test_analysis_decoupling test contains an unused local variable srgb which ruff
flags; remove the unused srgb declaration from the test or, if it was intended
to feed the analysis path, pass it into the call that triggers analysis (e.g.,
provide srgb to editor._apply_edits or to whatever ImageEditor method that
consumes an sRGB input) so that the variable is actually used; update assertions
only if the analysis input changes; relevant symbols: test_analysis_decoupling,
srgb, editor._apply_edits, ImageEditor, _initial_edits, and
_last_highlight_state.
| import unittest | ||
| from unittest.mock import MagicMock | ||
| from concurrent.futures import Future | ||
| from unittest.mock import MagicMock, patch | ||
| import sys | ||
|
|
||
| # Mock config before importing prefetcher | ||
| sys.modules['faststack.config'] = MagicMock() | ||
| from faststack.imaging.prefetch import Prefetcher |
There was a problem hiding this comment.
Missing Future import causes runtime error.
The Future class is used on line 31 (spec=Future) but is not imported. This will raise a NameError when the test runs.
🐛 Proposed fix
import unittest
from unittest.mock import MagicMock, patch
import sys
+from concurrent.futures import Future
# Mock config before importing prefetcher
sys.modules['faststack.config'] = MagicMock()🤖 Prompt for AI Agents
In `@faststack/tests/test_prefetch_logic.py` around lines 1 - 7, The test imports
use Future in a type/spec at spec=Future but never imports it, causing a
NameError at runtime; fix by adding an import for concurrent.futures.Future (or
from concurrent.futures import Future) at the top of
faststack/tests/test_prefetch_logic.py so the spec=Future reference in the test
(around the Prefetcher-related mocks) resolves correctly.
| """Returns highlight analysis state for UI display. | ||
|
|
||
| Returns dict with: | ||
| - headroom_pct: Fraction of pixels with recoverable data above 1.0 (16-bit sources) | ||
| - clipped_pct: Fraction of pixels clipped in the SOURCE image (JPEG flat-top @ 254+) | ||
| - near_white_pct: Fraction of pixels currently near white in the processed state. | ||
| """ |
There was a problem hiding this comment.
Docstring lists obsolete highlight keys.
The docstring still mentions clipped_pct/near_white_pct, but the property returns source_clipped_pct/current_nearwhite_pct. Update the docstring to avoid UI/API confusion.
📝 Docstring fix
- - clipped_pct: Fraction of pixels clipped in the SOURCE image (JPEG flat-top @ 254+)
- - near_white_pct: Fraction of pixels currently near white in the processed state.
+ - source_clipped_pct: Fraction of pixels clipped in the SOURCE image (JPEG flat-top @ 254+)
+ - current_nearwhite_pct: Fraction of pixels currently near white in the processed state.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Returns highlight analysis state for UI display. | |
| Returns dict with: | |
| - headroom_pct: Fraction of pixels with recoverable data above 1.0 (16-bit sources) | |
| - clipped_pct: Fraction of pixels clipped in the SOURCE image (JPEG flat-top @ 254+) | |
| - near_white_pct: Fraction of pixels currently near white in the processed state. | |
| """ | |
| """Returns highlight analysis state for UI display. | |
| Returns dict with: | |
| - headroom_pct: Fraction of pixels with recoverable data above 1.0 (16-bit sources) | |
| - source_clipped_pct: Fraction of pixels clipped in the SOURCE image (JPEG flat-top @ 254+) | |
| - current_nearwhite_pct: Fraction of pixels currently near white in the processed state. | |
| """ |
🤖 Prompt for AI Agents
In `@faststack/ui/provider.py` around lines 823 - 829, Update the docstring for
the highlight analysis return value in provider.py to match the actual keys
returned by the function: replace references to clipped_pct and near_white_pct
with source_clipped_pct and current_nearwhite_pct (keep headroom_pct as-is), and
adjust the descriptions to refer to the SOURCE image for source_clipped_pct and
the processed/current state for current_nearwhite_pct so the docstring matches
the returned dict keys from this highlight analysis routine.
| @@ -0,0 +1,67 @@ | |||
| import unittest | |||
| import numpy as np | |||
| from faststack.imaging.editor import ImageEditor, _apply_headroom_shoulder | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "_apply_headroom_shoulder" faststack -g'*.py'Repository: AlanRockefeller/faststack
Length of output: 2109
🏁 Script executed:
sed -n '1,50p' faststack/imaging/editor.py | grep -A 20 "_apply_headroom_shoulder\|__all__"Repository: AlanRockefeller/faststack
Length of output: 515
🏁 Script executed:
sed -n '20,30p' faststack/tests/test_highlight_recovery.pyRepository: AlanRockefeller/faststack
Length of output: 291
Fix import source to use the defining module.
_apply_headroom_shoulder is defined in faststack.imaging.math_utils, not faststack.imaging.editor. While the current import happens to work because editor.py imports it, imports should reference the defining module for maintainability. Align with the pattern used in test_rolloff.py:
Corrected import
-from faststack.imaging.editor import ImageEditor, _apply_headroom_shoulder
+from faststack.imaging.editor import ImageEditor
+from faststack.imaging.math_utils import _apply_headroom_shoulder📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from faststack.imaging.editor import ImageEditor, _apply_headroom_shoulder | |
| from faststack.imaging.editor import ImageEditor | |
| from faststack.imaging.math_utils import _apply_headroom_shoulder |
🤖 Prompt for AI Agents
In `@tests/test_highlights_v2.py` at line 3, The test imports
_apply_headroom_shoulder from the wrong module; update the import in
tests/test_highlights_v2.py to import _apply_headroom_shoulder from its defining
module faststack.imaging.math_utils (keep ImageEditor import as-is if needed) so
the test references the symbol _apply_headroom_shoulder from the correct source
module for maintainability and consistency with test_rolloff.py.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@faststack/imaging/editor.py`:
- Around line 1710-1722: The fallback branch that assigns exif_bytes from
original_image.info.get("exif") can leave a non-1 Orientation tag; update the
logic so that when using original_image's EXIF you pass it through
sanitize_exif_orientation before assigning to exif_bytes. Locate the block
around transforms_applied / _source_exif_bytes handling (methods/variables:
_get_sanitized_exif_bytes, _source_exif_bytes, original_image) and replace the
direct assignment from original_image.info.get("exif") with a sanitized value
via sanitize_exif_orientation(original_image.info.get("exif")) while preserving
the existing precedence and None handling.
- Around line 515-544: The cached_preview path lacks the EXIF orientation guard
applied to the master float_image; update the cached_preview branch to check the
existing orientation variable and, when orientation != 1, call
apply_orientation_to_np on preview_arr (or on the uint8 buffer before astype) so
the preview matches the oriented master, then convert to float32/scale into
loaded_float_preview; reference cached_preview, preview_arr,
loaded_float_preview, orientation, and apply_orientation_to_np to locate and
implement the change.
🧹 Nitpick comments (1)
faststack/app.py (1)
766-766: Drop redundant{e}inlog.exception(...)messages.
log.exceptionalready appends exception context/trace; keeping{e}adds noise.♻️ Suggested tweak
- log.exception(f"Unexpected error during save: {e}") + log.exception("Unexpected error during save") ... - log.exception(f"execute_crop: Unexpected error during save: {e}") + log.exception("execute_crop: Unexpected error during save") ... - log.exception(f"quick_auto_levels: Unexpected error during save: {e}") + log.exception("quick_auto_levels: Unexpected error during save") ... - log.exception(f"quick_auto_white_balance: Unexpected error during save: {e}") + log.exception("quick_auto_white_balance: Unexpected error during save")Also applies to: 3691-3691, 3869-3869, 3946-3946
| # --- Create Float Preview --- | ||
| # Use the cached, display-sized preview if available to speed up | ||
| if cached_preview: | ||
| # cached_preview.buffer is uint8 | ||
| preview_arr = np.frombuffer(cached_preview.buffer, dtype=np.uint8).reshape( | ||
| (cached_preview.height, cached_preview.width, 3) | ||
| ) | ||
| preview_arr = np.frombuffer( | ||
| cached_preview.buffer, dtype=np.uint8 | ||
| ).reshape((cached_preview.height, cached_preview.width, 3)) | ||
|
|
||
| # IMPORTANT: The cached_preview coming from the viewer is already "cooked" | ||
| # (it has Color Management / Saturation applied). | ||
| # Our editor expects a "raw" float buffer (non-managed) as its starting point for _apply_edits. | ||
| # To prevent a color "pop" when edits start, we have two choices: | ||
| # 1. "Un-cook" the preview (expensive/inaccurate). | ||
| # 2. Use the cooked preview for the VERY FIRST frame, but immediately | ||
| # re-render from the master float_image in the background. | ||
| # Since we already apply EXIF orientation to the master float_image above, | ||
| # we should also ensure the preview_arr matches orientation if it doesn't already. | ||
| # Generally, the Prefetcher already applies orientation to the cached preview. | ||
| loaded_float_preview = preview_arr.astype(np.float32) / 255.0 | ||
| else: | ||
| # Downscale from float_image | ||
| # Simple striding for speed or creating a PIL thumbnail from original? | ||
| # PIL thumbnail is faster and better quality usually. | ||
| # Downscale from float_image (which now has orientation applied) | ||
| thumb = loaded_original.copy() | ||
| thumb.thumbnail((1920, 1080)) | ||
| thumb_rgb = thumb.convert("RGB") | ||
| loaded_float_preview = np.array(thumb_rgb).astype(np.float32) / 255.0 | ||
|
|
||
| # If we applied orientation to the original, the thumbnail will already be correct | ||
| # because we derived it from loaded_original AFTER exif_transpose. | ||
| # If we derived from cached_preview, we might still need to apply orientation. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "exif|orientation|exif_transpose|apply_orientation|apply_exif_orientation" faststack/imaging/prefetch.pyRepository: AlanRockefeller/faststack
Length of output: 1292
🏁 Script executed:
# Get the broader context of the code in editor.py around lines 515-544
head -550 faststack/imaging/editor.py | tail -80Repository: AlanRockefeller/faststack
Length of output: 4603
🏁 Script executed:
# Search for orientation-related code in editor.py
rg -n "orientation|exif" faststack/imaging/editor.py | head -30Repository: AlanRockefeller/faststack
Length of output: 2041
🏁 Script executed:
# Check what cached_preview is and how it's populated
rg -n "cached_preview" faststack/imaging/editor.py | head -20Repository: AlanRockefeller/faststack
Length of output: 1054
🏁 Script executed:
# Check where cached_preview is created and passed to the editor
rg -n "DecodedImage\|cached_preview.*=" faststack/imaging/editor.py | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Search for where cached_preview is set/created in prefetch or other sources
rg -n "DecodedImage.*=" faststack/imaging/prefetch.py | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Check the full load_image method signature and see all parameters available
sed -n '400,430p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 1178
🏁 Script executed:
# Verify if orientation variable is in scope when cached_preview is processed
sed -n '490,525p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 1954
🏁 Script executed:
# Check if apply_orientation_to_np is imported in editor.py
rg -n "apply_orientation_to_np" faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 229
🏁 Script executed:
# Check what's imported at the top of editor.py
head -50 faststack/imaging/editor.py | grep -n "import\|from"Repository: AlanRockefeller/faststack
Length of output: 652
Add explicit orientation check for cached_preview to match master float_image handling.
The prefetcher generally applies EXIF orientation to cached previews during decode, but the editor code acknowledges this with "generally"—indicating it's not guaranteed. The master float_image is explicitly checked and oriented (line 504), but the cached_preview path has no corresponding check or fallback. Since orientation is already calculated and apply_orientation_to_np is imported, add an explicit orientation guard to ensure preview_arr is oriented when the master buffer is.
🔧 Suggested fix
if cached_preview:
# cached_preview.buffer is uint8
preview_arr = np.frombuffer(
cached_preview.buffer, dtype=np.uint8
).reshape((cached_preview.height, cached_preview.width, 3))
+ # Ensure preview matches master float_image orientation
+ if orientation > 1:
+ preview_arr = apply_orientation_to_np(preview_arr, orientation)🤖 Prompt for AI Agents
In `@faststack/imaging/editor.py` around lines 515 - 544, The cached_preview path
lacks the EXIF orientation guard applied to the master float_image; update the
cached_preview branch to check the existing orientation variable and, when
orientation != 1, call apply_orientation_to_np on preview_arr (or on the uint8
buffer before astype) so the preview matches the oriented master, then convert
to float32/scale into loaded_float_preview; reference cached_preview,
preview_arr, loaded_float_preview, orientation, and apply_orientation_to_np to
locate and implement the change.
| # Determine EXIF for sidecar - prefer source EXIF (from paired JPEG) | ||
| exif_bytes = None | ||
| if transforms_applied: | ||
| # Use sanitized EXIF (orientation reset to 1) | ||
| exif_bytes = self._get_sanitized_exif_bytes() | ||
| elif self._source_exif_bytes: | ||
| # Use cached source EXIF from paired JPEG | ||
| exif_bytes = self._source_exif_bytes | ||
| # Must sanitize orientation because we baked it on load! | ||
| exif_bytes = sanitize_exif_orientation(self._source_exif_bytes) | ||
| elif self.original_image: | ||
| # Fallback to current image's EXIF (may be empty for TIFFs) | ||
| exif_bytes = self.original_image.info.get('exif') | ||
| exif_bytes = self.original_image.info.get("exif") | ||
|
|
There was a problem hiding this comment.
Sanitize EXIF for developed sidecar even when no transforms.
At Line 1721 you fall back to original_image.info["exif"] without sanitizing orientation, but orientation is already baked on load. This can reintroduce a non‑1 Orientation tag and cause double-rotation in viewers. Please sanitize here as well.
🐛 Proposed fix
- elif self.original_image:
- # Fallback to current image's EXIF (may be empty for TIFFs)
- exif_bytes = self.original_image.info.get("exif")
+ elif self.original_image:
+ # Fallback to current image's EXIF (may be empty for TIFFs)
+ exif_bytes = sanitize_exif_orientation(self.original_image.info.get("exif"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Determine EXIF for sidecar - prefer source EXIF (from paired JPEG) | |
| exif_bytes = None | |
| if transforms_applied: | |
| # Use sanitized EXIF (orientation reset to 1) | |
| exif_bytes = self._get_sanitized_exif_bytes() | |
| elif self._source_exif_bytes: | |
| # Use cached source EXIF from paired JPEG | |
| exif_bytes = self._source_exif_bytes | |
| # Must sanitize orientation because we baked it on load! | |
| exif_bytes = sanitize_exif_orientation(self._source_exif_bytes) | |
| elif self.original_image: | |
| # Fallback to current image's EXIF (may be empty for TIFFs) | |
| exif_bytes = self.original_image.info.get('exif') | |
| exif_bytes = self.original_image.info.get("exif") | |
| # Determine EXIF for sidecar - prefer source EXIF (from paired JPEG) | |
| exif_bytes = None | |
| if transforms_applied: | |
| # Use sanitized EXIF (orientation reset to 1) | |
| exif_bytes = self._get_sanitized_exif_bytes() | |
| elif self._source_exif_bytes: | |
| # Use cached source EXIF from paired JPEG | |
| # Must sanitize orientation because we baked it on load! | |
| exif_bytes = sanitize_exif_orientation(self._source_exif_bytes) | |
| elif self.original_image: | |
| # Fallback to current image's EXIF (may be empty for TIFFs) | |
| exif_bytes = sanitize_exif_orientation(self.original_image.info.get("exif")) |
🤖 Prompt for AI Agents
In `@faststack/imaging/editor.py` around lines 1710 - 1722, The fallback branch
that assigns exif_bytes from original_image.info.get("exif") can leave a non-1
Orientation tag; update the logic so that when using original_image's EXIF you
pass it through sanitize_exif_orientation before assigning to exif_bytes. Locate
the block around transforms_applied / _source_exif_bytes handling
(methods/variables: _get_sanitized_exif_bytes, _source_exif_bytes,
original_image) and replace the direct assignment from
original_image.info.get("exif") with a sanitized value via
sanitize_exif_orientation(original_image.info.get("exif")) while preserving the
existing precedence and None handling.
1 participant
Fix some minor bugs
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.