[native] Fix reading video files for processing
AcceptedPublic
Actions

Authored by angelika on Tue, Apr 1, 6:51 AM.

Details

Reviewers

bartek
ashoat

Summary

https://linear.app/comm/issue/ENG-10466/processing-failed-when-sending-video-on-ios

On physical devices with iOS 18 there are problems with reading uris in the form of file://.... On iPhone 15 Pro with iOS 18.3.2 transcoding the video fails with underlying exception:

Cannot Access URL Error Domain=AVFoundationErrorDomain Code=-11884 "Cannot Access URL" UserInfo={NSLocalizedFailureReason=The sandbox extension was not issued., NSLocalizedDescription=Cannot Access URL, NSUnderlyingError=0x3034e26d0 {Error Domain=NSOSStatusErrorDomain Code=-17507 "(null)"}}

My guess is that ios sandbox doesn't allow us to open file://... uris . This also explains why it was working on simulator (it has no sandbox).
Uris like asset-library://... that are returned from ImagePicker do work though.

Test Plan

Try to send a video on iOS and Android physical devices. Force video trancoding if necessary.

Diff Detail

Repository

rCOMM Comm

Branch

graszka22/ENG-10466

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Tue, Apr 1, 6:51 AM

angelika held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Apr 1, 6:51 AM

angelika edited the summary of this revision. (Show Details)Tue, Apr 1, 7:07 AM

Harbormaster completed remote builds in B33866: Diff 47615.Tue, Apr 1, 7:10 AM

angelika published this revision for review.Tue, Apr 1, 7:14 AM

Nice find! I tested it and it resolves the issue for me. However, I think we can go deeper in removing the file:// URIs, since it doesn't seem like they're needed anymore.

I wonder if we should just remove localUri from being returning from fetchAssetInfo. I think it was only necessary on iOS because ffmpeg couldn't handle assets-library://, but now that we don't need that, it's better to remove it than to keep the legacy code around. This would also allow us to remove the suffix hack you had to add (see YnBsaXN in codebase).
If we do that, we could consider renaming fetchAssetInfo to fetchOrientation and asset_info_fetch to orientation_fetch.
We should also confirm that the image upload path works with those URIs (eg. expo-image-manipulator), but I would be surprised if they didn't.

native/media/media-utils.js
224 ↗	(On Diff #47615)	I suspect we don't need `initialURI` anymore, and can replace this usage with `selection.uri`
239 ↗	(On Diff #47615)	And replace this usage with `selection.uri`
native/media/video-utils.js
67–68 ↗	(On Diff #47615)	Is this change safe on Android? I guess you tested it, but I'm surprised that both iOS and Android can take either path or URI

This revision now requires changes to proceed.Tue, Apr 1, 10:37 AM

Review changes

However, I think we can go deeper in removing the file:// URIs, since it doesn't seem like they're needed anymore.

I wonder if we should just remove localUri from being returning from fetchAssetInfo. I think it was only necessary on iOS because ffmpeg couldn't handle assets-library://, but now that we don't need that, it's better to remove it than to keep the legacy code around. This would also allow us to remove the suffix hack you had to add (see YnBsaXN in codebase).
If we do that, we could consider renaming fetchAssetInfo to fetchOrientation and asset_info_fetch to orientation_fetch.
We should also confirm that the image upload path works with those URIs (eg. expo-image-manipulator), but I would be surprised if they didn't.

We can't do that easily. Tried to remove local uris and they're still used in fetchFileInfo. In particular it was crashing for example when fetching file size in fetchFileSize.

native/media/video-utils.js
67–68 ↗	(On Diff #47615)	I tested it. URIs returned from ImagePicker on Android are `file://` uris from the cache directory (at least according to my testing)