Paths

Table of Contentst

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

Differential D14478

[android] Implement transcodeVideo() function on Android
ClosedPublic
Actions

Authored by angelika on Thu, Mar 20, 4:45 AM.

Details

Reviewers

bartek

Commits

rCOMM99354f3d9c3a: [android] Implement transcodeVideo() function on Android

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 ↗	(On Diff #47489)	We're setting the profile here, but it can be ignored by the encoder. Encoder probably knows better anyway.
243 ↗	(On Diff #47489)	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 ↗	(On Diff #47489)	Which API is unstable here? It's worth adding an in-code comment
196 ↗	(On Diff #47489)	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 ↗	(On Diff #47489)	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

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

Rebase & review changes

angelika marked 2 inline comments as done.Tue, Mar 25, 10:13 PM

angelika marked an inline comment as done.

Harbormaster completed remote builds in B33836: Diff 47554.Tue, Mar 25, 10:39 PM

bartek accepted this revision.Wed, Mar 26, 3:08 AM

This revision is now accepted and ready to land.Wed, Mar 26, 3:08 AM

Closed by commit rCOMM99354f3d9c3a: [android] Implement transcodeVideo() function on Android (authored by angelika). · Explain WhyThu, Mar 27, 12:21 PM

This revision was automatically updated to reflect the committed changes.

angelika added a commit: rCOMM99354f3d9c3a: [android] Implement transcodeVideo() function on Android.

Revision Contents
Changeset List

Path

Size

native/

expo-modules/

comm-expo-package/

android/

build.gradle

4 lines

src/

main/

java/

app/

comm/

android/

media/

MediaModule.kt

175 lines

Diff 47554

View Options

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

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
import {		import {
RelationshipListItemTooltipModalRouteName,		RelationshipListItemTooltipModalRouteName,
FriendListRouteName,		FriendListRouteName,
BlockListRouteName,		BlockListRouteName,
} from '../navigation/route-names.js';		} from '../navigation/route-names.js';
import { useSelector } from '../redux/redux-utils.js';		import { useSelector } from '../redux/redux-utils.js';
import { type Colors, useColors, useStyles } from '../themes/colors.js';		import { type Colors, useColors, useStyles } from '../themes/colors.js';
import type { VerticalBounds } from '../types/layout-types.js';		import type { VerticalBounds } from '../types/layout-types.js';
		import { useNavigateToUserProfileBottomSheet } from '../user-profile/user-profile-utils.js';
import Alert from '../utils/alert.js';		import Alert from '../utils/alert.js';

type BaseProps = {		type BaseProps = {
+userInfo: AccountUserInfo,		+userInfo: AccountUserInfo,
+lastListItem: boolean,		+lastListItem: boolean,
+verticalBounds: ?VerticalBounds,		+verticalBounds: ?VerticalBounds,
+relationshipListRoute: NavigationRoute<'FriendList' \| 'BlockList'>,		+relationshipListRoute: NavigationRoute<'FriendList' \| 'BlockList'>,
+navigate: RelationshipListNavigate,		+navigate: RelationshipListNavigate,
Show All 10 Lines	type Props = {
// async functions that hit server APIs		// async functions that hit server APIs
+updateRelationships: (		+updateRelationships: (
request: RelationshipRequest,		request: RelationshipRequest,
) => Promise<RelationshipErrors>,		) => Promise<RelationshipErrors>,
// withOverlayContext		// withOverlayContext
+overlayContext: ?OverlayContextType,		+overlayContext: ?OverlayContextType,
// withKeyboardState		// withKeyboardState
+keyboardState: ?KeyboardState,		+keyboardState: ?KeyboardState,
		+navigateToUserProfileBottomSheet: (userID: string) => mixed,
};		};
class RelationshipListItem extends React.PureComponent<Props> {		class RelationshipListItem extends React.PureComponent<Props> {
editButton = React.createRef<React.ElementRef<typeof View>>();		editButton = React.createRef<React.ElementRef<typeof View>>();

render() {		render() {
const {		const {
lastListItem,		lastListItem,
removeUserLoadingStatus,		removeUserLoadingStatus,
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Lines	) {
>		>
<Text style={this.props.styles.blueAction}>Add</Text>		<Text style={this.props.styles.blueAction}>Add</Text>
</TouchableOpacity>		</TouchableOpacity>
);		);
}		}

return (		return (
<View style={[this.props.styles.container, borderBottom]}>		<View style={[this.props.styles.container, borderBottom]}>
		<TouchableOpacity
		ginsuAuthorUnsubmitted Done Inline Actions The red background shows the `TouchableOpacity`: I think it would be a be a better user experience if we had the entire item be a touchable instead of just the avatar/username ginsu: The red background shows the `TouchableOpacity`: {F773403} I think it would be a be a better…
		atulUnsubmitted Not Done Inline Actions What surfaces should be "touchable" is probably something we should consider as part of the Design System (at some point) CC @ashoat @ted In this particular case, it feels a little weird that we're extending so far to the right, but on the left we go just until the left edge. I guess this is a new scenario, because previously we were just listing "static" avatars/usernames and left all the interaction to the right-justified icon. Unsolicited, but if it were up to me I think it'd make sense to give the avatar/username some sort of inset to make it clear that A. the element is interactive B. the bounds of the tap target Something like: or atul: What surfaces should be "touchable" is probably something we should consider as part of the…
		ashoatUnsubmitted Not Done Inline Actions I like @atul's idea. Curious for @ted's thoughts, both on Atul's idea and also on how we can include touchable surface / interaction design of touchables in the design system ashoat: I like @atul's idea. Curious for @ted's thoughts, both on Atul's idea and also on how we can…
		tedUnsubmitted Not Done Inline Actions In this particular case, it feels a little weird that we're extending so far to the right, but on the left we go just until the left edge. I think the touch point can extend more on the left, to where `Search` starts at the input field above. @atul @ashoat I think adding touch points to the design system makes perfect sense and can be included for both web and mobile components. Each design system component lives on its own page in Figma, which will also include small documents to specifically point out the interactive touch points. These documents can point out other component rules such as places to use this component, certain rules of how to use this component, etc. For the concepts you shared, I don't think that adding an inset is necessary to point out that the element is interactive. Adding an inset feels a bit cluttered to the overall list layout. If we do want to use an inset container for the avatar and username, then reducing the separator lines (smaller, more subtle) will help with over-cluttering. I took a quick look at Discord's username touchpoints in their list. In the video, I am tapping as far right as I can before the two action buttons. The entire width seems tappable except for the two buttons. Not saying we need to do exactly how Discord handles this interaction, I wanted to see how other apps were handling this. With that, I think the UI is fine how it is, but always open if more people think we should add an inset or interactive indicator. Thank you! ted: > In this particular case, it feels a little weird that we're extending so far to the right…
		ginsuAuthorUnsubmitted Done Inline Actions For the concepts you shared, I don't think that adding an inset is necessary to point out that the element is interactive. Adding an inset feels a bit cluttered to the overall list layout. If we do want to use an inset container for the avatar and username, then reducing the separator lines (smaller, more subtle) will help with over-cluttering. I agree with this point, I think especially since we have the edit (pencil) icon to the right it would feel strange if that didn't have an inset/obvious indicator that the icon is touchable but the avatar/username does. To polish this up, I can make the entire container a touchable or at the very least, also extend the touchable to be touching the left side of this container ginsu: > For the concepts you shared, I don't think that adding an inset is necessary to point out…
		onPress={this.onPressUser}
		style={this.props.styles.userInfoContainer}
		>
<UserAvatar size="small" userID={this.props.userInfo.id} />		<UserAvatar size="small" userID={this.props.userInfo.id} />
<SingleLine style={this.props.styles.username}>		<SingleLine style={this.props.styles.username}>
{this.props.userInfo.username}		{this.props.userInfo.username}
</SingleLine>		</SingleLine>
		</TouchableOpacity>
{editButton}		{editButton}
</View>		</View>
);		);
}		}

		onPressUser = () => {
		this.props.navigateToUserProfileBottomSheet(this.props.userInfo.id);
		};

onSelect = () => {		onSelect = () => {
const { id, username } = this.props.userInfo;		const { id, username } = this.props.userInfo;
this.props.onSelect({ id, username });		this.props.onSelect({ id, username });
};		};

visibleEntryIDs() {		visibleEntryIDs() {
const { relationshipListRoute } = this.props;		const { relationshipListRoute } = this.props;
const id = {		const id = {
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Lines	container: {
paddingHorizontal: 24,		paddingHorizontal: 24,
paddingVertical: 10,		paddingVertical: 10,
backgroundColor: 'panelForeground',		backgroundColor: 'panelForeground',
borderColor: 'panelForegroundBorder',		borderColor: 'panelForegroundBorder',
},		},
borderBottom: {		borderBottom: {
borderBottomWidth: 1,		borderBottomWidth: 1,
},		},
		userInfoContainer: {
		flex: 1,
		flexDirection: 'row',
		},
buttonContainer: {		buttonContainer: {
flexDirection: 'row',		flexDirection: 'row',
},		},
editButtonWithMargin: {		editButtonWithMargin: {
marginLeft: 15,		marginLeft: 15,
},		},
username: {		username: {
color: 'panelForegroundSecondaryLabel',		color: 'panelForegroundSecondaryLabel',
Show All 29 Lines	) {
);		);
const colors = useColors();		const colors = useColors();
const styles = useStyles(unboundStyles);		const styles = useStyles(unboundStyles);
const dispatchActionPromise = useDispatchActionPromise();		const dispatchActionPromise = useDispatchActionPromise();
const boundUpdateRelationships = useServerCall(updateRelationships);		const boundUpdateRelationships = useServerCall(updateRelationships);
const overlayContext = React.useContext(OverlayContext);		const overlayContext = React.useContext(OverlayContext);
const keyboardState = React.useContext(KeyboardContext);		const keyboardState = React.useContext(KeyboardContext);

		const navigateToUserProfileBottomSheet =
		useNavigateToUserProfileBottomSheet();

return (		return (
<RelationshipListItem		<RelationshipListItem
{...props}		{...props}
removeUserLoadingStatus={removeUserLoadingStatus}		removeUserLoadingStatus={removeUserLoadingStatus}
colors={colors}		colors={colors}
styles={styles}		styles={styles}
dispatchActionPromise={dispatchActionPromise}		dispatchActionPromise={dispatchActionPromise}
updateRelationships={boundUpdateRelationships}		updateRelationships={boundUpdateRelationships}
overlayContext={overlayContext}		overlayContext={overlayContext}
keyboardState={keyboardState}		keyboardState={keyboardState}
		navigateToUserProfileBottomSheet={navigateToUserProfileBottomSheet}
/>		/>
);		);
});		});

export default ConnectedRelationshipListItem;		export default ConnectedRelationshipListItem;