Skip to content

perf: improve uploading image from gallery on native device #59330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mountiny merged 7 commits into from
Apr 3, 2025
Merged

perf: improve uploading image from gallery on native device #59330

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
203b20f
replace deprecated .manipulateAsync with .manipulate
TMisiukiewicz Mar 31, 2025
7bad6a4
add loader when picked an image
TMisiukiewicz Mar 31, 2025
3a7096f
add loader when picking an image
TMisiukiewicz Apr 1, 2025
921d1f8
fix lint-changed job
TMisiukiewicz Apr 1, 2025
a4a1ca0
Merge remote-tracking branch 'origin/main' into perf/manipulating-hei…
TMisiukiewicz Apr 2, 2025
4acd691
Revert "add loader when picking an image"
TMisiukiewicz Apr 2, 2025
215a62f
release manipulate context when error
TMisiukiewicz Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 35 additions & 25 deletions src/components/AttachmentPicker/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Str} from 'expensify-common';
import {manipulateAsync, SaveFormat} from 'expo-image-manipulator';
import {ImageManipulator, SaveFormat} from 'expo-image-manipulator';
import React, {useCallback, useMemo, useRef, useState} from 'react';
import {Alert, View} from 'react-native';
import RNFetchBlob from 'react-native-blob-util';
Expand All @@ -19,7 +19,7 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import * as FileUtils from '@libs/fileDownload/FileUtils';
import {cleanFileName, showCameraPermissionsAlert, verifyFileFormat} from '@libs/fileDownload/FileUtils';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
import type IconAsset from '@src/types/utils/IconAsset';
Expand Down Expand Up @@ -85,7 +85,7 @@ const getDocumentPickerOptions = (type: string, fileLimit: number): DocumentPick
const getDataForUpload = (fileData: FileResponse): Promise<FileObject> => {
const fileName = fileData.name || 'chat_attachment';
const fileResult: FileObject = {
name: FileUtils.cleanFileName(fileName),
name: cleanFileName(fileName),
type: fileData.type,
width: fileData.width,
height: fileData.height,
Expand Down Expand Up @@ -121,7 +121,6 @@ function AttachmentPicker({
const [isVisible, setIsVisible] = useState(false);
const StyleUtils = useStyleUtils();
const theme = useTheme();

const completeAttachmentSelection = useRef<(data: FileObject[]) => void>(() => {});
const onModalHide = useRef<() => void>();
const onCanceled = useRef<() => void>(() => {});
Expand Down Expand Up @@ -156,7 +155,7 @@ function AttachmentPicker({
if (response.errorCode) {
switch (response.errorCode) {
case 'permission':
FileUtils.showCameraPermissionsAlert();
showCameraPermissionsAlert();
return resolve();
default:
showGeneralAlert();
Expand All @@ -174,30 +173,41 @@ function AttachmentPicker({
}

if (targetAsset?.type?.startsWith('image')) {
FileUtils.verifyFileFormat({fileUri: targetAssetUri, formatSignatures: CONST.HEIC_SIGNATURES})
verifyFileFormat({fileUri: targetAssetUri, formatSignatures: CONST.HEIC_SIGNATURES})
.then((isHEIC) => {
// react-native-image-picker incorrectly changes file extension without transcoding the HEIC file, so we are doing it manually if we detect HEIC signature
if (isHEIC && targetAssetUri) {
manipulateAsync(targetAssetUri, [], {format: SaveFormat.JPEG})
.then((manipResult) => {
const uri = manipResult.uri;
const convertedAsset = {
uri,
name: uri
.substring(uri.lastIndexOf('/') + 1)
.split('?')
.at(0),
type: 'image/jpeg',
width: manipResult.width,
height: manipResult.height,
};

return resolve([convertedAsset]);
})
.catch((err) => reject(err));
} else {
return resolve(response.assets);
const manipulateContext = ImageManipulator.manipulate(targetAssetUri);

manipulateContext.renderAsync().then((image) =>
image
.saveAsync({format: SaveFormat.JPEG})
.then((result) => {
manipulateContext.release();
image.release();
return result;
})
.then((manipResult) => {
const uri = manipResult.uri;
const convertedAsset = {
uri,
name: uri
.substring(uri.lastIndexOf('/') + 1)
.split('?')
.at(0),
type: 'image/jpeg',
width: manipResult.width,
height: manipResult.height,
};
return resolve([convertedAsset]);
})
.catch(() => {
manipulateContext?.release();
resolve(response.assets ?? []);
}),

@DylanDylann DylanDylann Apr 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure free memory in exceptional case

    .catch(error => {
        manipulateContext?.release();
        resolve(response.assets || []); // Fallback to original assets if we want
    });

Or we can do it in the below catch

@DylanDylann DylanDylann Apr 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TMisiukiewicz What do you think about the above suggestion?

@TMisiukiewicz TMisiukiewicz Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah you are right we should release it in catch, good call 👍

);
}
return resolve(response.assets);
})
.catch((err) => reject(err));
} else {
Expand Down