Fix screenshot resolution issue with tolerance-based PAR comparison #1011
Conversation
Changed PAR normalization to use tolerance-based comparison (abs(par - 1.0) < 0.01) instead of strict equality checks. This prevents unintended scaling when PAR is within ±1% of 1.0, fixing common artifacts in WEB-DL content: - 1920x1040 no longer becomes 1924x1040 - 1792x1080 no longer becomes 1799x1080 Still applies scaling for legitimate PAR variations in DVD and anamorphic content.
WalkthroughReplaced exact PAR equality checks with a tolerance-based comparison ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-14T00:20:21.219ZApplied to files:
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (1)
src/region.py (1)
93-93: Good correction to the official Crunchyroll brand name.The change properly updates the service name mapping to use the correct spelling 'Crunchyroll' instead of 'Crunchy Roll'.
If backward compatibility is a concern for existing video files that may use the old 'Crunchy Roll' naming, you could optionally add it as an alias:
'Crunchyroll': 'CR', 'Crave': 'CRAV', 'CRIT': 'CRIT', 'Criterion': 'CRIT', 'Chorki': 'CRKI', 'CRKI': 'CRKI', 'CRKL': 'CRKL', 'Crackle': 'CRKL', +'Crunchy Roll': 'CR',This follows the existing pattern where multiple keys map to the same service code (e.g., 'HBO' and 'HBO Go' both mapping to 'HBO').
|
news? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/takescreens.py (1)
457-469: DVD PAR tolerance changes previous “always scale” behaviorThe new
abs(par - 1.0) < 0.01branch means DVDs with PAR very close to 1.0 will now skip any scaling (w_sar = h_sar = 1). Previously, this code path would always derivew_sar/h_sarfrompar/dar, effectively always applying DVD scaling.Given the earlier review note that “dvd's should always have scaling applied”, please confirm whether you intentionally want DVDs with near‑unity PAR to bypass scaling now. If not, you might want to:
- Keep this tolerance only in the non‑disc path (where WEB‑DLs are handled), or
- Use a tighter/zero tolerance here and retain the old behavior for DVDs.
Separately, the 1920×1040 example in the comment is WEB‑DL‑centric; consider rewording or referencing discs more explicitly to avoid confusion in this DVD helper.
🧹 Nitpick comments (1)
src/takescreens.py (1)
843-854: Unity-PAR tolerance in main screenshot path looks correctThis tolerance block cleanly addresses the WEB‑DL rounding issue:
abs(par - 1.0) < 0.01correctly treats near‑unity PAR (e.g., 1.002) as 1, sow_sar = h_sar = 1and no unintended scaling occurs.- Existing
par < 1/elsebranches remain unchanged for real anamorphic cases.If you want to fine‑tune later, you could centralize the
PAR≈1tolerance (e.g., a shared constant or helper) so both disc and non‑disc paths stay in sync, but as‑is this implementation is sound and matches the PR description.
|
Seems my review was ignored. |
2 participants
Changed PAR normalization to use tolerance-based comparison (abs(par - 1.0) < 0.01) instead of strict equality checks. This prevents unintended scaling when PAR is within ±1% of 1.0, fixing common artifacts in WEB-DL content:
Still applies scaling for legitimate PAR variations in DVD and anamorphic content.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.