expo / expo

An open-source framework for making universal native apps with React. Expo runs on Android, iOS, and the web.
https://docs.expo.dev
MIT License
35.06k stars 5.61k forks source link

Upgrading to expo@44.0.0 breaks expo-av compatibility with a certain type of metadata from mp3 #16323

Closed dantxal closed 2 years ago

dantxal commented 2 years ago

Summary

In our project we use multiple mp3 files to reproduce audio. When upgrading to SDK@44.0.0 we found that we could no longer pause an audio and later resume from the duration we paused on.

Turns out that is due to the metadata not being loaded correctly, to be more specific it doesn't load immediately on loading the file, instead it can take up more than 1min to load on larger files, while the file is already being played.

I made a snack that shows it worked until SDK@43 and a reproducible demo that shows it is not working on SDK@44.

Take a look at logs when playing the each of the files, the ones downloaded from cloudinary show positionMillis/durationMillis: 1/1 while the ones re-encoded through audacity show the correct duration and position.

Useful information

Talking to the person who produces the audio for the project, he told me he uses FL Studio 12, and exports those files through Edison plugin.

Expected behaviour

I expect that both mp3 files can have their metadata loaded right away.

Managed or bare workflow? If you have ios/ or android/ directories in your project, the answer is bare!

bare

What platform(s) does this occur on?

Android

SDK Version (managed workflow only)

44.0.0

Environment

Expo CLI 5.0.3 environment info: System: OS: macOS 12.2 Shell: 5.8 - /bin/zsh Binaries: Node: 16.13.2 - ~/.asdf/installs/nodejs/lts/bin/node Yarn: 1.22.17 - /opt/homebrew/bin/yarn npm: 8.1.2 - ~/.asdf/plugins/nodejs/shims/npm Watchman: 2022.01.31.00 - /opt/homebrew/bin/watchman Managers: CocoaPods: 1.11.2 - /Users/dantxal/.asdf/shims/pod SDKs: iOS SDK: Platforms: DriverKit 21.2, iOS 15.2, macOS 12.1, tvOS 15.2, watchOS 8.3 Android SDK: Android NDK: 22.1.7171670 IDEs: Android Studio: 2021.1 AI-211.7628.21.2111.8139111 Xcode: 13.2.1/13C100 - /usr/bin/xcodebuild npmPackages: expo: >=44.0.0-0 <45.0.0 => 44.0.6 react: 17.0.1 => 17.0.1 react-native: 0.64.3 => 0.64.3 npmGlobalPackages: expo-cli: 5.0.3 Expo Workflow: bare

Reproducible demo

Reproducible demo: https://github.com/dantxal/ExpoAvIssue

This demo was created by:

  1. Running npx react-native init ExpoAvIssue --version "0.64.3";
  2. Running npx install-expo-modules;
  3. Running expo install expo-av@10.2.0;
  4. Adding the custom code to demonstrate the issue.

To run it, simply install dependencies (by running yarn) and run it on android (yarn android)

Snack of it working on SDK@43

https://snack.expo.dev/@dantxal/both-versions-of-mp3-working-on-sdk43

mnightingale commented 2 years ago

It appears to work correctly on Expo Go with SDK 43. But not on bare projects, yarn eject on the snack breaks.

Observations

As far as I can tell there are a couple of things going on.

During development when you pass in require("./assets/file_downloaded_from_cloudinary.mp3") Asset.fromModule resolves the URI http://localhost:8081/assets/assets/file_downloaded_from_cloudinary.mp3? platform=android&hash=434c8a5a91977ef87f9f6fc433aec332.

The relevant code is: https://github.com/expo/expo/blob/b1b3cd0db52dec47f80020ce476665f2060d3def/packages/expo-av/src/AV.ts#L135-L143

