Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVGs loaded from asset bundles aren't cached #33

Closed
SteveAlexander opened this issue Jul 3, 2018 · 16 comments
Closed

SVGs loaded from asset bundles aren't cached #33

SteveAlexander opened this issue Jul 3, 2018 · 16 comments

Comments

@SteveAlexander
Copy link

I'm seeing that SVGs loaded as assets don't get cached at all, because key.bundle.load(key.name) doesn't cache, whereas key.bundle.loadString(key.name) does.

If I'm using SVGs to render icons, this means that the asset is loaded repeatedly as the app is used.

Would it make sense to use loadString instead?

@dnfield
Copy link
Owner

dnfield commented Jul 3, 2018

I'm not opposed to using loadString, as there are methods that should support taking in a string. However, there is caching that happens in this library itself - so you shouldn't really be loading assets more than once, unless you're loading the same asset multiple times with different colors (color becomes part of the picture and is part of the key for the PictureCache).

You could, of course, also choose to load and cache your assets in any other way and just use SvgPicture.string or SvgPicture.memory.

@SteveAlexander
Copy link
Author

Thanks. I've ended up using SvgPicture.string in my code, and loading the asset myself to take advantage of the AssetBundle's cacheing.

But, I do know that before I did this, the assets were being loaded pretty much on every widget render. I added debug logging to the code in asset_bundle.dart and platform_messages.dart, and I could see that the svg files were loaded repeatedly.

What got me looking into this in the first place was that my app was regularly crashing when I switched pages too quickly — it seems that there is a race condition of sorts in _sendPlatformMessage which is causing it to sometimes return a null reply rather than some ByteData. This would cause PlatformAssetBundle.load to complain it was "Unable to load asset", and crash the app.

@SteveAlexander
Copy link
Author

To be clear, the previous iteration of my code was using SvgPicture.asset as a child of some Widget. Is this meant to be automatically cached?

@dnfield
Copy link
Owner

dnfield commented Jul 4, 2018

That seems strange. Can you provide an example to reproduce this behavior?

There's caching logic that should kick in after the parsing is done. It cache's a Picture object that should be keyed based on the asset name. It shouldn't even have to actually get the Asset again once that Picture is cached.

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

I lost track of this for a while, sorry.

I see the problem now. I was incorrectly assuming that the caching I was doing would work in this case, when it clearly doesn't. This should be fixable without requiring your work around.

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

Or not...

Can you provide a simple way to reproduce this? I suspect something you're doing in your app ends up clearing the cache unexpectedly.

@SteveAlexander
Copy link
Author

Ok — so I changed back to using

SvgPicture.asset(
        assetPath,
        color: iconColor,
        colorBlendMode: BlendMode.multiply,
        allowDrawingOutsideViewBox: true
      )

I also added a print line in flutter/lib/src/services/asset_bundle.dart

/// An [AssetBundle] that loads resources using platform messages.
class PlatformAssetBundle extends CachingAssetBundle {
  @override
  Future<ByteData> load(String key) async {
    print('PlatformAssetBundle.load($key)');
    final Uint8List encoded = utf8.encoder.convert(Uri(path: Uri.encodeFull(key)).path);
    final ByteData asset =
        await BinaryMessages.send('flutter/assets', encoded.buffer.asByteData());
    if (asset == null)
      throw FlutterError('Unable to load asset: $key');
    return asset;
  }
}

I find that SVGs load repeatedly, as per debug output:


flutter: PlatformAssetBundle.load(assets/home.svg)
flutter: PlatformAssetBundle.load(assets/agenda.svg)
flutter: PlatformAssetBundle.load(assets/home.svg)
flutter: PlatformAssetBundle.load(assets/home.svg)
flutter: PlatformAssetBundle.load(assets/agenda.svg)
flutter: PlatformAssetBundle.load(assets/home.svg)
flutter: PlatformAssetBundle.load(assets/agenda.svg)
flutter: PlatformAssetBundle.load(assets/home.svg)
flutter: PlatformAssetBundle.load(assets/agenda.svg)

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

Right, this is what I'm not able to reproduce. I added some similar print statements (plus a couple others). I onl ever get one PlatformAssetBundle.load(...)

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

Can you share a small app that reproduces this?

@SteveAlexander
Copy link
Author

Ok, I've figured out a part of this.

My code uses final IconThemeData iconTheme = IconTheme.of(context); to get icon theme data, and uses iconTheme.color to get the color to use.

For reasons I don't understand, I'm getting different IconThemeData objects returned from of, and each one has a subtly different color.

Because the picture cache caches based on asset key, color, and blend mode, each time I'm rendering this, I get a cache miss. This results in an asset load, and another picture added to the cache.

It is currently a mystery to me why IconTheme is behaving this way.

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

Ahhh that's actually expected.

I suppose it would make sense to cache the asset for this case - I didn't really have that in mind, but I suppose it'd particularly be beneficial for icons where you want to have different colors with the same icon in multiple places.

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

I'd like to have some way to make this optional though, as I think there are plenty of use cases where you don't want to keep the SVG data around in memory longer than necessary (e.g. load a large SVG string and draw it in only one color).

@SteveAlexander
Copy link
Author

Okay, I figured it out.

_TabStyle is an AnimatedWidget. It animates tabs, from material/tabs.dart.

It lerps the color, and then builds its own child wrapped inside an IconTheme.

So, if the child is an svg, it's going to be loading many of them — one per frame I guess. Each will have a different color.

@SteveAlexander
Copy link
Author

Given that this thing with tabs is so counter intuitive, and that image assets are usually cached in Flutter, maybe it makes sense to have SVG assets cached by default?

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

This would end up happening with any animation involving color changes/lerps then.

Ok. I can buy that. If someone still wants these to not cache, they can use the same work around you're doing to get them to cache in reverse - e.g. load it as a string without caching it and calling SvgPicture.string.

@dnfield
Copy link
Owner

dnfield commented Nov 28, 2018

Fixed in v0.7.0+1 which is 99feec0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants