Page MenuHomePhabricator
Differential D14481

[native] Remove ffmpeg dependency
ClosedPublic
Actions

Authored by angelika on Thu, Mar 20, 4:48 AM.
Tags
None
Referenced Files
F5111648: D14481.diff
Wed, Apr 2, 4:32 AM
Unknown Object (File)
Mon, Mar 31, 12:51 PM
Unknown Object (File)
Sun, Mar 30, 11:10 AM
Unknown Object (File)
Sun, Mar 30, 10:28 AM
Unknown Object (File)
Sat, Mar 29, 5:00 PM
Unknown Object (File)
Sat, Mar 29, 12:50 PM
Unknown Object (File)
Sat, Mar 29, 12:26 PM
Unknown Object (File)
Sat, Mar 29, 9:33 AM
View All 25 Files
Subscribers
ashoat
tomek

Details

Reviewers
bartek
Commits
rCOMM2fb795a199b7: [native] Remove ffmpeg dependency
Summary

https://linear.app/comm/issue/ENG-10314/migrate-comm-app-to-use-new-media-module

  • got rid of ffmpeg dependency
  • renamed ffmpeg.js to mediaProcessingQueue.js because the main feature of this file is the queue and wrapping functions from the media module
  • reanamed FFmpegStatistics and video_ffmpeg_transcode step

Depends on D14480

Test Plan

Compile the app and verify sending videos still work

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Thu, Mar 20, 4:48 AM
angelika held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptThu, Mar 20, 4:48 AM
Harbormaster completed remote builds in B33788: Diff 47492.Thu, Mar 20, 5:22 AM
angelika edited the summary of this revision. (Show Details)Thu, Mar 20, 5:41 AM
angelika updated this revision to Diff 47503.Thu, Mar 20, 5:53 AM

Rebase

angelika published this revision for review.Thu, Mar 20, 5:57 AM
Harbormaster completed remote builds in B33799: Diff 47503.Thu, Mar 20, 6:11 AM
bartek accepted this revision.Fri, Mar 21, 8:57 AM

Wondering how much smaller our binaries will be without ffmpeg 😀

This revision is now accepted and ready to land.Fri, Mar 21, 8:57 AM
ashoat added inline comments.Fri, Mar 21, 4:13 PM
native/media/media-processing-queue.js
31 ↗(On Diff #47503)

Do we still need this synchronous queue? We previously had it because of limitations in ffmpeg-kit, but it reduces performance when a user uploads multiple media in a single message

bartek added inline comments.Sun, Mar 23, 11:55 PM
native/media/media-processing-queue.js
31 ↗(On Diff #47503)

I was thinking about this too, but I have a suspicion that mobile devices can struggle with processing multiple videos simultaneously and CPU performance will be a limiting factor

angelika updated this revision to Diff 47557.Tue, Mar 25, 10:07 PM

Rebase

angelika added inline comments.Tue, Mar 25, 10:21 PM
native/media/media-processing-queue.js
31 ↗(On Diff #47503)

Tested it and:

  1. It's very slow
  2. Reporting progress currently works for one video at a time - we can implement progress for multiple videos (for example by adding video id to the progress event) but it's some additional work
Harbormaster completed remote builds in B33839: Diff 47557.Tue, Mar 25, 10:49 PM
Closed by commit rCOMM2fb795a199b7: [native] Remove ffmpeg dependency (authored by angelika). · Explain WhyThu, Mar 27, 12:21 PM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMM2fb795a199b7: [native] Remove ffmpeg dependency.

Revision Contents
Changeset List

PathSize
lib/
types/
6 lines
native/
android/
4 lines
ios/
Comm.xcodeproj/
16 lines
1 line
12 lines
media/
4 lines
15 lines
11 lines
1 line
5 lines

Diff 47492

View Options

lib/types/media-types.js

Loading...
View Options

native/android/build.gradle

Loading...
View Options

native/ios/Comm.xcodeproj/project.pbxproj

Loading...
View Options

native/ios/Podfile

Loading...
View Options

native/ios/Podfile.lock

Loading...
View Options

native/media/ffmpeg.js

Loading...
View Options

native/media/file-utils.js

Loading...
View Options

native/media/media-processing-queue.js

Loading...
View Options

native/media/video-utils.js

Loading...
View Options

native/package.json

Loading...
View Options

yarn.lock

Loading...