[$500] Web - Video - play speed after returning from thread does not update

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.47-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any chat.
3. Send a video.
4. Send a reply to the video.
5. Send a reply to the reply in the video in Step 3.
6. In thread, play the video.
7. Click outside the video to return to the previous thread.
8. Click 3-dot menu > Playback speed.
9. We can hear the sound from previous chat
10. Change the speed or volume
11. Observe that playback speed or volume does not update

Expected Result:

Step 9: We should not hear the sound from previous chat
Step 11: Playback speed or volume should update

Actual Result:

Step 9: We can hear the sound from previous chat
Step 11: Playback speed or volume does not updated

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6402303_1709610042243.bandicam_2024-03-05_10-46-31-988.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fabf464f1c4f13cc
• Upwork Job ID: 1768354460890284032
• Last Price Increase: 2024-03-14
• Automatic offers:
• dukenv0307 | Contributor | 0
• tienifr | Contributor | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[izarutskaya]

@MitchExpensify 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.

[CristiCeban]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Video - App crashes when changing play speed after returning from thread

What is the root cause of that problem?

When returning from the tread and changing the playback speed of the video currentVideoPlayerRef.current.setStatusAsync({rate: speed}); cause crash because currentVideoPlayerRef is null there.

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.js

Lines 21 to 27 in 9411a08

	const updatePlaybackSpeed = useCallback(
	(speed) => {
	setCurrentPlaybackSpeed(speed);
	currentVideoPlayerRef.current.setStatusAsync({rate: speed});
	},
	[currentVideoPlayerRef],
	);

This is because videoRef comes from PlaybackContext here

App/src/components/VideoPlayerContexts/PlaybackContext.js

Line 105 in 9411a08

currentVideoPlayerRef,

When returning from the threads, it resets in this useEffect

App/src/components/VideoPlayerContexts/PlaybackContext.js

Lines 92 to 97 in 9411a08

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

and basically resets the player ref in this function

App/src/components/VideoPlayerContexts/PlaybackContext.js

Lines 83 to 90 in 9411a08

	const resetVideoPlayerData = useCallback(() => {
	stopVideo();
	unloadVideo();
	setCurrentlyPlayingURL(null);
	setSharedElement(null);
	setOriginalParent(null);
	currentVideoPlayerRef.current = null;
	}, [stopVideo, unloadVideo]);

thus, the running video has no valid ref and trying to call functions on it, crashes the app

What changes do you think we should make in order to solve the problem?

In the initial chat and in the other chat thread the currentReportID is different.

App/src/components/VideoPlayerContexts/PlaybackContext.js

Lines 92 to 97 in 9411a08

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

I don't see the point of resetting it when it is valid, only when it is null.
So I propose to change it to

 useEffect(() => {
        if (currentReportID) {
            return;
        }
        resetVideoPlayerData();
    }, [currentReportID, resetVideoPlayerData]);

Before

Screen.Recording.2024-03-05.at.14.28.48.mov

After

Screen.Recording.2024-03-05.at.14.29.39.mov

[melvin-bot]

📣 @CristiCeban! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[CristiCeban]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01b6ea444945773abe

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[MitchExpensify]

Reproduced:

[MitchExpensify]

Wave Collect worthy I think: https://expensify.slack.com/archives/C036QM0SLJK/p1709924791377559

[melvin-bot]

@MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MitchExpensify]

Bumping our fearless wave collect leader here https://expensify.slack.com/archives/C036QM0SLJK/p1710260567333629?thread_ts=1709924791.377559&cid=C036QM0SLJK

[MitchExpensify]

Confirming if changing the playback speed is possible as part of stage 1 and stage 2 plans as a next step

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01fabf464f1c4f13cc

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

In the original OP, the app crashes when changing the video playback speed after opening the thread parent. Now, the ap doesn't crash, but the playback speed button doesn't work at all including the other video controls button (volume & download)

What is the root cause of that problem?

If we start playing a video, it will set the currentlyPlayingURL context to the video URL (A_url) and set the video player ref from this useEffect (url is now equal to currentlyPlayingURL context).

App/src/components/VideoPlayer/BaseVideoPlayer.js