downloadFirst will be true (it's the default), so the asset will be downloaded and will then have a localUri like file:///data/user/0/... However that downloaded file doesn't get used, getNativeSourceFromSource returns the Metro/webserver URI which gets passed to ExoPlayer so it has to download/stream it.

Expo Go correctly returns the file:///data/... path

I think the reason is works with the reencoded_by_audacity.mp3 and not file_downloaded_from_cloudinary.mp3 is because cloudinary is a constant bitrate and audacity is variable bitrate, I expect the variable bitrate file has metadata that can be used to determine duration.

I've not verified but I expect the constant bitrate file doesn't have such metadata and it'd need to determine duration from the filesize/bitrate/samplerate, but the metro/webserver doesn't return a "Content-Length" header so it can't be determined until the end of the file has been buffered (i.e. it knows the filesize).

Verify

To verify this cause if you host the mp3 on a remote webserver and load it then it works correctly, which is why I think the Content-Length header is used.

downloadFirst will be ignored and the original remote URI passed to ExoPlayer.

await sound.loadAsync({
  uri: "https://YOURHOST/file_downloaded_from_cloudinary.mp3",
});

Fixing

I'm not sure what the issue is but I believe it must be with expo-asset and getNativeSourceFromSource which calls Asset.fromModule. On bare workflows it should find that it already has a cached version available and return a localUri and downloaded:true.

A secondary fix may be required for when downloadFirst has been set to false, the asset webserver should return the Content-Length header, I don't know how it works, maybe this only affects development? Also it'd be rare that a user set downloadFirst to false.

TL;DR

mnightingale commented 2 years ago

Example to demonstrate what I think the issue is:

let asset = Asset.fromModule(
  require("./assets/file_downloaded_from_cloudinary.mp3")
);

console.log(1, asset.downloaded, asset.localUri);

await asset.downloadAsync();

console.log(2, asset.downloaded, asset.localUri);

// Load it again like expo-av would

asset = Asset.fromModule(
  require("./assets/file_downloaded_from_cloudinary.mp3")
);

console.log(3, asset.downloaded, asset.localUri);

Output bare:

1 false null
2 true file:///data/user/0/com.expo.mp3sdk43/cache/ExponentAsset-434c8a5a91977ef87f9f6fc433aec332.mp3
3 false null

Output Go:

1 false null
2 true file:///data/user/0/host.exp.exponent/cache/ExperienceData/%2540anonymous%252Fsnack-08047475-b63b-41f3-8441-dcca44ade460-d93d1315-a5a2-42af-ae48-a731ab939056/ExponentAsset-434c8a5a91977ef87f9f6fc433aec332.mp3
3 true file:///data/user/0/host.exp.exponent/cache/ExperienceData/%2540anonymous%252Fsnack-08047475-b63b-41f3-8441-dcca44ade460-d93d1315-a5a2-42af-ae48-a731ab939056/ExponentAsset-434c8a5a91977ef87f9f6fc433aec332.mp3
AdamGerthel commented 2 years ago

It might be worth adding that this issue does not exist when using expo-av ^8.4.0 with react-native-unimodules ^0.10.1, so something's changed between those versions and the current (where unimodules has been deprecated and replaced with expo + expo-asset)

mnightingale commented 2 years ago

@AdamGerthel could you provide a bare repo that doesn't have this problem?

managed/Expo Go doesn't because it appears to get sent the whole bundle of files/assets (so Asset.loadModule resolves a local file)

I tried expo 41 and 8.4.0 and still had the issue.

I think I have some idea of what's wrong, but trying to determine the correct way to fix it without causing more issues and I'd much rather being able to pinpoint what change caused the problem.

If status.uri is /assets/... then its wrong, /data/user/0 is correctly using the local cached file.

dantxal commented 2 years ago

Hello, @mnightingale

I'm here to provide you with the demo.

It's worth adding that in the process to create the demo I've encountered the same problem after adding expo-av@8.4.0. The only way to solve it was to install expo-asset@~8.0.0 as you can see from the commit history.

Which might just prove your point:

I'm not sure what the issue is but I believe it must be with expo-asset

Reproducible demo: https://github.com/dantxal/bareExpoAvIssue

Important note: I couldn't build versions 8.1.0~8.1.2 and 8.1.3 is already broken. Also 8.1.5 is the first version in the expo/expo CHANGELOG.

mnightingale commented 2 years ago

I think this might only be an issue for dev/debug builds, release get the assets bundled in the app therefore the assets get resolved to local URIs.

But I do think expo-av has a bug that it doesn't use the downloadAsync() result.

{JSON.stringify(
  Asset.fromModule(
    require("./assets/file_downloaded_from_cloudinary.mp3")
  )
)}

From a release APK

image

Debug/Dev

image

This same code fails in a dev build, localUri is obviously not available it has to be loaded from the metro bundler.

However the expo-av downloadAsync() result doesn't get used so the metro bundler URI gets passed to the player, metro doesn't send a Content-Length header so position/duration are unavailable until the whole file is buffered (at about 70%) as I initinially observed..

Fix

You can try what I think needs to change to fix dev mode with the small change on https://github.com/mnightingale/expo/tree/%40mnightingale/av-fix-localassets (I'm not sure if you can add a dependecy on it, else just install expo-av next or 11.0.0 and copy the build folder to your node_modules).

I also tried getNativeSourceFromSource(asset ?? source) which looks neater but the other properties would be missing (headers and overridingExtension) although headers wouldn't be needed.

dantxal commented 2 years ago

Awesome work, @mnightingale!!

I can confirm that your fix works.

Do you want to submit a PR for 11.0.0 next?

Since it's your own fork, I dont think I can add a dependency to that either.

EDIT: I couldn't add your fork as a dependency, I get this error:

* What went wrong:
A problem occurred evaluating project ':expo-av'.
> Project :ReactAndroid not found.

Is your fork only for managed RN?

mnightingale commented 2 years ago

I've submitted a PR, it also has a little more info about why it appears the duration doesn't work while streaming with your file and how encoding with Xing headers could resolve at least that part of the problem.

Hopefully the change will get reviewed an merged fairly soon, but it can take weeks.

If you use Yarn > 2.1 (I still use 1), you can do the following:

"dependencies": {
    // ...
    "expo-av": "mnightingale/expo#head=@mnightingale/av-fix-localassets&workspace=expo-av",
   // ..

I couldn't get it to accept this via command line.

Also note my branch will get deleted once it's merged or rejected.

dantxal commented 2 years ago

First of all, thank you for the work you put into expo, @mnightingale and @bbarthec!!

About the merge, in which version will this PR's code be available? I tried 'next' tag and the code isn't there. Also, do you have any tips to get it to build on iOS?

bbarthec commented 2 years ago

@dantxal, ~if you're using SDK 44 in managed workflow then this is not available (you'll have to wait until new version of Expo Go client is available to have it there). It is available for bare workflow though.~ edit: I've misconnected the PR and the issue and I've written nonsense ๐Ÿ˜… Proper info: It will be available once I publish a patch to the npm.js for the expo-av package.

expo-bot commented 2 years ago

Some changes in the following packages that may fix this issue have just been published to npm under next tag ๐Ÿš€

๐Ÿ“ฆ Package ๐Ÿ”ข Version โ†–๏ธ Pull requests ๐Ÿ“ Release notes
expo-av 11.0.1 #16544 CHANGELOG.md

If you're using bare workflow you can upgrade them right away. We kindly ask you for some feedbackโ€”even if it works ๐Ÿ™

They will become available in managed workflow with the next SDK release ๐Ÿ‘€

Happy Coding! ๐ŸŽ‰

dantxal commented 2 years ago

So far I was only able to build it for android, and again by following the same steps I meantioned here: https://github.com/expo/expo/pull/16123#issuecomment-1058393223.

It works, and solves the problem I was having.

Should that change to kotlin version be implemented in the expo project?

mnightingale commented 2 years ago

@dantxal for a bare workflow you can add a kotlinVersion override in your own android/build.gradle

buildscript {
    ext {
        kotlinVersion = "1.6.10"

It won't be needed once the modules are all published again (I assume with the next SDK release).

expo-bot commented 2 years ago

Some changes in the following packages that may fix this issue have just been published to npm under next tag ๐Ÿš€

๐Ÿ“ฆ Package ๐Ÿ”ข Version โ†–๏ธ Pull requests ๐Ÿ“ Release notes
expo-av 11.1.0 #16544 CHANGELOG.md

If you're using bare workflow you can upgrade them right away. We kindly ask you for some feedbackโ€”even if it works ๐Ÿ™

They will become available in managed workflow with the next SDK release ๐Ÿ‘€

Happy Coding! ๐ŸŽ‰