Open lanitochka17 opened 1 month ago
Triggered auto assignment to @twisterdotcom (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@twisterdotcom FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
My days, what a horrible bug. It's like it's set to max max volume by default. It's so tinny and loud.
https://github.com/user-attachments/assets/510e8643-3e24-4aee-8a4e-a943f14a399a
Job added to Upwork: https://www.upwork.com/jobs/~012cf16a806465ece4
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mkhutornyi (External
)
Onboarding - Onboarding video ignores mute and doesn't stop playing in background
We didn't pass the initial value to the Expo video for volume and isMuted, we only pass the value if we update the volume and invoke the updateVolume
function
https://github.com/Expensify/App/blob/05e044a98e71ccc1d8ae7a05fac754fc8162f884/src/components/VideoPlayer/BaseVideoPlayer.tsx#L181
https://github.com/Expensify/App/blob/05e044a98e71ccc1d8ae7a05fac754fc8162f884/src/components/VideoPlayer/BaseVideoPlayer.tsx#L344-L370
We do not listen to React navigation when the screen is blurred when we navigate to another screen, we do not pause the video
We can pass volume and isMuted properties for the initial value to the Video expo element And listen to React Navigation when the screen is blurred we can pause the Video element
# Listen to React Navigation
useEffect(() => {
const unsubscribe = navigation.addListener('blur', () => {
pauseVideo()
});
return unsubscribe;
}, [navigation, pauseVideo]);
# Video Element
const {updateVolume, volume} = useVolumeContext();
<Video
ref={videoPlayerRef}
style={[styles.w100, styles.h100, videoPlayerStyle]}
videoStyle={[styles.w100, styles.h100, videoStyle]}
source={{
// if video is loading and is offline, we want to change uri to "" to
// reset the video player after connection is back
uri: !isLoading || (isLoading && !isOffline) ? sourceURL : '',
}}
shouldPlay={shouldPlay}
useNativeControls={false}
resizeMode={resizeMode as ResizeMode}
isLooping={isLooping}
onReadyForDisplay={(e) => {
if (isCurrentlyURLSet && !isUploading) {
playVideo();
}
onVideoLoaded?.(e);
const {source} = videoPopoverMenuPlayerRef.current?.props ?? {};
if (typeof source === 'number' || !source || source.uri !== sourceURL) {
return;
}
videoPlayerRef.current?.setStatusAsync?.({rate: currentPlaybackSpeed});
}}
onPlaybackStatusUpdate={handlePlaybackStatusUpdate}
onFullscreenUpdate={handleFullscreenUpdate}
volume={volume.value}
isMuted={volume.value === 0}
/>
Proposal updated
@mkhutornyi Please kindly review above proposal
Bump on this @mkhutornyi.
reviewing
@NJ-2020 please avoid code diff in proposal as stated in guideline. And share demo video which fixes 2 issues:
Onboarding video ignores mute and doesn't stop playing in background.
The onboarding video itself works fine with the mute and unmute buttons. However, the main issue is that another instance of the video is running in the background, which is not visible in the UI. This occurs due to the inconsistent ref value in videoPlayerRef
throughout the render lifecycle. As a result, multiple instances of the video play simultaneously, causing overlapping audio.
In the provided video below, you can hear multiple audio tracks playing, indicating the presence of an additional video instance running in the background. (Turn the sound on)
https://github.com/user-attachments/assets/868a4e61-c286-43b4-8a7a-33053654d929
Try this steps to reproduce:
By "inconsistent ref values," I mean that the videoPlayerRef
somehow is losing its correct value and changing to {"current": null}. Please refer to the image below for clarification.
In the code above, shouldPlay
is set to true
. Before we obtain the correct refs for both videoPlayerRef
and currentVideoPlayerRef
, the player starts to play since shouldPlay
is true from the beginning. When we eventually get the correct refs for both videoPlayerRef
and currentVideoPlayerRef
, the player has already started playing with an incorrect ref. As a result, once we have the updated ref values, we lose control over the instance that started when videoPlayerRef.current
was null, causing it to play in the background.
The mute and unmute functionality works on the video that is currently displayed in the UI, but not on the one running in the background. This is because we are manipulating the updated and current ref, i.e., currentVideoPlayerRef
, in the code below:
To solve the problem, we should modify the shouldPlay
prop to include an additional condition that checks whether both videoPlayerRef.current
and currentVideoPlayerRef.current
are valid by changing below code condition of shouldPlay
:
to
shouldPlay={shouldPlay && (!!videoPlayerRef.current && !!currentVideoPlayerRef.current)}
By doing so, we ensure that the video only plays when both references are correctly initialized and valid.
https://github.com/user-attachments/assets/75d83d05-66b6-4cb8-ab5b-0a684936b887
NA
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@tushar-a-b thanks for the detailed explanation.
By "inconsistent ref values," I mean that the videoPlayerRef somehow is losing its correct value and changing to {"current": null}.
do you know why this happens only when navigate to other page before video opens? as you know, this bug doesn't happen without navigating to other pages.
do you know why this happens only when navigate to other page before video opens? as you know, this bug doesn't happen without navigating to other pages.
Hi @mkhutornyi,
It looks like the video tries to play even when we don't navigate, but it fails somehow. We have an error in the console indicating that it attempted to play but failed. I am attaching an image below.
@mkhutornyi Sorry for the delay, When trying the solution it didn't work, I will try to look into it
We have an error in the console indicating that it attempted to play but failed.
@tushar-a-b ok, then why doesn't this happen in the case of navigating to other page, which leads to playing video?
@mkhutornyi It is unclear for me too. But it is clear that we have another media playing in background which we have no control to stop. Although if we do not provide the extra check to shouldPlay
as I mentioned in proposal and just provide the prop isMuted=true
to Video
component it also solves the problem, but who knows media is playing in background and is muted.
Edit:- While debugging, I noticed that sometimes there is no background play even if we navigate before the onboarding video, and other times I observed a brief background play that is quickly paused by the browser when we stay on the home screen. This inconsistent behavior suggests that it might be a browser issue in how it handles media elements, especially when dealing with rapid navigation changes and media initialization. It could be that the browser is delaying or suppressing media playback in certain conditions to optimize resource usage or prevent multiple media elements from playing simultaneously.
@twisterdotcom, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!
Waiting for more proposal with clear root cause.
@bernhardoj @nkdengineer @dominictb @daledah
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@twisterdotcom @mkhutornyi this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@twisterdotcom, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Still waiting
@twisterdotcom, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!
What do we all think here? Do we think it's a pretty niche flow, given it requires quick action and isn't always reproducible? If so, we could close?
I'm only reticent because it was honestly, so, very very annoying. But refreshing did get rid of it.
@twisterdotcom I agree that the issue may seem niche due to the specific steps required to reproduce it, but I’m concerned about the potential frustration it could cause for users who encounter it. Also, I believe we are removing the welcome video from the onboarding flow in issue #47369, which might mitigate this problem altogether.
Ah nice. Okay, let's hold on that and make this Monthly for now.
Probably should move this to f1 if it does need to be pursued.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.14-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Onboarding video is played correctly and adheres to mute
Actual Result:
Video opens with sound playing even though mute is activated. Sound continues to play on repeat even though user has closed video
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/d68882b1-61b3-48bb-b598-f00d4d0088e7
View all open jobs on GitHub
Upwork Automation - Do Not Edit