Reader: Add saliency detection for smart cropping#25451
Conversation
kean
force-pushed
the
fix/image-crop-feed
branch
from
79fe6ba to
9b8925a
Compare
kean
force-pushed
the
fix/image-crop-feed
branch
from
9b8925a to
7512ee3
Compare
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 31794 | |
| Version | PR #25451 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 7242024 | |
| Installation URL | 5pf8mq9ftmiq0 |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 31794 | |
| Version | PR #25451 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 7242024 | |
| Installation URL | 73uqt90jlbvoo |
|
| } | ||
|
|
||
| /// Runs saliency detection serially — one image at a time. | ||
| private actor SaliencyDetector { |
There was a problem hiding this comment.
The SaliencyDetector.detect looks like a pure function. Does this type need to be an actor?
| // Convert to UIKit coordinates (origin at top-left, Y increases downward). | ||
| return CGRect( | ||
| x: union.origin.x, | ||
| y: 1.0 - union.origin.y - union.height, |
There was a problem hiding this comment.
What is this 1.0? Should the y be: image.size.height - union.origin.y - union.height?
| private static let diskURL: URL = { | ||
| let caches = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask)[0] | ||
| return caches.appendingPathComponent("saliency_cache.json") | ||
| }() |
There was a problem hiding this comment.
I'm not sure if it's worth saving the cache to disc. It should not be too bad to recalculate again on the next launch, right? Also, the disk cache will only be usable if the same images appear again in the Reader feed.
| /// When `true`, saliency detection only runs for images whose height exceeds their | ||
| /// width (portrait images). Landscape and square images are displayed immediately | ||
| /// without blocking on detection. Default is `true`. | ||
| public var isSaliencyPortraitOnly = true |
There was a problem hiding this comment.
Nitpick: These two properties can be merged into one property of enum SaliencyDetection { disabled, portraintOnly }
| } else { | ||
| imageView.isHidden = false | ||
| backgroundColor = .clear | ||
| } |
There was a problem hiding this comment.
Probably should call self.setNeedsLayout() here to update the imageView.frame?
3 participants
Fixes CMM-103: Reader: Portrait Reader Images Cropped Too Much.
Before / After