Add automatic saliency cropping - #46
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 3/5The saliency crop feature works correctly for typical well-exposed images, but produces silently wrong model inputs for any image whose brightest pixel is below 255, and the hard ML runtime dependency in require is a breaking change for all library consumers. The normalization in detectSaliency() divides by the image's own maximum pixel value instead of the fixed 255.0 that U2NET was trained with. For an underexposed image (max pixel ≈ 180), every channel value is inflated by 1.4× before mean subtraction, pushing the input distribution significantly off the model's training range — silently producing degraded or wrong saliency maps with no error or warning. This affects a meaningful class of real images and is a straightforward fix. Files Needing Attention: src/Image/Image.php — the normalization divisor in detectSaliency(); composer.json — the hard ML runtime dependency Important Files Changed
Reviews (3): Last reviewed commit: "feat: add automatic saliency cropping" | Re-trigger Greptile |
| protected function detectFocus(string $focus): array | ||
| { | ||
| self::$focusDetector ??= pipeline('zero-shot-object-detection'); | ||
|
|
||
| $path = tempnam(sys_get_temp_dir(), 'utopia-image-focus-'); | ||
| if ($path === false) { | ||
| throw new Exception('Failed to create image for focus detection'); | ||
| } | ||
|
|
||
| try { | ||
| if (file_put_contents($path, $this->image->getImageBlob(), LOCK_EX) === false) { | ||
| throw new Exception('Failed to create image for focus detection'); | ||
| } | ||
|
|
||
| $detections = (self::$focusDetector)($path, [$focus], threshold: 0.1, percentage: true, topK: PHP_INT_MAX); | ||
| } finally { | ||
| @unlink($path); | ||
| } |
There was a problem hiding this comment.
If pipeline() or the pipeline invocation throws (e.g., missing or incompatible ONNX Runtime, model not yet downloaded), the exception propagates straight through crop() and kills the entire image operation. Because the focus feature is additive and the caller already supplies a gravity fallback, users likely expect the method to fall back to gravity rather than crash. The PR notes this explicitly as a design choice, but it will cause unhandled exceptions for any caller on Alpine/musl or any environment where the binary artifacts have not been pre-downloaded, even if they are fine with a gravity-based result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 269-286
Comment:
**Hard failure when inference runtime is unavailable**
If `pipeline()` or the pipeline invocation throws (e.g., missing or incompatible ONNX Runtime, model not yet downloaded), the exception propagates straight through `crop()` and kills the entire image operation. Because the `focus` feature is additive and the caller already supplies a gravity fallback, users likely expect the method to fall back to gravity rather than crash. The PR notes this explicitly as a design choice, but it will cause unhandled exceptions for any caller on Alpine/musl or any environment where the binary artifacts have not been pre-downloaded, even if they are fine with a gravity-based result.
How can I resolve this? If you propose a fix, please make it concise.
| { | ||
| self::$focusDetector ??= pipeline('zero-shot-object-detection'); | ||
|
|
||
| $path = tempnam(sys_get_temp_dir(), 'utopia-image-focus-'); |
There was a problem hiding this comment.
tempnam prefix truncated on Windows
The prefix 'utopia-image-focus-' is 19 characters. PHP's tempnam() documentation states that on Windows only the first 3 characters of the prefix are used, so the created file gets the prefix uto instead. Keeping the prefix at 5 characters or fewer would work reliably on all platforms.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 273
Comment:
**`tempnam` prefix truncated on Windows**
The prefix `'utopia-image-focus-'` is 19 characters. PHP's `tempnam()` documentation states that on Windows only the first 3 characters of the prefix are used, so the created file gets the prefix `uto` instead. Keeping the prefix at 5 characters or fewer would work reliably on all platforms.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Image::GRAVITY_AUTOwithout changing the existing three-argumentcrop()APIankane/onnxruntimeto detect visually salient regionsUsage
Model And Runtime
8d10d2f3bb75ae3b6d527c77944fc5e7dcd94b29809d47a739a7a728a912b491resources/models/NOTICE.mdOnnxRuntime\Vendor::checkto root Composer post-install and post-update scriptsPerformance
Measured on an Apple M3 Pro with a 1280x837 JPEG cropped to 180x320:
Native ONNX Runtime and Imagick allocations are not fully represented by PHP's memory counter. This mode is best suited to asynchronous or controlled-concurrency image processing.
Testing
vendor/bin/pint --testcomposer validate --strict --no-check-publishcomposer audit --lockedAll validation passed. PHPUnit reports one existing deprecation.