Improve TurboJPEG detection on Windows and document fallback behavior#58
Improve TurboJPEG detection on Windows and document fallback behavior#58AlanRockefeller merged 2 commits intomainfrom
Conversation
WalkthroughCentralizes TurboJPEG discovery/initialization into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as Consumer (jpeg/prefetcher)
participant TurboMod as faststack.imaging.turbo
participant OS as OS / Env / PATH
participant PyTurbo as PyTurboJPEG
participant Pillow as Pillow
Consumer->>TurboMod: create_turbojpeg()
TurboMod->>OS: build candidate list (env vars, Program Files, PATH, None)
alt Try default loader (None candidate)
TurboMod->>PyTurbo: TurboJPEG() -- no args
alt success
PyTurbo-->>TurboMod: TurboJPEG instance
TurboMod-->>Consumer: (instance, True)
else failure
PyTurbo-->>TurboMod: Exception
TurboMod->>OS: select next candidate
end
else Try explicit DLL candidate
TurboMod->>PyTurbo: TurboJPEG(path)
alt success
PyTurbo-->>TurboMod: TurboJPEG instance
TurboMod-->>Consumer: (instance, True)
else failure
PyTurbo-->>TurboMod: Exception
TurboMod->>OS: try next candidate
end
end
alt all attempts failed or PyTurbo missing
TurboMod-->>Consumer: (None, False)
TurboMod-->>Consumer: log WARNING "Falling back to Pillow"
Consumer->>Pillow: decode via Pillow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ecf316547
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
faststack/imaging/turbo.py (1)
74-102: Consider memoizingcreate_turbojpeg()to avoid duplicate initialization/log noise.
create_turbojpeg()is invoked at multiple module import sites, so initialization can run more than once per process. Caching the result once will avoid repeated loader attempts and repeated warnings.♻️ Proposed change
from __future__ import annotations import logging import os +from functools import lru_cache from pathlib import Path from typing import Optional, Tuple @@ -def create_turbojpeg() -> Tuple[Optional["TurboJPEG"], bool]: +@lru_cache(maxsize=1) +def create_turbojpeg() -> Tuple[Optional["TurboJPEG"], bool]: """Create a TurboJPEG decoder if possible."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/turbo.py` around lines 74 - 102, The create_turbojpeg() function runs heavy initialization and emits logs on each import; cache its result so it runs only once per process by memoizing the call (e.g., use a module-level cache variable or `@functools.lru_cache` on create_turbojpeg) and return the cached (decoder, bool) tuple on subsequent calls; ensure the cache key is constant (no args) and keep existing behavior around TurboJPEG, _candidate_library_paths(), and logging unchanged except that repeated attempts/logs are suppressed after the first successful or failed initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 91-99: Update the README troubleshooting section to match the
actual runtime warning emitted by faststack/imaging/turbo.py: replace the old
lines "Unable to locate turbojpeg library automatically. You may specify the
turbojpeg library path manually." with the exact message produced by turbo.py
("TurboJPEG initialization failed (... location(s) tried). Falling back to
Pillow for JPEG decoding.") and change the explanatory sentence to state that
FastStack could not initialize TurboJPEG from any discovered location and is
using Pillow instead; reference the turbo.py initialization/logging code paths
and the README header "Troubleshooting on Windows" when making the replacement
so the doc and runtime logs are consistent.
---
Nitpick comments:
In `@faststack/imaging/turbo.py`:
- Around line 74-102: The create_turbojpeg() function runs heavy initialization
and emits logs on each import; cache its result so it runs only once per process
by memoizing the call (e.g., use a module-level cache variable or
`@functools.lru_cache` on create_turbojpeg) and return the cached (decoder, bool)
tuple on subsequent calls; ensure the cache key is constant (no args) and keep
existing behavior around TurboJPEG, _candidate_library_paths(), and logging
unchanged except that repeated attempts/logs are suppressed after the first
successful or failed initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f105d85-c5e6-4db4-af6f-b857d751a122
📒 Files selected for processing (5)
README.mdfaststack/imaging/jpeg.pyfaststack/imaging/turbo.pyfaststack/tests/test_turbo.pyfaststack/thumbnail_view/prefetcher.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
faststack/imaging/turbo.py (2)
60-62: NormalizePATHentries before candidate construction.On Windows,
PATHentries can contain surrounding quotes/whitespace; trimming avoids malformed candidates.Proposed change
- for path_dir in os.getenv("PATH", "").split(os.pathsep): - if path_dir: - candidates.append(str(Path(path_dir) / "turbojpeg.dll")) + for raw_dir in os.getenv("PATH", "").split(os.pathsep): + path_dir = raw_dir.strip().strip('"') + if path_dir: + candidates.append(str(Path(path_dir) / "turbojpeg.dll"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/turbo.py` around lines 60 - 62, The loop building DLL candidates from PATH may produce malformed paths when PATH entries contain surrounding quotes or whitespace; in the loop that iterates over os.getenv("PATH", "").split(os.pathsep) (the path_dir variable) trim surrounding quotes and whitespace (e.g., strip() and strip('"')) before constructing Path(path_dir) / "turbojpeg.dll" and appending to candidates so only normalized directory strings are used.
85-88: Narrow constructor error handling to avoid masking unexpected failures.At line 85, catching
Exceptionis broader than needed for load/probe failures. The TurboJPEG constructor is documented to raiseRuntimeErrorwhen unable to locate the library automatically, andOSErrorwhen an invalid library path is provided (ctypes.CDLL failure). Prefer these specific exceptions so unexpected bugs still surface.Proposed change
- except Exception as exc: + except (OSError, RuntimeError) as exc: source = "default loader" if candidate is None else candidate failures.append(f"{source}: {exc}") continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/turbo.py` around lines 85 - 88, Narrow the broad except in the TurboJPEG load loop to only catch the documented constructor failures: replace the generic "except Exception as exc" with "except (RuntimeError, OSError) as exc" so only known library-probing/CTypes errors are appended to failures (referencing the candidate variable and failures list) and let unexpected exceptions propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@faststack/imaging/turbo.py`:
- Around line 60-62: The loop building DLL candidates from PATH may produce
malformed paths when PATH entries contain surrounding quotes or whitespace; in
the loop that iterates over os.getenv("PATH", "").split(os.pathsep) (the
path_dir variable) trim surrounding quotes and whitespace (e.g., strip() and
strip('"')) before constructing Path(path_dir) / "turbojpeg.dll" and appending
to candidates so only normalized directory strings are used.
- Around line 85-88: Narrow the broad except in the TurboJPEG load loop to only
catch the documented constructor failures: replace the generic "except Exception
as exc" with "except (RuntimeError, OSError) as exc" so only known
library-probing/CTypes errors are appended to failures (referencing the
candidate variable and failures list) and let unexpected exceptions propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8d3b3af-1a30-4fdf-b79f-fb4da03b5e1b
📒 Files selected for processing (3)
ChangeLog.mdREADME.mdfaststack/imaging/turbo.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- ChangeLog.md
Summary by CodeRabbit
Documentation
New Features
Tests