Page MenuHomePhabricator
Differential D14478

[android] Implement transcodeVideo() function on Android
Needs RevisionPublic
Actions

Authored by angelika on Thu, Mar 20, 4:45 AM.
Tags
None
Referenced Files
F4959314: D14478.id47489.diff
Sun, Mar 23, 6:31 AM
Unknown Object (File)
Sat, Mar 22, 10:34 PM
Unknown Object (File)
Sat, Mar 22, 1:02 PM
Unknown Object (File)
Fri, Mar 21, 2:11 PM
Subscribers
ashoat
tomek

Details

Reviewers
bartek
Summary

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

Changes:

  • on the native side we check progress every 200ms and send an event to the js side. The JS side then calls the callback with the progress
  • we can specify the profile and bitrate, we can't specify fps though
  • width and height is mandatory to keep it simple (ios requires it)
  • I'm returning some basic stats (speed, size and duration) but some are missing compared to ffmpeg implementation because I don't see much sense in collecting them. However I can add them if necessary (should be easy on Android, more complicated on iOS)

On Android there is no explicit way to set -movflags +faststart from ffmpeg. According to this question and github issue it's not supported.

Depends on D14477

Test Plan

Send some videos on some devices and verify they're transcoded correctly

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Thu, Mar 20, 4:45 AM
angelika held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptThu, Mar 20, 4:45 AM
angelika added a child revision: D14479: [ios] Implement transcodeVideo() function on iOS.Thu, Mar 20, 4:46 AM
Harbormaster failed remote builds in B33785: Diff 47489!Thu, Mar 20, 5:18 AM
angelika edited the summary of this revision. (Show Details)Thu, Mar 20, 5:21 AM
angelika added inline comments.Thu, Mar 20, 5:25 AM
native/expo-modules/comm-expo-package/android/src/main/java/app/comm/android/media/MediaModule.kt
225

We're setting the profile here, but it can be ignored by the encoder. Encoder probably knows better anyway.

243

Run the timer to check progress every 200ms on the app thread. Start the transformer also on the app thread, because the transformer *must* always be accessed on the same thread. However the actual processing is done in some other internal thread, so we don't hang the app when processing.

angelika edited the summary of this revision. (Show Details)Thu, Mar 20, 5:34 AM
angelika published this revision for review.Thu, Mar 20, 5:57 AM
bartek added inline comments.Fri, Mar 21, 2:19 AM
native/expo-modules/comm-expo-package/android/src/main/java/app/comm/android/media/MediaModule.kt
188

Which API is unstable here? It's worth adding an in-code comment

196

Theoretically this will always be not-null but I'd prefer a safer and explicit approach like here https://github.com/CommE2E/comm/blob/c1e6a12cab9e7c8d8eb565a9c0327735e88afddc/native/expo-modules/comm-expo-package/android/src/main/java/app/comm/android/thumbhash/ThumbhashModule.kt#L73

285

I'd parse second argument (Uri) at the beginning of the fun transcodeVideo() and do a null check in case caller provided invalid path/uri.
Now it can lead to crashing the app

bartek requested changes to this revision.Sun, Mar 23, 11:40 PM

Requesting changes because of these null checks which might lead to crash

This revision now requires changes to proceed.Sun, Mar 23, 11:40 PM

Revision Contents
Changeset List

PathSize
native/
expo-modules/
comm-expo-package/
android/
4 lines
src/
main/
java/
app/
comm/
android/
media/
165 lines

Diff 47489

View Options

native/expo-modules/comm-expo-package/android/build.gradle

Loading...
View Options

native/expo-modules/comm-expo-package/android/src/main/java/app/comm/android/media/MediaModule.kt

Loading...