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

Video Filters and Save Video #1306

Merged
merged 15 commits into from
Nov 21, 2018
Merged

Video Filters and Save Video #1306

merged 15 commits into from
Nov 21, 2018

Conversation

nickgzzjr
Copy link
Contributor

I added the ability to set video filters to the video player. This can be passed as a prop to the Component.

I also added the ability to save the video with the current filter using a saveAsync method that returns a promise.

I am open to changes and suggestions.

Copy link
Contributor

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all: Awesome job! Unfortunately it is very hard to see a diff because of all the reformatted code, could you please keep the current formatting?

@nickgzzjr
Copy link
Contributor Author

@n1ru4l Thanks! Yeah not a problem. AppCode kept reformatting it for me, but it should be good to go now.

@cobarx
Copy link
Contributor

cobarx commented Nov 5, 2018

Hi @nickgzzjr, thanks for submitting this. Having save functionality will be great, a lot of people want it!

I did a few minor updates to the docs and function names. Imo, calling is save() is ok since you cover that in the docs it doesn't need to be saveAsync().

On the filters, can we name them the same as they are on iOS (Noir, Sepia, Posterize, etc)? I'm a big fan of not modifying APIs so that if someone is looking at the iOS docs to figure out which filter they want, they don't have to translate names. I'd also like to support as many of the filters on the list as possible. I would put all of the filters in a FilterType.js file so that you just go FilterType.SEPIA like you can with TextTrackType.

I would also add a link to the iOS docs so developers can see what the filters look like:
https://developer.apple.com/library/archive/documentation/GraphicsImaging/Reference/CoreImageFilterReference/index.html

For saving, that needs a lot of documentation for it to be useful to other developers. A couple questions I'd have if I was using the API:

  • Does it work for non-MP4 content like HLS streams? How about .mov files?
  • Where is the file saved and how to do I play it back once saved?
  • When can I save? If there is a long video and it's not completely buffered, will it still work?
  • How do I remove the video when I no longer want it to be stored?
  • @n1ru4l recently added caching for iOS. Will this work when caching is enabled?

Can you update the docs to cover this and any other implementation details that you think people need to know.

@nickgzzjr
Copy link
Contributor Author

Hi @cobarx, thanks so much for you feedback, and your doc changes.

  • I went ahead and renamed saveAsync() to save().
  • I added the FilterType.js file with most of the filters from Apple as recommended. (I did not add the filters that required extra parameters)
  • I renamed the filter names to match the iOS docs
  • I updated the docs for both filter and save

I need to do further research and testing when saving videos that are buffering and cached. My guess is that it works just fine since it grabs the video from the player asset item.

@nickgzzjr
Copy link
Contributor Author

@cobarx I can confirm that saving cached videos works, as well as videos that are buffering. I have updated the docs to reflect my findings. I modified the video-caching example to test. Let me know if there are any other suggestions.

@cobarx
Copy link
Contributor

cobarx commented Nov 14, 2018

@nickgzzjr Thanks for taking the time to make the changes I requested. I finally had a chance to test the filters, those are very cool!

I ran into a couple issues with the filters. First, when I play a video with the filter turned on in the iOS Simulator, it's very jerky and is maxing out the CPU on my machine. Activity Monitor shows 200% cpu vs. 30% cpu with it off. Have you seen this when testing? Can you see if you can figure out what might be causing this?

Second, the filters don't appear to work for HLS playlists. If I play an MP4, the filter works but for HLS nothing happens. Here is a sample stream you can use for testing:
http://devimages.apple.com/iphone/samples/bipbop/bipbopall.m3u8
You will need to turn off App Transport Security since it's http:
https://github.com/react-native-community/react-native-video#ios-app-transport-security
If possible, I'd like to get this fixed as most companies doing video delivery are using HLS not mp4s and I'm trying to avoid having features that work in some scenarios but not others. However, if it can't be done, we can ship as is, just make sure to add a note to the docs.

@nickgzzjr
Copy link
Contributor Author

nickgzzjr commented Nov 18, 2018

@cobarx I am glad you liked them :)

The way filters work is that every video frame is processed with an image filter. This is why it uses so much CPU. I have only experienced "jerkyness" on the simulator but not an actual device. One workaround is to save the video first then load the output.

Unfortunately, I was not able to get the HLS playlist to work. I went ahead and added notes to the docs on these 2 things.

I also modified where the filter gets applied as it was conflicting with the repeat functionality. It's a known issue when using videoComposition and seeking. It would be nice if you could test the filters one last time with the repeat option as well.

@n1ru4l
Copy link
Contributor

n1ru4l commented Nov 20, 2018

  1. Just an idea: Maybe saving the videos could replace the video cache implementation that I made and the video caching could be completely done in userland?

  2. Do I understand correctly that if I save the video it appears on my Camera Roll/Photo App? if I am building a social network app (like instagram), I would prefer that not every single video is saved to the CameraRoll. If someone wanted this he could use CameraRoll (https://facebook.github.io/react-native/docs/cameraroll). I think this might be out of scope!

@cobarx cobarx merged commit 5e684d4 into TheWidlarzGroup:master Nov 21, 2018
beauner69 pushed a commit to beauner69/react-native-video that referenced this pull request Oct 10, 2019
Video Filters and Save Video

(rebased from commit 5e684d4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants