ryanheise / just_audio

Audio Player
1.05k stars 671 forks source link

Duplicated local cache entries when the same AudioSource is loaded multiple times from the app assets #278

Closed ekuleshov closed 3 years ago

ekuleshov commented 3 years ago

Which API doesn't behave as documented, and how does it misbehave? I'm loading a small audio file from the app assets (on Android) on the app startup using the following code:

_player = AudioPlayer();
await _player.setAsset('assets/beep.mp3', preload: true);

Every time app is started the just_autio plugin adds a new copy of the same file into the .../cache/just_audio_asset_cache app data folder.

Minimal reproduction project https://github.com/ekuleshov/just_audio_assets

To Reproduce (i.e. user steps, not code) Steps to reproduce the behavior:

Build, install to Android emulator (I'm using Pixel 4 Android 11). Launch app, kill it, run, kill, run again. Do that several times.

Then look at the app cache in the "Device File Explorer" from Android Studio.

Error messages

N/A

Expected behavior

Expected no new cached files for the same asset file loaded multiple times.

Screenshots image

Desktop (please complete the following information):

N/A

Smartphone (please complete the following information):

Flutter SDK version

[✓] Flutter (Channel stable, 1.22.5, on Mac OS X 10.15.7 19H2 darwin-x64, locale en-CA)
    • Flutter version 1.22.5 at /***/flutter
    • Framework revision 7891006299 (4 weeks ago), 2020-12-10 11:54:40 -0800
    • Engine revision ae90085a84
    • Dart version 2.10.4

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
    • Android SDK at /Users/***/android-sdks
    • Platform android-29, build-tools 29.0.2
    • ANDROID_HOME = /Users/***/android-sdks
    • ANDROID_SDK_ROOT = /Users/***/android-sdks
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.5)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 11.5, Build version 11E608c
    • CocoaPods version 1.10.0

[!] Android Studio (version 4.1)
    • Android Studio at /Applications/Android Studio.app/Contents
    ✗ Flutter plugin not installed; this adds Flutter specific functionality.
    ✗ Dart plugin not installed; this adds Dart specific functionality.
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495)

[✓] Connected device (2 available)
    • sdk gphone x86 (mobile)    • emulator-5554                        • android-x86 • Android 11 (API 30) (emulator)

Additional context

N/A

ryanheise commented 3 years ago

Hi @ekuleshov , I would suggest trying out the proxy_improvements branch which eliminates the asset cache and instead streams assets directly from the asset bundle.

ekuleshov commented 3 years ago

Thnx. I wonder if same issue apply for other resources, like external urls, etc.

ryanheise commented 3 years ago

This would only affect assets. External URLs and files have always been streamed directly from the source to the player, while assets were first extracted from the asset bundle into a file where it could then be streamed to the player.

By the way, thank you for testing this branch. Let me know how it goes and whether you run into any issues with the new implementation.

ekuleshov commented 3 years ago

Not sure how I can specify git branch in dependency for that plugin in the pubspec. I will have to wait for a released version.

ryanheise commented 3 years ago

You can try this:

just_audio: 
    git:
      url: git://github.com/ryanheise/just_audio.git
      ref: proxy_improvements
      path: just_audio
ekuleshov commented 3 years ago

I get the log below on the Android 11 app startup and no sound is played after.

I think I may know how I can fix that on Android, but I'd prefer not to make these changes and would rather have a single entry in the asset cache. Though if you're going to keep that, you may be able handle the required Android networking policy change trough manifest merge facility.

E/ExoPlayerImplInternal( 8180): Playback error
E/ExoPlayerImplInternal( 8180):   com.google.android.exoplayer2.ExoPlaybackException: Source error
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:554)
E/ExoPlayerImplInternal( 8180):       at android.os.Handler.dispatchMessage(Handler.java:102)
E/ExoPlayerImplInternal( 8180):       at android.os.Looper.loop(Looper.java:223)
E/ExoPlayerImplInternal( 8180):       at android.os.HandlerThread.run(HandlerThread.java:67)
E/ExoPlayerImplInternal( 8180):   Caused by: com.google.android.exoplayer2.upstream.HttpDataSource$CleartextNotPermittedException: Cleartext HTTP traffic not permitted. See https://exoplayer.dev/issues/cleartext-not-permitted
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.open(DefaultHttpDataSource.java:312)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.DefaultDataSource.open(DefaultDataSource.java:199)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.StatsDataSource.open(StatsDataSource.java:84)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.source.ProgressiveMediaPeriod$ExtractingLoadable.load(ProgressiveMediaPeriod.java:1017)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:415)
E/ExoPlayerImplInternal( 8180):       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
E/ExoPlayerImplInternal( 8180):       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
E/ExoPlayerImplInternal( 8180):       at java.lang.Thread.run(Thread.java:923)
E/ExoPlayerImplInternal( 8180):   Caused by: java.io.IOException: Cleartext HTTP traffic to 127.0.0.1 not permitted
E/ExoPlayerImplInternal( 8180):       at com.android.okhttp.HttpHandler$CleartextURLFilter.checkURLPermitted(HttpHandler.java:127)
E/ExoPlayerImplInternal( 8180):       at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:462)
E/ExoPlayerImplInternal( 8180):       at com.android.okhttp.internal.huc.HttpURLConnectionImpl.connect(HttpURLConnectionImpl.java:131)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.makeConnection(DefaultHttpDataSource.java:594)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.makeConnection(DefaultHttpDataSource.java:498)
E/ExoPlayerImplInternal( 8180):       at com.google.android.exoplayer2.upstream.DefaultHttpDataSource.open(DefaultHttpDataSource.java:307)
E/ExoPlayerImplInternal( 8180):       ... 7 more
E/AudioPlayer( 8180): TYPE_SOURCE: Cleartext HTTP traffic not permitted. See https://exoplayer.dev/issues/cleartext-not-permitted
ryanheise commented 3 years ago

You can fix that error by manually following these instructions:

https://pub.dev/packages/just_audio#platform-specific-configuration

I would not want to turn this on by default, it should be the app developer's choice for their security consideration. This issue happens because assets are now streamed via the aid of a proxy, and this proxy is running the HTTP protocol.

In the future, I hope to implement an HTTPS proxy. See #254 .

Maybe it is more urgent now to implement #254 since otherwise it would be a surprise to existing users of setAsset that their old code no longer works without additional configuration changes.

ekuleshov commented 3 years ago

@ryanheise like I said, I know how to get around that, but I considering enabling plain text traffic for everything in the app and a local http proxy a very very BAD choice, especially only to play a beep sound. In many cases allowing plain text http is just not acceptable.

Basically you are not giving developer any choice. The local http proxy has to be enabled or else it doesn't work. And really local https proxy is not that much better for this, e.g. how do you prevent other apps on a given device from accessing it?

All in all, copying assets to the local resources once would be a much more preferable solution.

ryanheise commented 3 years ago

@ryanheise like I said, I know how to get around that, but I considering enabling plain text traffic for everything in the app and a local http proxy a very very BAD choice, especially only to play a beep sound. In many cases allowing plain text http is just not acceptable.

I was hoping you could at least use clear text for testing purposes. The plan is to secure it.

Basically you are not giving developer any choice. The local http proxy has to be enabled or else it doesn't work. And really local https proxy is not that much better for this, e.g. how do you prevent other apps on a given device from accessing it?

By using a secure token in the proxy URL that only just_audio knows.

All in all, copying assets to the local resources once would be a much more preferable solution.

You do still have a choice if you prefer to write to a file, and you could implement this more efficiently within your app. After the app install, you can extract the asset into a file like this:

      await someFile.writeAsBytes(
          (await rootBundle.load(assetPath)).buffer.asUint8List());

Then, you can play it like this without ever having to re-extract the asset:

await player.setFilePath(somefile.path);

However, for convenience, I certainly want to provide a convenient method for loading assets that is secure. Do you see any problems with the above approach of securing access?

ekuleshov commented 3 years ago

By using a secure token in the proxy URL that only just_audio knows.

However, for convenience, I certainly want to provide a convenient method for loading assets that is secure. Do you see any problems with the above approach of securing access?

Unfortunately that is security by obscurity. The token will have to be stored in the app, so it can be extracted. So, I do see that being a problem.

For the the option with a file path. Isn't it platform-dependent? Not all platforms may have files or creating them may not be ideal.

Maybe instead of all that there could be a memory-based AudioSource that one could load from rootBundle.load(assetPath)).buffer.asUint8List()?

ryanheise commented 3 years ago

By using a secure token in the proxy URL that only just_audio knows.

However, for convenience, I certainly want to provide a convenient method for loading assets that is secure. Do you see any problems with the above approach of securing access?

Unfortunately that is security by obscurity. The token will have to be stored in the app, so it can be extracted. So, I do see that being a problem,

This has larger implications for other uses of the proxy (request headers, caching, ...), so there is still a need to get it right. I had intended to initialise the token at runtime randomly (it could be a hash of various random and internal values, or leverage the security features of the native platform including Secure Storage) with an override option allowing the app to set its own token. Keep in mind that if you are concerned someone will steal your app bundle's assets, I would point out that there are easier ways to do that. Once you publish your app bundle, it's out there. If you want more security, you need to employ some form of encryption, and you will need to decide all over again how to securely handle a token.

