Details

Reviewers

bartek
tomek

Commits

rCOMM7d9b1e286785: [native][RN80] Fix media gallery memory usage

Summary

https://linear.app/comm/issue/ENG-11020/media-gallery-memory-issue

Used solution from https://github.com/TheWidlarzGroup/react-native-video/issues/4497 with onEnd callback.
Also disableFocus prop docs fixed issue with video not playing automatically.

Depends on D15133

Test Plan

Open media gallery

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Jul 24 2025, 9:11 AM

angelika held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 24 2025, 9:11 AM

Harbormaster completed remote builds in B34793: Diff 48998.Jul 24 2025, 9:13 AM

Rebase

Harbormaster completed remote builds in B34977: Diff 49184.Jul 24 2025, 4:43 PM

Harbormaster completed remote builds in B34978: Diff 49185.

angelika retitled this revision from [native][RN80][skip-ci] Fix media gallery memory usage to [native][RN80] Fix media gallery memory usage.Jul 24 2025, 7:10 PM

Trigger ci

Harbormaster failed remote builds in B34979: Diff 49186!Jul 24 2025, 7:21 PM

angelika added a child revision: D15137: [ci] CI fixes.Jul 25 2025, 8:53 AM

angelika published this revision for review.Jul 25 2025, 10:08 AM

Perhaps we could set windowSize=2 to have a balance between memory performance and possible flickering during scroll, caused by only one media being loaded at once?

In D15134#415524, @bartek wrote:

Perhaps we could set windowSize=2 to have a balance between memory performance and possible flickering during scroll, caused by only one media being loaded at once?

The default is 21, so reducing to 1 or 2 sounds extreme - I expect the blank spaces to frequently appear. Can we play with this value a bit and test how the app feels?

This revision now requires changes to proceed.Aug 5 2025, 10:43 AM

Rebase & review changes

Harbormaster failed remote builds in B35242: Diff 49493!Aug 9 2025, 12:39 PM

This diff was probably moved to needs review by a mistake

This revision now requires changes to proceed.Aug 11 2025, 3:28 PM

Better fix

Harbormaster failed remote builds in B35289: Diff 49562!Aug 15 2025, 5:02 AM

angelika edited the summary of this revision. (Show Details)Aug 15 2025, 5:02 AM

Looks great! I'm wondering if there are any differences in behavior compared to what we currently have in our app. Does anything change?

native/media/media-gallery-media.react.js
232–235 ↗	(On Diff #49562)	I think our videos have a `loop` property that could be considered here. Not sure if we're using it at all, though.
233–235 ↗	(On Diff #49562)	This has to be a callback

This revision now requires changes to proceed.Aug 15 2025, 5:28 PM

In D15134#418100, @tomek wrote:

Looks great! I'm wondering if there are any differences in behavior compared to what we currently have in our app. Does anything change?

Nothing changes. The video still autoplays and always repeats as before. disableFocus={true} Allows background audio/music from other apps to continue playing on Android but our videos are muted anyway so it's not an issue.

native/media/media-gallery-media.react.js
232–235 ↗	(On Diff #49562)	I think our videos have a loop property that could be considered here. Not sure if we're using it at all, though. We're not displaying our videos here, just videos returned from expo's MediaLibrary. And it makes sense that they're always looped: when user wants to send a video, they want to have a constant preview of what the video contains.