Page MenuHomePhabricator

[native] Replace react-native-ffmpeg with ffmpeg-kit-react-native
ClosedPublic

Authored by ashoat on Aug 7 2024, 3:31 PM.
Tags
None
Referenced Files
F2772771: D13021.diff
Fri, Sep 20, 1:45 AM
Unknown Object (File)
Sat, Sep 7, 1:47 PM
Unknown Object (File)
Sat, Sep 7, 11:37 AM
Unknown Object (File)
Sat, Sep 7, 11:36 AM
Unknown Object (File)
Fri, Sep 6, 7:02 AM
Unknown Object (File)
Wed, Sep 4, 5:25 AM
Unknown Object (File)
Wed, Sep 4, 3:49 AM
Unknown Object (File)
Fri, Aug 30, 8:34 AM
Subscribers

Details

Summary

This diff replaces our use of react-native-ffmpeg, which is now deprecated in favor of ffmpeg-kit-react-native.

It should also address the repeated failures we've seen due to the com.arthenica:mobile-ffmpeg-min-gpl:4.3.1.LTS artifact randomly going missing on occasion: ENG-4177, ENG-8959, and ENG-8989.

Test Plan
  • Confirmed I could build and bundle the app on iOS and Android
  • On release builds of both iOS and Android, tested the four usages of ffmpeg in our codebase, all called from native/media/ffmpeg.js:
    • getVideoInfo
    • transcodeVideo
    • generateThumbnail
    • hasMultipleFrames, which actually isn't in active use in our codebase. To test it I had to use this patch to make it so we transcode GIFs if they have multiple frames. It correctly ascertained whether the GIFs had multiple frames on both iOS and Android, but transcoding failed afterwards, so I left this functionality disabled.

Working on testing the actual functionality, but I want to see if CI passes first

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/ffmpeg
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/media/ffmpeg.js
88 ↗(On Diff #43242)

The migration guide lists this method, with an empty cell as the replacement. I'm not sure what this means, but there is no mention of resetStatistics in the new library anywhere other than the migration guide.

It doesn't seem like resetStatistics was well-documented in react-native-ffmpeg, either. The only record of it I could find was in the code. This code comment mentions:

Resets last received statistics. It is recommended to call it before starting a new execution.

My best guess is that this call is simply no longer necessary in ffmpeg-kit-react-native. I have a vague recollection that one of the improvements in ffmpeg-kit-react-native is that multiple commands can be run in parallel, which potentially seems related to why we wouldn't need to call such an API anymore.

ashoat requested review of this revision.Aug 7 2024, 3:48 PM
This revision is now accepted and ready to land.Aug 7 2024, 4:18 PM
lib/media/video-utils.js
101

We can now use Android hardware-accelerated MediaCodec libraries, which didn't seem to work on the older version of ffmpeg we were on (we went from ffmpeg 4 to ffmpeg 6, I think)

103–104

When I left these in, I got errors like these:

Unrecognized option 'crf'.
Unrecognized option 'preset'.

I Googled around, and it looks like these aren't supported by ffmpeg's MediaCodec integration, and in fact there aren't many easy options for varying the quality/speed of the encoding. Here's the most helpful post I found

125

The new version of ffmpeg seems to want this explicitly specified. I'm not sure, but I think it affects the encoders/decoders available

129

The new version of ffmpeg doesn't like this config:

One of -r/-fpsmax was specified together a non-CFR -vsync/-fps_mode. This is contradictory.

Commit that changes this is here. I found that from here, which also links to the docs.

Looks like -vsync is deprecated in favor of -fps_mode, and it looks like we should be using CFR (not VFR) if we're explicitly stating a framerate with -r 30.

130

Through experimentation I found that removing -pix_fmt yuv420p on Android doubled the speed of encoding on my test Android device (Google Pixel 6a). I remember very little about what it was for, and it was committed before I really was documenting anything. I think it's probably safe to remove

native/android/build.gradle
34

We initially started using min-gpl-lts here because we couldn't use MediaCodec. Now we can use MediaCodec, so it's no longer necessary

We still need -lts since we're one minSdkVersion too early

native/ios/Podfile
13

I switched from min-lts to min because ffmpeg-kit-react-native won't let us use ffmpeg with Apple hardware acceleration for decoding in the -lts release, and because the LTS stuff is way less important for Apple devices anyways

native/media/ffmpeg.js
88

The migration guide lists this method, with an empty cell as the replacement. I'm not sure what this means, but there is no mention of resetStatistics in the new library anywhere other than the migration guide.

It doesn't seem like resetStatistics was well-documented in react-native-ffmpeg, either. The only record of it I could find was in the code. This code comment mentions:

Resets last received statistics. It is recommended to call it before starting a new execution.

My best guess is that this call is simply no longer necessary in ffmpeg-kit-react-native. I have a vague recollection that one of the improvements in ffmpeg-kit-react-native is that multiple commands can be run in parallel, which potentially seems related to why we wouldn't need to call such an API anymore.

126

Before this change, I was seeing:

Requested output format "'singlejpeg' is not a suitable output format"

Googled it and found this random forum post that suggested mjpeg: http://orbisvitae.com/ubbthreads/ubbthreads.php?ubb=showflat&Number=58268

144

The duration returned here is now in seconds instead of milliseconds

native/media/video-utils.js
84

We can now use hardware acceleration on both iOS and Android!