Lines 176 to 181 in 8b54aff

	useEffect(() => {
	if (shouldUseSharedVideoElement || url !== currentlyPlayingURL) {
	return;
	}
	shareVideoPlayerElements(videoPlayerRef.current, videoPlayerElementParentRef.current, videoPlayerElementRef.current, isUploading);
	}, [currentlyPlayingURL, shouldUseSharedVideoElement, shareVideoPlayerElements, updateSharedElements, url, isUploading]);

currentlyPlayingURL = A_url
video player ref = A_video_ref

When we press the thread parent message, it will navigate to the thread parent and mount a new report screen and also the video player preview (which will render the video player) which triggers the above useEffect again.

We have a logic to reset the video player data, including the currentlyPlayingURL and the ref every time we navigate to another report.

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 80 to 85 in 8b54aff

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

However, the current report ID value is updated with a delay and also the logic to clear the video player data is executed in a useEffect that depends on the currentReportID state.

App/src/libs/Navigation/NavigationRoot.tsx

Lines 120 to 122 in 8b54aff

	// Performance optimization to avoid context consumers to delay first render
	setTimeout(() => {
	currentReportIDValue?.updateCurrentReportID(state);

When the video player (preview) is mounted, the currentlyPlayingURL still contains the previous video URL (A_url) and it's actually the same as the current video url (A_url),

So the isThumbnail becomes false which will render the video player component and the shareVideoPlayerElements is called and the video is auto-played.

App/src/components/VideoPlayerPreview/index.tsx

Lines 62 to 67 in df28d85

	useEffect(() => {
	if (videoUrl !== currentlyPlayingURL) {
	return;
	}
	setIsThumbnail(false);
	}, [currentlyPlayingURL, updateCurrentlyPlayingURL, videoUrl]);

After the current report ID is updated, the video player data is cleared.
currentlyPlayingURL = null
video player ref = null

So, every time we try to update the playback speed, it does nothing because the video player ref now is null.

Also, when navigating back to the thread parent report, we can still hear the previous video sound because it's not stopped yet and it's because the stopAsync function here is always undefined.

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 23 to 24 in e504806

	const stopVideo = useCallback(() => {
	currentVideoPlayerRef.current?.stopAsync?.();

What changes do you think we should make in order to solve the problem?

The idea is to prevent rendering the video player component when the reportID of the current video is different than the reportID of the previous video.

1. First, pass reportID from VideoRenderer to VideoPlayerPreview.
   

   App/src/components/HTMLEngineProvider/HTMLRenderers/VideoRenderer.tsx

   Lines 28 to 31 in 2ce785c

   	{({report}) => (
   	<VideoPlayerPreview
   	key={key}
   	videoUrl={sourceURL}

reportID={report?.reportID}

2. Next, create a new state called currentlyPlayingURLReportID in PlaybackContext.

const [currentlyPlayingURLReportID, setCurrentlyPlayingURLReportID] = useState<string | undefined>();

3. Set currentlyPlayingURLReportID in updateCurrentlyPlayingURL and clears it in resetVideoPlayerData

setCurrentlyPlayingURLReportID(currentReportID);

Don't forget to pass currentlyPlayingURLReportID to the context value.

4. In VideoPlayerPreview useEffect, returns early if the reportID doesn't match..
   

   App/src/components/VideoPlayerPreview/index.tsx

   Lines 62 to 67 in 2ce785c

   	useEffect(() => {
   	if (videoUrl !== currentlyPlayingURL) {
   	return;
   	}
   	setIsThumbnail(false);
   	}, [currentlyPlayingURL, updateCurrentlyPlayingURL, videoUrl]);

if (videoUrl !== currentlyPlayingURL || reportID !== currentlyPlayingURLReportID) {
    return;
}

And to fix the undefined stopAsync, we can replace it with its equivalent.

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Line 24 in 2ce785c

currentVideoPlayerRef.current?.stopAsync?.();

currentVideoPlayerRef.current?.setStatusAsync?.({shouldPlay: false, positionMillis: 0});

Don't forget to update each useEffect and useCallback that is affected by above changes

What alternative solutions did you explore? (Optional)

If the resetVideoPlayerData is called earlier before the video player is mounted, then the thread parent video won't be auto played. If the video isn't auto played, then the user need to press the play button which will set the correct video player ref and we won't have the issue of null ref.

So, I suggest to call resetVideoPlayerData as soon as the nav state changes instead of waiting for the state to re-trigger the useEffect.

const currentReportIDRef = useRef('');

useEffect(() => {
    const listener = navigationRef.addListener('state', () => {
        const newCurrentReportID = Navigation.getTopmostReportId(navigationRef.getRootState()) ?? '';
        if (currentReportIDRef.current !== newCurrentReportID) {
            resetVideoPlayerData();
        }
        currentReportIDRef.current = newCurrentReportID;
    });
    return navigationRef.removeListener('state', listener);
}, [])

Then, we don't need the effect anymore

App/src/components/VideoPlayerContexts/PlaybackContext.tsx

Lines 83 to 88 in e504806

	useEffect(() => {
	if (!currentReportID) {
	return;
	}
	resetVideoPlayerData();
	}, [currentReportID, resetVideoPlayerData]);

Last, we should set the thumbnail to true when the url doesn't match.

App/src/components/VideoPlayerPreview/index.tsx

Lines 62 to 67 in df28d85

	useEffect(() => {
	if (videoUrl !== currentlyPlayingURL) {
	return;
	}
	setIsThumbnail(false);
	}, [currentlyPlayingURL, updateCurrentlyPlayingURL, videoUrl]);

if (videoUrl !== currentlyPlayingURL) {
    setIsThumbnail(true);
    return;
}

[bernhardoj]

The app doesn't crash anymore, but the root cause isn't fixed yet as shown in the video below. The playback speed button, volume, and download doesn't have any effect.

Untitled.1.mov

[MitchExpensify]

Do we all agree this is the new issue?

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any chat.
3. Send a video.
4. Send a reply to the video.
5. Send a reply to the reply in the video in Step 3.
6. In thread, play the video.
7. Click outside the video to return to the previous thread.
8. Click 3-dot menu > Playback speed.
9. Change the speed.

Expected Result:

Playback speed should update

Actual Result:

Playback speed does not update

[dukenv0307]

@MitchExpensify Here's my suggestion:

1. Go to staging.new.expensify.com
2. Go to any chat.
3. Send a video.
4. Send a reply to the video.
5. Send a reply to the reply in the video in Step 3.
6. In thread, play the video.
7. Click outside the video to return to the previous thread.
8. Click 3-dot menu > Playback speed.
9. We can hear the sound from previous chat
10. Change the speed or volume
11. Observe that playback speed or volume does not update

Expected Result:

Step 9: We should not hear the sound from previous chat
Step 11: Playback speed or volume should update

Actual Result:

Step 9: We can hear the sound from previous chat
Step 11: Playback speed or volume does not updated

[dukenv0307]

@bernhardoj This solution didn't work on my side. I still can hear the sound.

@tienifr Do you want to update your proposal to fix that case?

[MitchExpensify]

Nice, updated!

[dukenv0307]

bump @bernhardoj @tienifr

[bernhardoj]

Updated my proposal

[dukenv0307]

will review today

[dukenv0307]

@bernhardoj Can you open the PR, so I can test it easier. BTW, your solution doesn't work after backing to the parent chat at first time, 2 videos play simultaneously. Did you face this problem?

[bernhardoj]

@dukenv0307 I don't remember encountering that problem when I was testing my solution, but I have posted a new solution that is more reliable.

[dukenv0307]

@bernhardoj Can you pls open the PR since you already got assigned?

[bernhardoj]

PR is here

[melvin-bot]

This issue has not been updated in over 15 days. @jasperhuangg, @MitchExpensify, @bernhardoj, @tienifr, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[jasperhuangg]

Looks like the fix for this was deployed 2 weeks ago, it seems the automation failed.

[jasperhuangg]

@MitchExpensify I believe we need to pay out @bernhardoj for their PR solution and @dukenv0307 for the review

[MitchExpensify]

Payment summary:

$500 C @bernhardoj (Upwork)
$500 C @tienifr (Upwork)
$500 C+ @dukenv0307 (Upwork)