Skip to content

Commit

Permalink
Do not try to load main/default asset image if only higher-res varian…
Browse files Browse the repository at this point in the history 
…ts exist (flutter#128143)

Fixes flutter#127090.

flutter#122505 did a few things to speed up the first asset load that a flutter app performs. One of those things was to not include the main asset in its own list of variants in the asset manifest. The idea was that we know that the main asset always exists, so including it in its list of variants is a waste of storage space and loading time (even if the cost was tiny).

However, the assumption that the main asset always exists is wrong. From [Declaring resolution-aware image assets](https://docs.flutter.dev/ui/assets-and-images#resolution-aware), which predates flutter#122505:

> Each entry in the asset section of the pubspec.yaml should correspond to a real file, with the exception of the main asset entry. If the main asset entry doesn�t correspond to a real file, then the asset with the lowest resolution is used as the fallback for devices with device pixel ratios below that resolution. The entry should still be included in the pubspec.yaml manifest, however.

For example, it's valid to declare `assets/image.png` as an asset even if only `assets/3x/image.png` exists on disk.

This fix restores older behavior of including a main asset as a variant of itself in the manifest if it exists.

This fix also includes a non-user-visible behavior change:
* `"dpr"` is no longer a required field in the asset manifest's underlying structure. For the main asset entry, we do not include `"dpr"`. It makes less sense for the tool to decide what the default target dpr for an image should be. This should be left to the framework.
  • Loading branch information
@andrewkolos
andrewkolos committed Jun 7, 2023
1 parent 682aa38 commit 5007e43
Show file tree
Hide file tree
Showing 8 changed files with 382 additions and 223 deletions.
6 changes: 1 addition & 5 deletions packages/flutter/lib/src/painting/image_resolution.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,10 @@ class AssetImage extends AssetBundleImageProvider {
}

AssetMetadata _chooseVariant(String mainAssetKey, ImageConfiguration config, Iterable<AssetMetadata>? candidateVariants) {
if (candidateVariants == null) {
if (candidateVariants == null || candidateVariants.isEmpty || config.devicePixelRatio == null) {
return AssetMetadata(key: mainAssetKey, targetDevicePixelRatio: null, main: true);
}

if (config.devicePixelRatio == null) {
return candidateVariants.firstWhere((AssetMetadata variant) => variant.main);
}

final SplayTreeMap<double, AssetMetadata> candidatesByDevicePixelRatio =
SplayTreeMap<double, AssetMetadata>();
for (final AssetMetadata candidate in candidateVariants) {
Expand Down
23 changes: 11 additions & 12 deletions packages/flutter/lib/src/services/asset_manifest.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ abstract class AssetManifest {
/// Retrieves metadata about an asset and its variants. Returns null if the
/// key was not found in the asset manifest.
///
/// This method considers a main asset to be a variant of itself and
/// includes it in the returned list.
/// This method considers a main asset to be a variant of itself. The returned
/// list will include it if it exists.
List<AssetMetadata>? getAssetVariants(String key);
}

Expand Down Expand Up @@ -73,22 +73,21 @@ class _AssetManifestBin implements AssetManifest {
}
_typeCastedData[key] = ((_data[key] ?? <Object?>[]) as Iterable<Object?>)
.cast<Map<Object?, Object?>>()
.map((Map<Object?, Object?> data) => AssetMetadata(
.map((Map<Object?, Object?> data) {
final String asset = data['asset']! as String;
final Object? dpr = data['dpr'];
return AssetMetadata(
key: data['asset']! as String,
targetDevicePixelRatio: data['dpr']! as double,
main: false,
))
targetDevicePixelRatio: dpr as double?,
main: key == asset,
);
})
.toList();

_data.remove(key);
}

final AssetMetadata mainAsset = AssetMetadata(key: key,
targetDevicePixelRatio: null,
main: true
);

return <AssetMetadata>[mainAsset, ..._typeCastedData[key]!];
return _typeCastedData[key]!;
}

@override
Expand Down
22 changes: 11 additions & 11 deletions packages/flutter/test/painting/image_resolution_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ void main() {
final Map<String, List<Map<Object?, Object?>>> assetBundleMap =
<String, List<Map<Object?, Object?>>>{};

assetBundleMap[mainAssetPath] = <Map<Object?, Object?>>[];

final AssetImage assetImage = AssetImage(
mainAssetPath,
bundle: TestAssetBundle(assetBundleMap),
Expand Down Expand Up @@ -160,15 +158,17 @@ void main() {
double chosenAssetRatio,
String expectedAssetPath,
) {
final Map<String, List<Map<Object?, Object?>>> assetBundleMap =
<String, List<Map<Object?, Object?>>>{};

final Map<Object?, Object?> mainAssetVariantManifestEntry = <Object?, Object?>{};
mainAssetVariantManifestEntry['asset'] = variantPath;
mainAssetVariantManifestEntry['dpr'] = 3.0;
assetBundleMap[mainAssetPath] = <Map<Object?, Object?>>[mainAssetVariantManifestEntry];

final TestAssetBundle testAssetBundle = TestAssetBundle(assetBundleMap);
const Map<String, List<Map<Object?, Object?>>> assetManifest =
<String, List<Map<Object?, Object?>>>{
'assets/normalFolder/normalFile.png': <Map<Object?, Object?>>[
<Object?, Object?>{'asset': 'assets/normalFolder/normalFile.png'},
<Object?, Object?>{
'asset': 'assets/normalFolder/3.0x/normalFile.png',
'dpr': 3.0
},
]
};
final TestAssetBundle testAssetBundle = TestAssetBundle(assetManifest);

final AssetImage assetImage = AssetImage(
mainAssetPath,
Expand Down
40 changes: 37 additions & 3 deletions packages/flutter/test/services/asset_manifest_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:convert';

import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';

Expand All @@ -11,12 +13,19 @@ class TestAssetBundle extends AssetBundle {
if (key == 'AssetManifest.smcbin') {
final Map<String, List<Object>> binManifestData = <String, List<Object>>{
'assets/foo.png': <Object>[
<String, Object>{
'asset': 'assets/foo.png',
},
<String, Object>{
'asset': 'assets/2x/foo.png',
'dpr': 2.0
}
},
],
'assets/bar.png': <Object>[
<String, Object>{
'asset': 'assets/bar.png',
},
],
'assets/bar.png': <Object>[],
};

final ByteData data = const StandardMessageCodec().encodeMessage(binManifestData)!;
Expand All @@ -32,7 +41,6 @@ class TestAssetBundle extends AssetBundle {
}
}


void main() {
TestWidgetsFlutterBinding.ensureInitialized();

Expand Down Expand Up @@ -65,3 +73,29 @@ void main() {
expect(manifest.getAssetVariants('invalid asset key'), isNull);
});
}

String createAssetManifestJson(Map<String, List<AssetMetadata>> manifest) {
final Map<Object, Object> jsonObject = manifest.map(
(String key, List<AssetMetadata> value) {
final List<String> variants = value.map((AssetMetadata e) => e.key).toList();
return MapEntry<String, List<String>>(key, variants);
}
);

return json.encode(jsonObject);
}

ByteData createAssetManifestSmcBin(Map<String, List<AssetMetadata>> manifest) {
final Map<Object, Object> smcObject = manifest.map(
(String key, List<AssetMetadata> value) {
final List<Object> variants = value.map((AssetMetadata variant) => <String, Object?>{
'asset': variant.key,
'dpr': variant.targetDevicePixelRatio,
}).toList();

return MapEntry<String, List<Object>>(key, variants);
}
);

return const StandardMessageCodec().encodeMessage(smcObject)!;
}
32 changes: 15 additions & 17 deletions packages/flutter/test/widgets/image_resolution_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,15 @@ import '../image_data.dart';
ByteData testByteData(double scale) => ByteData(8)..setFloat64(0, scale);
double scaleOf(ByteData data) => data.getFloat64(0);

final Map<Object?, Object?> testManifest = json.decode('''
{
"assets/image.png" : [
{"asset": "assets/1.5x/image.png", "dpr": 1.5},
{"asset": "assets/2.0x/image.png", "dpr": 2.0},
{"asset": "assets/3.0x/image.png", "dpr": 3.0},
{"asset": "assets/4.0x/image.png", "dpr": 4.0}
]
}
''') as Map<Object?, Object?>;
final Map<Object?, Object?> testManifest = <Object?, Object?>{
'assets/image.png' : <Map<String, Object>>[
<String, String>{'asset': 'assets/image.png'},
<String, Object>{'asset': 'assets/1.5x/image.png', 'dpr': 1.5},
<String, Object>{'asset': 'assets/2.0x/image.png', 'dpr': 2.0},
<String, Object>{'asset': 'assets/3.0x/image.png', 'dpr': 3.0},
<String, Object>{'asset': 'assets/4.0x/image.png', 'dpr': 4.0}
],
};

class TestAssetBundle extends CachingAssetBundle {
TestAssetBundle({ required Map<Object?, Object?> manifest }) {
Expand Down Expand Up @@ -303,13 +302,12 @@ void main() {
// if higher resolution assets are not available we will pick the best
// available.
testWidgets('Low-resolution assets', (WidgetTester tester) async {
final Map<Object?, Object?> manifest = json.decode('''
{
"assets/image.png" : [
{"asset": "assets/1.5x/image.png", "dpr": 1.5}
]
}
''') as Map<Object?, Object?>;
const Map<Object?, Object?> manifest = <Object?, Object?>{
'assets/image.png': <Map<String, Object>>[
<String, Object>{'asset': 'assets/image.png'},
<String, Object>{'asset': 'assets/1.5x/image.png', 'dpr': 1.5},
],
};
final AssetBundle bundle = TestAssetBundle(manifest: manifest);

Future<void> testRatio({required double ratio, required double expectedScale}) async {
Expand Down
17 changes: 7 additions & 10 deletions packages/flutter_tools/lib/src/asset.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ class ManifestAssetBundle implements AssetBundle {
DateTime? _lastBuildTimestamp;

// We assume the main asset is designed for a device pixel ratio of 1.0.
static const double _defaultResolution = 1.0;
static const String _kAssetManifestJsonFilename = 'AssetManifest.json';
static const String _kAssetManifestBinFilename = 'AssetManifest.smcbin';

Expand Down Expand Up @@ -688,7 +687,7 @@ class ManifestAssetBundle implements AssetBundle {
DevFSByteContent _createAssetManifestBinary(
Map<String, List<String>> assetManifest
) {
double parseScale(String key) {
double? parseScale(String key) {
final Uri assetUri = Uri.parse(key);
String directoryPath = '';
if (assetUri.pathSegments.length > 1) {
Expand All @@ -699,7 +698,8 @@ class ManifestAssetBundle implements AssetBundle {
if (match != null && match.groupCount > 0) {
return double.parse(match.group(1)!);
}
return _defaultResolution;

return null;
}

final Map<String, dynamic> result = <String, dynamic>{};
Expand All @@ -708,15 +708,12 @@ class ManifestAssetBundle implements AssetBundle {
final List<dynamic> resultVariants = <dynamic>[];
final List<String> entries = (manifestEntry.value as List<dynamic>).cast<String>();
for (final String variant in entries) {
if (variant == manifestEntry.key) {
// With the newer binary format, don't include the main asset in it's
// list of variants. This reduces parsing time at runtime.
continue;
}
final Map<String, dynamic> resultVariant = <String, dynamic>{};
final double variantDevicePixelRatio = parseScale(variant);
final double? variantDevicePixelRatio = parseScale(variant);
resultVariant['asset'] = variant;
resultVariant['dpr'] = variantDevicePixelRatio;
if (variantDevicePixelRatio != null) {
resultVariant['dpr'] = variantDevicePixelRatio;
}
resultVariants.add(resultVariant);
}
result[manifestEntry.key] = resultVariants;
Expand Down
Loading

0 comments on commit 5007e43

Please sign in to comment.