For the the option with a file path. Isn't it platform-dependent? Not all platforms may have files or creating them may not be ideal.

That's true and is actually a limitation of the previous approach implemented by just_audio which just extracted the asset to a file.

There are solutions that can be implemented with direct memory buffers, although the simplest ones are only applicable for short audio assets (e.g. beeps). But just_audio is not designed for playing 200ms beeps, it is designed for playing more substantial audio resources (songs, podcasts, audiobooks, etc.). For playing beeps, I would recommend using different APIs specifically optimised for audio assets that can be held in memory (e.g. soundpool).

Maybe instead of all that there could be a memory-based AudioSource that one could load from rootBundle.load(assetPath)).buffer.asUint8List()?

I think that's a good idea. It would make a good feature request. The native implementation of this would be very different, so it's not something that is immediately doable, but it's something that could theoretically be added in the future.

With all of that said, I am still open to changing the way I load assets, including reverting it back to the old approach, with an improved cache system. I don't know what that improved cache system should be, but the simplest approach would be to take a hash of the asset path as the cache filename and actually do cache hit tests so that we don't extract the asset repeatedly. This assumes that the app is OK with never evicting the cache, so a more advanced approach would be to add an LRU cache on top of that. But I would think that if an app included those assets in the bundle, it intended to use them, so maybe eviction isn't necessary unless they are huge assets (in which case it would be better to stream directly from the asset if it can be done securely).

ekuleshov commented 3 years ago

Thank you for looking into this. I think using hash of the asset path url for the cache keys and never evicting it will work ok. Like you said, assets are pretty much static resources.

The only issue is in case the asset content changes in the app. Just need to remember to change the asset name in this case. That should be fine for the audio files.

PS: with proxy I'm that concerned with the assets content that can be exposed, but anything else that could leak trough the proxy unexpectedly. It has to guard against malformed requests and all other kinds of traditional http attacks.

ryanheise commented 3 years ago

That's a good point about app updates. I may be able to use package_info to detect an app upgrade and re-extract the asset.

Concerning the proxy, the proxy generates unique URLs for each audio source that you register by calling setAudioSource or its variants and stores these in a "finite" map. The proxy server only responds to requests if the URL key exists in that map, and when it does exist in that map, it is hard coded to map to the specific audio source that you registered. In the case of assets, the proxy can only load an asset whose path has been registered via setAudioSource, and the implementation of the asset request handler will ignore all headers except for the byte range. Files do not go through the proxy at all.

Anyway, I will look at reverting to a file-based solution for assets, but for large assets it does seem wasteful to copy them to a file first.

ryanheise commented 3 years ago

The latest commit on the proxy_improvements branch reverts to a file-based asset cache avoiding duplicates. It will also delete the old cache directory along with any duplicates. Let me know how it goes.

Note that the old version (the currently released version) was highly sensitive on the app calling dispose to clean up the cache afterwards which suggests that probably your app wasn't successfully calling dispose on the player. This was a difficult part of the plugin design because in Flutter there is no way to execute this cleanup code if the user kills the app. This was one of the benefits of the proxy-based solution which never copied the asset to a file in the first place and therefore had no cleanup to do, but I guess I'll put that idea off until your security concerns can be addressed.

ekuleshov commented 3 years ago

@ryanheise tried out the branch. All seem look good. Thank you. Any idea when it goes to the release version?

ryanheise commented 3 years ago

Hi @ekuleshov , thanks for testing. I will aim for a weekend release, although in the meantime I would suggest pinning your pubspec to that commit.

ekuleshov commented 3 years ago

@ryanheise you mean?

ref: a74834af378243310c982ed7a11cc10440b3462e

ryanheise commented 3 years ago

Right (where ref is a sub-property under git).

ryanheise commented 3 years ago

I was not able to find a reliable way to detect when an app's assets have been updated in order to update the asset cache. It would be possible for an app to do its own versioning in the same way that sqlite manages versions and detects the right time to do migrations. So the app could include an asset containing an asset bundle version string and then if the cache and asset bundle contain different version strings, it means the asset cache needs to be updated.

But rather than implement that logic within just_audio, it will probably be easier if I just provide a method to clear the cache. That way, an app could implement that logic themselves (if it wants such logic) and call the clearAssetCache method when it detects the assets have changed.

ekuleshov commented 3 years ago

I agree think it is okay to leave it to the app to handle the cache updates. E.g. the app can just change the asset name when content changed. Though method to clear the cache will help.

ryanheise commented 3 years ago

I've now implemented the clearAssetCache() method. All that's left is to do testing which I'll try to do sometime today.

ryanheise commented 3 years ago

This is now released in 0.6.6.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs, or use StackOverflow if you need help with just_audio.