Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$16000] Feature Request: App should play the uploaded videos inline - reported by @murataka #7835

Closed mvtglobally closed 2 years ago

mvtglobally commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open any chat
  2. Send a video file
  3. Try to play video in chat

Expected Result:

App should play the uploaded videos inline

Actual Result:

User has to download video to play

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.39-1 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

Screen Shot 2022-02-06 at 08 27 17

Expensify/Expensify Issue URL: Issue reported by: @murataka Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644125263019529

View all open jobs on GitHub

mallenexpensify commented 2 years ago

Doubled price to $16000 https://www.upwork.com/jobs/~0103b194cebb2bdef4

gladiator-1 commented 2 years ago

it is a back-end issue, you can fix it by following this blog, it worked fine on safari browser without any issues.

you can test using this endpoint https://lionfish-app-nvp9x.ondigitalocean.app/video?uri=, i will let it for one day if someone would like to test.

https://user-images.githubusercontent.com/102553449/168660719-8718435e-e05c-433d-a18c-e5366199404c.mp4

mananjadhav commented 2 years ago

Thanks, @gladiator-1, looks like this resolves the issue for iOS Safari. I can see the proposal is fragmented into multiple comments. Would you mind sharing it with steps in one single comment, so that I can review and decide on that?

gladiator-1 commented 2 years ago

Proposal

Implementation

Web :

Native :

Props

Web :

Native :

Cons

Web :

Results

Web

https://user-images.githubusercontent.com/102553449/163058277-6ffcd4b0-03d1-4256-ab89-0cf8c9180332.mp4

Native (react-native-web-view)

https://user-images.githubusercontent.com/102553449/163058582-12fda4bb-cf49-4606-b926-9348835fbdc2.mp4

Native (react-native-video and react-native-video-player

https://user-images.githubusercontent.com/102553449/163058886-6361bae4-0950-4609-829f-95ab32ba6a9f.mp4

mananjadhav commented 2 years ago

Thanks, @gladiator-1. This was really helpful.

@stitesExpensify Based on the previous discussions, I am inclined with @gladiator-1's proposal of not using external lib. Do we need provisions to play Youtube/Vimeo files at the moment?

melvin-bot[bot] commented 2 years ago

@mananjadhav, @mallenexpensify, @stitesExpensify Eep! 4 days overdue now. Issues have feelings too...

mallenexpensify commented 2 years ago

@stitesExpensify can you review @mananjadhav comment above? https://github.com/Expensify/App/issues/7835#issuecomment-1131201855

mallenexpensify commented 2 years ago

@stitesExpensify is out sick so he might not be able to review til next week

stitesExpensify commented 2 years ago

I am inclined with @gladiator-1's proposal of not using external lib

I agree

Do we need provisions to play Youtube/Vimeo files at the moment?

I don't think so, but i'll double check with the product team.

@gladiator-1 I noticed in your cons you only mentioned needing an external library for web to play youtube videos, is that correct? Or would we have the same problem on mobile?

gladiator-1 commented 2 years ago

would we have the same problem on mobile?

No, we can use a third party script inside the react-native web view and play Youtube/Vimeo videos.

stitesExpensify commented 2 years ago

Okay then, for now let's just stick to the non-library implementation and we can add the required library for web in the future if that becomes a priority!

mananjadhav commented 2 years ago

@stitesExpensify Adding a comment for a proper confirmation. That we're going ahead with @gladiator-1's proposal.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

stitesExpensify commented 2 years ago

Yep, moving ahead with @gladiator-1's proposal!

melvin-bot[bot] commented 2 years ago

πŸ“£ @gladiator-1 You have been assigned to this job by @mallenexpensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

mallenexpensify commented 2 years ago

@gladiator-1 can you please apply for the job here and confirm once you have? https://www.upwork.com/jobs/~0103b194cebb2bdef4

gladiator-1 commented 2 years ago

@mallenexpensify applied with code "4635"

mallenexpensify commented 2 years ago

Hired @gladiator-1 and @mananjadhav in Upwork!

stitesExpensify commented 2 years ago

Not overdue, just hired a contributor

gladiator-1 commented 2 years ago

on small screen Android devices,: when we go back from ReportScreen, componentWillUnmount inside the ReportScreen won't call, there's nothing happens, we are actually on the same screen and only closing and opening the drawer, so if we play any video inside the ReportScreen and go back the video is still playing, to fix it we can put a webview video container as a video thumbnail in ReportScreen when we pressing it, it open a react-native-modal contains a webview which contains the video.

demo :

https://user-images.githubusercontent.com/102553449/172187185-a24518fc-1b89-4729-8add-485cea3703b0.mov

Is it Ok?

Also, we need a back engineer to fix this issue, we can't test on Safari and IOS.

stitesExpensify commented 2 years ago

I'm a bit confused, what needs to be done on the back end?

gladiator-1 commented 2 years ago

we need to stream portions of the video rather than streaming the entire video, if you download this repo you will find two nodeJS routes, first one is /works-in-chrome it streams the entire video file, the second one is /works-in-chrome-and-safari it streams portions of the video, if you start the server you will find that /works-in-chrome or localhost:3000/works-in-chrome works on chrome only but localhost:3000/works-in-chrome-and-safari works on both (chrome and safari), so if our server is a NodeJS server we need to make our route look like /works-in-chrome-and-safar rather than /works-in-chrome.

Referance : this blog

stitesExpensify commented 2 years ago

Hm okay, got it. I missed it the first time that there were extensive backend changes necessary.

I have 2 questions:

  1. We don't use node.js, we use PHP for our API layer. Do you know if there is a comparable method to fs.createReadStream(filePath, options); that we can use?
  2. We don't store video files ourselves, they are in an s3 bucket. Do you know if there is a way to only download video files partially from a bucket? Otherwise I'm concerned that each time the front end asks for a piece of the file, we will have to download the whole thing from amazon again just to send a small piece.
gladiator-1 commented 2 years ago

This code worked for me, I got it from this gist

Screen Shot 2022-06-07 at 4 46 41 PM

You can check if the request coming from an IOS device and the file format is [mp4,ogg,webM], download the file from s3 and save it in a temp folder, and for every request coming from an IOS device requires a [mp4,ogg,webM] file we check this temp folder first if the file does not exist download it from s3 and save it, then delete this temp folder content every few hours. (this should be considered as a temporary fix, the clients should get their files from s3 directly rather than causing a heavy load on the back end server.)

mananjadhav commented 2 years ago

@gladiator-1 Have you tried react-native-video for the iOS playback? I am also guessing it supports streaming via S3.

download the file from s3 and save it in a temp folder, and for every request coming from an IOS device requires a [mp4,ogg,webM] file we check this temp folder first if the file does not exist download it from s3

This isn't a good approach. It won't scale.

gladiator-1 commented 2 years ago

@mananjadhav I tried it before and didn't work. I think if the video runs on safari it will 100% run on IOS, the problem isn't in S3, I uploaded my video to a S3 bucket, you can play it on safari or any IOS device, here is the pre-signed URL, (it will be valid for 12 hours)

mallenexpensify commented 2 years ago

@gladiator-1 is working on, updating from Daily to Weekly cuz it's in progress

mananjadhav commented 2 years ago

I haven't got time to check this. but I'll check if there's a way to stream S3 videos directly to react-native-video. I read somewhere it does support, need to verify it once.

@stitesExpensify Do we foresee large videos in the platform? Is it absolutely necessary for us to stream partial videos?

gladiator-1 commented 2 years ago

@mananjadhav You can play S3 videos on Safari, react-native-webview or react-native-video without any issue.

https://user-images.githubusercontent.com/102553449/173901776-9a8c23e8-ca22-4afd-bb9d-60c6c2b5c2bb.mov

you can test it, should work on Safari without any issue (exp after 12h), So the issue isn't in S3.

mananjadhav commented 2 years ago

You can play S3 videos on Safari, react-native-webview or react-native-video without any issue.

@gladiator-1 I am a little confused. Please pardon my ignorance, I just want to get it right and get both of us on the same page . Are you saying that the S3 videos play with partial downloads? with react-native-webview and react-native-video?

If yes, then do we still need the changes from the backend?

gladiator-1 commented 2 years ago

Are you saying that the S3 videos play with partial downloads?

Yes, I think so because it works on Safari and IOS.

do we still need the changes from the backend?

Yes, while we can't play our videos on Safari (as in the following video) we need backend changes because videos not working on Safari = not working on Desktop = not working on IOS.

https://user-images.githubusercontent.com/102553449/173927305-7c5d5823-3138-4a8c-8950-140c4986a06a.mov

note: my repo is ready for PR we can test it while we are waiting for the issue to get fixed and let me know if there are any changes required. (you will need to add isVideo method in Str.js file).

mananjadhav commented 2 years ago

Okay!! So you're saying, the S3 videos that you've tried are working fine on iOS, Safari and Mac, with partial download. The videos hosted by Expensify are the only ones not working. Is my understanding correct?

note: my repo is ready for PR

Let me review this on the weekend.

you will need to add isVideo method in Str.js file

For the above change, you can fork and create a PR against expensify-common. After it is merged, we take the hash and update it expensify/App -> package.json.

gladiator-1 commented 2 years ago

Okay!! So you're saying, the S3 videos that you've tried are working fine on iOS, Safari and Mac, with partial download. The videos hosted by Expensify are the only ones not working. Is my understanding correct?

Exactly, this is what I mean.

you can fork and create a PR against expensify-common.

I did it here, waiting for the merge.

mananjadhav commented 2 years ago

Okay!! So you're saying, the S3 videos that you've tried are working fine on iOS, Safari and Mac, with partial download. > > > The videos hosted by Expensify are the only ones not working. Is my understanding correct?

Exactly, this is what I mean.

Ahh! Thank you. I am assuming directly posting the S3 URL, no Nodejs/PHP server in between.

gladiator-1 commented 2 years ago

I am assuming directly posting the S3 URL, no Nodejs/PHP server in between.

If we are using the S3 pre-signed URL, this means our PHP server is only redirecting the request to this URL, what happens is :

  1. The client sends a request to our PHP server "I need this file test.mp4".
  2. Our PHP server send a request to the S3 "Could you create a pre-signed URL valid for a few minutes for this file test.mp4"
  3. The S3 send the response ''Here is the URL .......'
  4. Our PHP server redirect the first request sent from the client to this URL.
stitesExpensify commented 2 years ago

I have not looked into this yet due to newsletter 7 priorities

mananjadhav commented 2 years ago

I'll be taking a deeper look at the implementation possibilities by next week.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

stitesExpensify commented 2 years ago

I have not had a chance to look into this further. This may require extensive internal work and it is not currently a priority.

JmillsExpensify commented 2 years ago

Agreed, I wonder if we should at least apply the Exported label so that melvin won't keep pushing this back to daily.

stitesExpensify commented 2 years ago

hm it looks like the exported label is already on here, so I'm not sure what to do @JmillsExpensify πŸ˜…

JmillsExpensify commented 2 years ago

Hmm, yeah I guess I'm also unclear what the logic is. Let's see if that worked.

JmillsExpensify commented 2 years ago

Still not a priority. @stitesExpensify Add anything else I might not be aware of or have missed!

stitesExpensify commented 2 years ago

Same as last week. Focusing on n7 to get offline first out the door!

jeet-dhandha commented 2 years ago

Anyone working on this one ?

stitesExpensify commented 2 years ago

@gladiator-1 has provided us with a potential solution but it takes backend effort that we have not yet prioritized. If there is a purely front-end solution that would be preferable, but it doesn't seem like that is the case.

jeet-dhandha commented 2 years ago

I was working on a front end solution but might take some time will provide you in 1-2 days. It will include.

  1. Inline video run.
  2. With new screen video run too.
stitesExpensify commented 2 years ago

Any update @jeet-dhandha ?

JalalMitali commented 2 years ago

I am working on a possible front end solution :)

JalalMitali commented 2 years ago

PROPOSAL : Hello Team, Today I Created a Platform Specific Video Player That's Already integrated in a Bare React Native CLI App ( Just Like Expensify )

Native Android Module ( Adding iOS & MacOS ) :

The integration is now in android only this is just a proof of work I can make iOS and MacOS in short time too!

Screen Shot 2022-08-08 at 00 29 46

cc : @stitesExpensify && @JmillsExpensify

stitesExpensify commented 2 years ago

To clarify @JalalMitali you wrote an actual video player component? It is purely in JS or is it native? Just trying to get an idea of your proposal since there isn't any code provided