Shouldn't getVideoProcessingPlan determine whether we need to rotate the video, and then that information gets passed to the native layer? If you've made changes that result in the code ignoring the result of getVideoProcessingPlan, then I think we should deprecate / adjust that function, rather than leave it around and have it be ignored.
It's interesting that the height/width values were rotated, as the dimensions of the video still appear correct in my bug report... just the video appears smaller. Can you explain why the dimensions of the video in the bug report still look correct?

Looking at getVideoProcessingPlan more closely, it doesn't seem to mention rotating of the video... not sure how this was handled in the old system, but I guess it was internal to FFmpeg. Sorry for the misdirection!

bartek accepted this revision.Thu, Apr 3, 12:46 AM

This revision is now accepted and ready to land.Thu, Apr 3, 12:46 AM

It's interesting that the height/width values were rotated, as the dimensions of the video still appear correct in my bug report... just the video appears smaller. Can you explain why the dimensions of the video in the bug report still look correct?

There are two places we get the dimensions of the video from. One is getVideoInfo(), the second one is the result of ImagePicker: https://github.com/CommE2E/comm/blob/master/native/media/media-utils.js#L93 I guess in the report there were values from ImagePicker.

Closed by commit rCOMMdae1f8b2153b: [native] Handle exif data orientation when returning video size in getVideoInfo… (authored by angelika). · Explain WhyThu, Apr 3, 3:51 PM

This revision was automatically updated to reflect the committed changes.

angelika added a commit: rCOMMdae1f8b2153b: [native] Handle exif data orientation when returning video size in getVideoInfo….

I was asking about why the aspect ratio in the video I put in the bug report is still correct.

I was asking about why the aspect ratio in the video I put in the bug report is still correct.

As I answered before the values in the report come from expo's image picker. Details:

correctly rotated video has width 1080, height 1920
selection.dimensions comes from ImagePicker, which takes into account exif data
the first step in the bug report is the following:

{
        "dimensions": {
          "height": 1920,
          "width": 1080
        },
        "duration": 2.2316666666666665,
        "filename": "IMG_9232.MOV",
        "mediaNativeID": "8483761D-54E0-4A64-9D3B-AD0BDD99DEBE/L0/001",
        "retries": 0,
        "selectTime": 1743430376010,
        "sendTime": 1743430376010,
        "step": "video_library",
        "uri": "assets-library://asset/asset.mov?id=8483761D-54E0-4A64-9D3B-AD0BDD99DEBE&ext=mov"
      },

and is created here: https://github.com/CommE2E/comm/blob/master/native/media/media-gallery-keyboard.react.js#L386 so the dimensions are from image picker so they're correct

now as for displayed video, the dimensions come from getVideoInfo (I won't dig exactly how) and it reports width 1920, height 1080 (incorrectly)
the phone you're holding has portrait orientation
so, what I think happens now is that we create video player, scale it down with styles so it displays "horizontal" video on a screen with portrait orientation that fits the screen with correct aspect ratio but the video player figures out that the video is rotated so it's still displayed in correct orientation, just scaled down with our styles

Thanks for explaining! I can confirm the fix works for me :)

Revision Contents
Changeset List

Path

Size

native/

expo-modules/

comm-expo-package/

ios/

MediaModule.swift

6 lines

Diff 47648

View Options

native/expo-modules/comm-expo-package/ios/MediaModule.swift

Show First 20 Lines • Show All 1,077 Lines • ▼ Show 20 Lines	memberInfo:
\| ServerMemberInfo,		\| ServerMemberInfo,
): boolean {		): boolean {
if (memberInfo.minimallyEncoded) {		if (memberInfo.minimallyEncoded) {
return hasPermission(memberInfo.permissions, threadPermissions.CHANGE_ROLE);		return hasPermission(memberInfo.permissions, threadPermissions.CHANGE_ROLE);
}		}
return !!memberInfo.permissions[threadPermissions.CHANGE_ROLE]?.value;		return !!memberInfo.permissions[threadPermissions.CHANGE_ROLE]?.value;
}		}

		function roleIsDefaultRole(
		roleInfo: ?ClientLegacyRoleInfo \| ?RoleInfo,
		): boolean {
		return !!(roleInfo && roleInfo.isDefault);
		}

function roleIsAdminRole(roleInfo: ?ClientLegacyRoleInfo \| ?RoleInfo): boolean {		function roleIsAdminRole(roleInfo: ?ClientLegacyRoleInfo \| ?RoleInfo): boolean {
return !!(roleInfo && !roleInfo.isDefault && roleInfo.name === 'Admins');		return !!(roleInfo && !roleInfo.isDefault && roleInfo.name === 'Admins');
}		}

function threadHasAdminRole(		function threadHasAdminRole(
threadInfo: ?(		threadInfo: ?(
\| LegacyRawThreadInfo		\| LegacyRawThreadInfo
\| RawThreadInfo		\| RawThreadInfo
▲ Show 20 Lines • Show All 746 Lines • ▼ Show 20 Lines	export {
getMinimallyEncodedCurrentUser,		getMinimallyEncodedCurrentUser,
threadFrozenDueToBlock,		threadFrozenDueToBlock,
threadFrozenDueToViewerBlock,		threadFrozenDueToViewerBlock,
rawThreadInfoFromServerThreadInfo,		rawThreadInfoFromServerThreadInfo,
threadUIName,		threadUIName,
threadInfoFromRawThreadInfo,		threadInfoFromRawThreadInfo,
threadTypeDescriptions,		threadTypeDescriptions,
memberHasAdminPowers,		memberHasAdminPowers,
		roleIsDefaultRole,
roleIsAdminRole,		roleIsAdminRole,
threadHasAdminRole,		threadHasAdminRole,
identifyInvalidatedThreads,		identifyInvalidatedThreads,
permissionsDisabledByBlock,		permissionsDisabledByBlock,
emptyItemText,		emptyItemText,
threadNoun,		threadNoun,
threadLabel,		threadLabel,
useWatchThread,		useWatchThread,
Show All 24 Lines

[native] Handle exif data orientation when returning video size in getVideoInfo on iOSClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 47648

native/expo-modules/comm-expo-package/ios/MediaModule.swift

[native] Handle exif data orientation when returning video size in getVideoInfo on iOS
ClosedPublic
Actions

Revision Contents
Changeset List