mirror of
https://github.com/immich-app/immich
synced 2026-08-15 13:03:57 +00:00
fix(mobile): stop long images squishing on ios (#29367)
* fix(mobile): stop long images squishing on ios * fix(mobile): bound the original to 16384 with fast resize * refactor(mobile): reuse the shared request options for the original * fix(mobile): fix squished long image previews on ios * fix(mobile): retry with exact resize when fast resize overshoots the texture bound * fix(mobile): clamp preview targets to a minimum of one pixel * fix nullable cast flagged by the stricter lints
This commit is contained in:
parent
099cb25ce0
commit
00e813ba84
4 changed files with 124 additions and 9 deletions
|
|
@ -20,6 +20,7 @@ class LocalImageApiImpl: LocalImageApi {
|
|||
requestOptions.version = .current
|
||||
return requestOptions
|
||||
}()
|
||||
private static let maxPixelSize: CGFloat = 16384
|
||||
|
||||
private static let registry = RequestRegistry<ImageRequest>()
|
||||
|
||||
|
|
@ -108,11 +109,16 @@ class LocalImageApiImpl: LocalImageApi {
|
|||
]))
|
||||
}
|
||||
|
||||
let isOriginal = !(width > 0 && height > 0)
|
||||
let targetSize = isOriginal
|
||||
? CGSize(width: Self.maxPixelSize, height: Self.maxPixelSize)
|
||||
: CGSize(width: Double(width), height: Double(height))
|
||||
let contentMode: PHImageContentMode = isOriginal ? .aspectFit : .aspectFill
|
||||
var image: UIImage?
|
||||
Self.imageManager.requestImage(
|
||||
for: asset,
|
||||
targetSize: width > 0 && height > 0 ? CGSize(width: Double(width), height: Double(height)) : PHImageManagerMaximumSize,
|
||||
contentMode: .aspectFill,
|
||||
targetSize: targetSize,
|
||||
contentMode: contentMode,
|
||||
options: Self.requestOptions,
|
||||
resultHandler: { (_image, info) -> Void in
|
||||
image = _image
|
||||
|
|
@ -123,12 +129,38 @@ class LocalImageApiImpl: LocalImageApi {
|
|||
return request.completion(ImageProcessing.cancelledResult)
|
||||
}
|
||||
|
||||
guard let image = image,
|
||||
let cgImage = image.cgImage else {
|
||||
guard let fastImage = image,
|
||||
var cgImage = fastImage.cgImage else {
|
||||
Self.registry.remove(requestId: requestId)
|
||||
return request.completion(.failure(PigeonError(code: "", message: "Could not get pixel data for \(assetId)", details: nil)))
|
||||
}
|
||||
|
||||
// .fast can return larger than the target, so retry with .exact to guarantee the bound.
|
||||
if max(cgImage.width, cgImage.height) > Int(Self.maxPixelSize) {
|
||||
let exactOptions = Self.requestOptions.copy() as! PHImageRequestOptions
|
||||
exactOptions.resizeMode = .exact
|
||||
image = nil
|
||||
Self.imageManager.requestImage(
|
||||
for: asset,
|
||||
targetSize: targetSize,
|
||||
contentMode: contentMode,
|
||||
options: exactOptions,
|
||||
resultHandler: { (_image, info) -> Void in
|
||||
image = _image
|
||||
}
|
||||
)
|
||||
|
||||
if request.isCancelled {
|
||||
return request.completion(ImageProcessing.cancelledResult)
|
||||
}
|
||||
|
||||
guard let exactImage = image?.cgImage else {
|
||||
Self.registry.remove(requestId: requestId)
|
||||
return request.completion(.failure(PigeonError(code: "", message: "Could not resize image for \(assetId)", details: nil)))
|
||||
}
|
||||
cgImage = exactImage
|
||||
}
|
||||
|
||||
if request.isCancelled {
|
||||
return request.completion(ImageProcessing.cancelledResult)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -167,6 +167,8 @@ ImageProvider getFullImageProvider(
|
|||
size: size,
|
||||
assetType: asset.type,
|
||||
isAnimated: asset.isAnimatedImage,
|
||||
width: asset.width,
|
||||
height: asset.height,
|
||||
checksum: asset.checksum,
|
||||
);
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -1,3 +1,5 @@
|
|||
import 'dart:math' as math;
|
||||
|
||||
import 'package:flutter/foundation.dart';
|
||||
import 'package:flutter/widgets.dart';
|
||||
import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
|
||||
|
|
@ -8,6 +10,9 @@ import 'package:immich_mobile/presentation/widgets/images/image_provider.dart';
|
|||
import 'package:immich_mobile/presentation/widgets/images/one_frame_multi_image_stream_completer.dart';
|
||||
import 'package:immich_mobile/presentation/widgets/timeline/constants.dart';
|
||||
|
||||
// iOS GPU textures max out at 16384px; larger images squish.
|
||||
const _kMaxPixelSize = 16384;
|
||||
|
||||
class LocalThumbProvider extends CancellableImageProvider<LocalThumbProvider>
|
||||
with CancellableImageProviderMixin<LocalThumbProvider> {
|
||||
final String id;
|
||||
|
|
@ -62,6 +67,8 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
|
|||
final Size size;
|
||||
final AssetType assetType;
|
||||
final bool isAnimated;
|
||||
final int? width;
|
||||
final int? height;
|
||||
final String? checksum;
|
||||
|
||||
LocalFullImageProvider({
|
||||
|
|
@ -69,9 +76,30 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
|
|||
required this.assetType,
|
||||
required this.size,
|
||||
required this.isAnimated,
|
||||
this.width,
|
||||
this.height,
|
||||
this.checksum,
|
||||
});
|
||||
|
||||
Size _previewTarget(double dpr, bool previewIsFinal) =>
|
||||
previewTargetSize(size.width * dpr, size.height * dpr, width, height, previewIsFinal: previewIsFinal);
|
||||
|
||||
// Use an aspect-correct target when aspectFill would exceed the texture limit.
|
||||
@visibleForTesting
|
||||
static Size previewTargetSize(double boxW, double boxH, int? width, int? height, {required bool previewIsFinal}) {
|
||||
if (width == null || height == null || width <= 0 || height <= 0) {
|
||||
return Size(boxW, boxH);
|
||||
}
|
||||
final imgLong = math.max(width, height).toDouble();
|
||||
final coverLong = imgLong * math.max(boxW / width, boxH / height);
|
||||
if (coverLong <= _kMaxPixelSize) {
|
||||
return Size(boxW, boxH);
|
||||
}
|
||||
final bound = previewIsFinal ? _kMaxPixelSize.toDouble() : math.max(boxW, boxH);
|
||||
final scale = math.min(1.0, bound / imgLong);
|
||||
return Size(math.max(1.0, width * scale), math.max(1.0, height * scale));
|
||||
}
|
||||
|
||||
@override
|
||||
Future<LocalFullImageProvider> obtainKey(ImageConfiguration configuration) {
|
||||
return SynchronousFuture(this);
|
||||
|
|
@ -118,7 +146,7 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
|
|||
final devicePixelRatio = PlatformDispatcher.instance.views.first.devicePixelRatio;
|
||||
var request = this.request = LocalImageRequest(
|
||||
localId: key.id,
|
||||
size: Size(size.width * devicePixelRatio, size.height * devicePixelRatio),
|
||||
size: _previewTarget(devicePixelRatio, !loadOriginal),
|
||||
assetType: key.assetType,
|
||||
);
|
||||
yield* loadRequest(request, decode, isFinal: !loadOriginal);
|
||||
|
|
@ -146,7 +174,7 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
|
|||
final devicePixelRatio = PlatformDispatcher.instance.views.first.devicePixelRatio;
|
||||
final previewRequest = request = LocalImageRequest(
|
||||
localId: key.id,
|
||||
size: Size(size.width * devicePixelRatio, size.height * devicePixelRatio),
|
||||
size: _previewTarget(devicePixelRatio, false),
|
||||
assetType: key.assetType,
|
||||
);
|
||||
yield* loadRequest(previewRequest, decode, isFinal: false);
|
||||
|
|
@ -173,11 +201,16 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
|
|||
return true;
|
||||
}
|
||||
if (other is LocalFullImageProvider) {
|
||||
return id == other.id && size == other.size && isAnimated == other.isAnimated && checksum == other.checksum;
|
||||
return id == other.id &&
|
||||
size == other.size &&
|
||||
isAnimated == other.isAnimated &&
|
||||
width == other.width &&
|
||||
height == other.height &&
|
||||
checksum == other.checksum;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
@override
|
||||
int get hashCode => Object.hash(id, size, isAnimated, checksum);
|
||||
int get hashCode => Object.hash(id, size, isAnimated, width, height, checksum);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -22,6 +22,54 @@ void main() {
|
|||
loads = 0;
|
||||
});
|
||||
|
||||
group('LocalFullImageProvider.previewTargetSize', () {
|
||||
const box = Size(1179, 2556);
|
||||
const cases = <(String, Size, int?, int?, bool, Size)>[
|
||||
('normal', box, 4032, 3024, true, box),
|
||||
('missing dimensions', box, null, null, true, box),
|
||||
('invalid dimensions', box, 1000, 0, true, box),
|
||||
('long final preview', box, 1000, 30000, true, Size(16384 / 30, 16384)),
|
||||
('long first preview', box, 30000, 1000, false, Size(2556, 2556 / 30)),
|
||||
('ultra-thin preview', box, 10, 50000, false, Size(1, 2556)),
|
||||
('small source', box, 50, 2000, true, Size(50, 2000)),
|
||||
('at limit', Size(16384, 100), 1000, 1000, true, Size(16384, 100)),
|
||||
('over limit', Size(16385, 100), 1000, 1000, true, Size(1000, 1000)),
|
||||
];
|
||||
|
||||
for (final (name, box, width, height, previewIsFinal, expected) in cases) {
|
||||
test(name, () {
|
||||
final actual = LocalFullImageProvider.previewTargetSize(
|
||||
box.width,
|
||||
box.height,
|
||||
width,
|
||||
height,
|
||||
previewIsFinal: previewIsFinal,
|
||||
);
|
||||
expect(actual.width, closeTo(expected.width, 1e-6));
|
||||
expect(actual.height, closeTo(expected.height, 1e-6));
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
group('LocalFullImageProvider equality', () {
|
||||
LocalFullImageProvider make({int? width, int? height}) => LocalFullImageProvider(
|
||||
id: 'a',
|
||||
assetType: AssetType.image,
|
||||
size: const Size(100, 200),
|
||||
isAnimated: false,
|
||||
width: width,
|
||||
height: height,
|
||||
);
|
||||
|
||||
test('uses dimensions in the cache key', () {
|
||||
final a = make(width: 100, height: 200);
|
||||
final b = make(width: 100, height: 200);
|
||||
expect(a, b);
|
||||
expect(a.hashCode, b.hashCode);
|
||||
expect(a == make(width: 200, height: 100), isFalse);
|
||||
});
|
||||
});
|
||||
|
||||
group('LocalThumbProvider caching', () {
|
||||
test('editing on device re-renders the thumbnail', () {
|
||||
cache.putIfAbsent(LocalThumbProvider(id: 'asset-1', assetType: AssetType.image, checksum: 'before'), load);
|
||||
|
|
@ -58,7 +106,7 @@ void main() {
|
|||
test('thumbnails are keyed by the asset checksum', () {
|
||||
final asset = LocalAssetFactory.create().copyWith(checksum: 'abc');
|
||||
|
||||
final provider = getThumbnailImageProvider(asset) as LocalThumbProvider;
|
||||
final provider = getThumbnailImageProvider(asset)! as LocalThumbProvider;
|
||||
|
||||
expect(provider.checksum, 'abc');
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue