Skip to content
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.

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

[$500] Chat - The video is reloaded every time you open it #37168

Closed
4 of 6 tasks
izarutskaya opened this issue Feb 24, 2024 · 66 comments
Closed
4 of 6 tasks

[$500] Chat - The video is reloaded every time you open it #37168

izarutskaya opened this issue Feb 24, 2024 · 66 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@izarutskaya
Copy link

izarutskaya commented Feb 24, 2024

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


Found when validating PR : #37036

Version Number: v1.4.43-18
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation:

Action Performed:

  1. Open the app
  2. Login with any account
  3. Go to any chat
  4. Plus button -> Add attachment -> Send a video
  5. Open the video that you just sent
  6. Tap the back button
  7. Open the video again

Expected Result:

Loading spinner is displayed only when you open the video for the first time

Actual Result:

The video is reloaded every time you open it

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

Bug6390764_1708771155473.Record_2024-02-23-23-25-53_4f9154176b47c00da84e32064abf1c48.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018db53b84762ad824
  • Upwork Job ID: 1761342164490567680
  • Last Price Increase: 2024-03-30
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 24, 2024
@melvin-bot melvin-bot bot changed the title Chat - The video is reloaded every time you open it [$500] Chat - The video is reloaded every time you open it Feb 24, 2024
Copy link

melvin-bot bot commented Feb 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~018db53b84762ad824

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 24, 2024
Copy link

melvin-bot bot commented Feb 24, 2024

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

Copy link

melvin-bot bot commented Feb 24, 2024

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

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Feb 24, 2024
Copy link

melvin-bot bot commented Feb 24, 2024

Triggered auto assignment to @Gonals (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@izarutskaya
Copy link
Author

We think that this bug might be related to #vip-vsb
CC @quinthar

@webbdays
Copy link

webbdays commented Feb 24, 2024

This shows no caching in the app.

Copy link

melvin-bot bot commented Feb 24, 2024

📣 @webbdays! 📣
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.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@brandonhenry
Copy link
Contributor

brandonhenry commented Feb 24, 2024

Proposal

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

The problem we are trying to solve is that the video attachment in the chat is reloaded every time it is opened, instead of being cached after the first load. This results in unnecessary network requests and a suboptimal user experience. This only happens on the mobile version of the app.

What is the root cause of that problem?

The root cause of the problem is that the video is not being cached on the device after the first load in the AttachmentViewVideo component. Every time the video is opened, it is fetched from the server again, causing the reload. This is a common issue for react native and caching videos.

function AttachmentViewVideo({source, isHovered, shouldUseSharedVideoElement, videoDuration}) {

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

To solve the problem, we should implement a caching mechanism for the video files. When a video is opened for the first time, it should be downloaded and cached on the device. Subsequent openings of the video should use the cached version instead of fetching it from the server again.

Thankfully, since we are already using expo, we can just make use of expo's file system.

  1. Import the necessary functions from expo-file-system:
import * as FileSystem from 'expo-file-system';
  1. Create a utility function to check if the video is cached and fetch/cache it if necessary:
const MAX_CACHE_SIZE = 500 * 1024 * 1024; // 500MB
const CACHE_DIR = `${FileSystem.cacheDirectory}videos/`;

const fetchAndCacheVideo = async (videoUri) => {  
  const fileName = videoUri.split('/').pop();
  const videoPath = `${CACHE_DIR}${fileName}`;

  // Check if the video is already cached
  const isCached = await FileSystem.getInfoAsync(videoPath).then(
    (fileInfo) => fileInfo.exists
  );

  if (isCached) {
    // Load the cached video
    return videoPath;
  } else {
    // Ensure the cache directory exists
    await FileSystem.makeDirectoryAsync(CACHE_DIR, { intermediates: true });
    
    // Check available space and clean cache if needed
    await cleanCacheIfNeeded();

    // Fetch and cache the video
    const downloadResult = await FileSystem.downloadAsync(
      videoUri,
      videoPath
    );

    if (downloadResult.status === 200) {
      return videoPath;
    } else {
      throw new Error('Failed to fetch video');
    }
  }
};

const getCacheSize = async () => {
  const files = await FileSystem.readDirectoryAsync(CACHE_DIR);
  const sizes = await Promise.all(
    files.map(file => FileSystem.getInfoAsync(`${CACHE_DIR}${file}`))
  );
  
  return sizes.reduce((total, {size}) => total + size, 0); 
};

const cleanCacheIfNeeded = async () => {
  const cacheSize = await getCacheSize();
  
  if (cacheSize > MAX_CACHE_SIZE) {
    const files = await FileSystem.readDirectoryAsync(CACHE_DIR);
    const fileInfos = await Promise.all(
      files.map(file => FileSystem.getInfoAsync(`${CACHE_DIR}${file}`))  
    );
    
    // Sort by modificationTime ascending to remove oldest first
    fileInfos.sort((a,b) => a.modificationTime - b.modificationTime);
    
    let sizeToClear = cacheSize - MAX_CACHE_SIZE;
  
    for (const {uri, size} of fileInfos) {
      await FileSystem.deleteAsync(uri);
      sizeToClear -= size;
      
      if (sizeToClear <= 0) {
        break;
      }
    }
  }
};

The fetchAndCacheVideo function checks if the video is already cached in the app's cache directory (FileSystem.cacheDirectory). If the video is cached, it returns the cached file path. If not, it creates a videos/ directory in the cache directory (if it doesn't exist), fetches the video from the provided videoUri, and caches it in the videos/ directory.

UPDATE:

Two new utility functions are added:

  • getCacheSize recursively gets the total size of all files in the video cache directory
  • cleanCacheIfNeeded checks if the cache exceeds MAX_CACHE_SIZE (set to 500MB here) and if so, deletes the oldest videos until the size is below the limit. The check is performed before caching a new video.

This approach limits the video cache to a maximum size and automatically removes the oldest cached videos when that limit is reached. The specific size limit and cache location can be adjusted as needed.

@webbdays
Copy link

Note:
Streaming the video is the best thing to do than download whole video at a time.
Its best to achieve this by doing things like when video plays for the first time we get the stream segments. As we get more and more segments we join them to form a video. This helps us to get the video and users sees the video without waiting long time instead of waiting for full long video to download at first.

Second when we add caching, cache invalidation is very important. Adding functionality like ask the server "is the video/file modified?" before asking for video. (This needs some server side changes(very minor)).

@Skalakid
Copy link
Contributor

Skalakid commented Feb 26, 2024

@izarutskaya @DylanDylann @greg-schroeder I'm not sure if it is a bug, I think it is more like a new feature for the video player. The presented behavior is correct according to the video player design doc. Video caching wasn't planned during the predesign. I think it might be a great improvement! What do you think about it @francoisl?

@brandonhenry video player work differently depending on the screen size. On small screens, they are only usable inside the attachment modal. On large screens video players are available inside chat. When extending the video we are sharing the component to the attachment carousel to optimize video loadings (currently it is broken on main but I created a fix for it). Because of that, I think we should cache only videos that have shouldUseSharedVideoElement set to false.
Also, videos may have bigger sizes, so I think it would be nice to limit the amount of videos that are currently cached so a user won't run out of memory. What's your thoughts on that?

Copy link

melvin-bot bot commented Mar 16, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Mar 16, 2024
Copy link

melvin-bot bot commented Mar 16, 2024

@Gonals @greg-schroeder @DylanDylann this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@brandonhenry
Copy link
Contributor

brandonhenry commented Mar 16, 2024

i can put together a working POC this weekend. Trying to get my iOS build / mWeb localhost working but having issues with websocket... trying to get that resolved quickly so i can get a PR merged but will get back to this (getting 200+ lottie errors when building ios but everything else builds fine)

Edit: Fixed the below issues. I just needed to update my Mac to latest vesrsion and swap to XCode 15.1

image image

@greg-schroeder
Copy link
Contributor

conversation ongoing

@melvin-bot melvin-bot bot removed the Overdue label Mar 17, 2024
@DylanDylann
Copy link
Contributor

Discussing on Slack

Copy link

melvin-bot bot commented Mar 23, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Mar 23, 2024
@webbdays
Copy link

webbdays commented Mar 24, 2024

@DylanDylann
@brandonhenry
any outcome?
Thank you.

@DylanDylann
Copy link
Contributor

DylanDylann commented Mar 25, 2024

Discussing on Slack

@melvin-bot melvin-bot bot removed the Overdue label Mar 25, 2024
@greg-schroeder
Copy link
Contributor

This might be moved Internal per discussion - at any rate it's definitely chat-related (but not expense related) so I'm going to put this into #vip-vsb

Copy link

melvin-bot bot commented Mar 30, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Mar 30, 2024
@DylanDylann
Copy link
Contributor

DylanDylann commented Apr 1, 2024

@francoisl We decided to handle this issue internally. Should we close this one?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 1, 2024
@greg-schroeder
Copy link
Contributor

I'm going to close it per that discussion ... please reopen if you disagree.

@lanitochka17
Copy link

Issue is still reproducible on the latest build 1.4.74-0

vide.mp4

@lanitochka17 lanitochka17 reopened this May 15, 2024
@melvin-bot melvin-bot bot added the Overdue label May 15, 2024
@greg-schroeder
Copy link
Contributor

Thanks @lanitochka17 - we're just handling it in the linked issue above, so we can leave this closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

10 participants