Add video support to AsyncImageKit #25000
Conversation
Generated by 🚫 Danger |
| private func avatar(url: URL?) -> some View { | ||
| let processedURL: URL? | ||
| let size = Int(ceil(diameter * UIScreen.main.scale)) | ||
| let size = Int(ceil(diameter * screenScale)) |
There was a problem hiding this comment.
This isn't related to the PR but I came across it during this PR and figured I'd address it.
|
Congratulations on PR number 25,000 🍾 |
jkmassel
force-pushed
the
add/video-support-to-asyncimageview
branch
from
511b461 to
0f8a25c
Compare
|
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29824 | |
| Version | PR #25000 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 0f8a25c | |
| Installation URL | 2cu5dn6hn2qbo |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29824 | |
| Version | PR #25000 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 0f8a25c | |
| Installation URL | 008c0ge2rslm0 |
|
|
||
| let path = self.path(for: key) | ||
|
|
||
| try FileManager.default.createDirectory(at: path.deletingLastPathComponent(), withIntermediateDirectories: true) |
There was a problem hiding this comment.
store it in the disk cache "forever"
I suggest to store these images in the existing URLCache (see urlSessionWithCache). I recently used URLCache store storing thumbnails for Gutenberg patterns, and it worked pretty well – it doesn't have to be a network response. You can always re-create a preview from AVURLAsset if needed. If these are stored on disk with no cleanup mechanism, it will leave lingering files on disk increasing the space the app takes on disk.
There was a problem hiding this comment.
If these are stored on disk with no cleanup mechanism, it will leave lingering files on disk increasing the space the app takes on disk
The cleanup mechanism is that the system will purge anything in the Caches directory if it needs to, and #24972 adds a cache cleaning mechanism that can do it on user request. There's no such great way to do this with URLCache because under the hood it's a sqlite DB in ~/Library/Caches/com.automattic.jetpack/org.automattic.ImageDownloader for the in-memory DB and stores its files in the same directory. Because that's all managed by urlsessiond I'm not sure we can just erase it?
I'm not sure what we lose by storing the files in ~/Library/Caches/image-cache aside from the automatic cache eviction based on size? But again, if the system needs the space it'll just take it back?
There was a problem hiding this comment.
Just realized we could call ImageDownloader.clearURLSessionCache() which is presumably thread-safe so never mind that part. 🤦
It'd be weird/kinda annoying to work it into the cache clearing stuff but it's certainly doable.
There was a problem hiding this comment.
The longer I look at this the less I feel like URLCache should be used as a general-purpose disk cache?
If I want to store something in it, I need to use a URLResponse alongside my data, but do I also need to manually add fake HTTP headers so that URLCache will store it indefinitely? If I just use an empty URLResponse will it store the results at all? Storing bytes on-disk seems much more straightforward in comparison tbh
There was a problem hiding this comment.
I'd be happy to have a custom disk cache with LRU cleanup as an alternative. If you'd like to address it separately, that's OK.
But again, if the system needs the space it'll just take it back?
It's only the last resort. It's best to be a good citizen and not waste space by setting a limit and cleanup up automatically.
| func authorize(_ asset: AVURLAsset) -> AVURLAsset { | ||
|
|
||
| // Don't authorize requests for other domains | ||
| guard asset.url.host() == "public-api.wordpress.com" else { |
There was a problem hiding this comment.
Are the uploaded videos in support tickets hosted at this domain? This condition may not be true for all videos, though. I believe the videos on the site's Media Library are under the site's domain.
That's probably something we can address later, if the current approach works with the support system.
There was a problem hiding this comment.
This is true for the moment, yes.
If we rely on this for more things we'll need to expand it out to i.wp.com (and related domains) but I wanted to keep it small for now and deliberately add more cases as needed.
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
Approved, but caching could be further improved (left a separate comment).
Description
Extracts
AsyncImageKitchanges (and suggestions by @kean) from #24972Testing instructions
I've added SwiftUI Previews covering common cases – if those look good this should be ready to land.