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

Basic <Video> tag implementation #466

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@brentvatne
Collaborator

brentvatne commented Mar 30, 2015

Put this together together today with the help of @tadeuzagallo, far from complete but you can see it in action in my react-native-login repo.

Special props beyond the standard RCTView:

  • resizeMode with the same options as Image
  • source which just accepts a string currently, which is then used to locate the resource locally.

demo gif

Some remaining tasks:

  • Support require('video!...')
    • Support other extensions than just mp4?
  • Add prop to set repeat (none or forever)
  • Restart video when view becomes active again?
  • Decide how far to go with this PR - many more properties and options we could potentially implement
@tadeuzagallo

View changes

Show outdated Hide outdated Libraries/Video/VideoContentModes.m
@implementation VideoContentModes
- (NSDictionary *)constantsToExport

This comment has been minimized.

@tadeuzagallo

tadeuzagallo Mar 30, 2015

Contributor

No need for this extra module, you can move it into RCTVideoManager.m

@tadeuzagallo

tadeuzagallo Mar 30, 2015

Contributor

No need for this extra module, you can move it into RCTVideoManager.m

@joshhornby

This comment has been minimized.

Show comment
Hide comment
@joshhornby

joshhornby Mar 30, 2015

Really like the look of this 👍

joshhornby commented Mar 30, 2015

Really like the look of this 👍

@grp

This comment has been minimized.

Show comment
Hide comment
@grp

grp Mar 30, 2015

Contributor

Hey, this looks great but in the past I've noticed a few issues with MPMoviePlayerController — it often blocks the main thread (causing stutters) whenever you create one, and you can only have one of them on the screen at a time. Using AVPlayer directly can avoid that, although it's more complex to use.

Internally, we have a wrapper around AVPlayer with a nice API. I'll see about open sourcing that: it might make a good core for a <Video> component.

Contributor

grp commented Mar 30, 2015

Hey, this looks great but in the past I've noticed a few issues with MPMoviePlayerController — it often blocks the main thread (causing stutters) whenever you create one, and you can only have one of them on the screen at a time. Using AVPlayer directly can avoid that, although it's more complex to use.

Internally, we have a wrapper around AVPlayer with a nice API. I'll see about open sourcing that: it might make a good core for a <Video> component.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Mar 30, 2015

Collaborator

@grp - that would be great! Looking forward to seeing it

Collaborator

brentvatne commented Mar 30, 2015

@grp - that would be great! Looking forward to seeing it

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 30, 2015

Contributor

Actually, I think this would be better for you to create a separate github repo for this specific component, iterate on it and when it reaches maturity we can see about including it in the main repo. Otherwise we're going to slow you down as we're not going to include something half baked as part of the core build.

Contributor

vjeux commented Mar 30, 2015

Actually, I think this would be better for you to create a separate github repo for this specific component, iterate on it and when it reaches maturity we can see about including it in the main repo. Otherwise we're going to slow you down as we're not going to include something half baked as part of the core build.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Mar 30, 2015

Collaborator

@vjeux - that makes total sense, agreed

Collaborator

brentvatne commented Mar 30, 2015

@vjeux - that makes total sense, agreed

@brentvatne brentvatne closed this Mar 30, 2015

@brentvatne

This comment has been minimized.

Show comment
Hide comment

facebook-github-bot added a commit that referenced this pull request Mar 9, 2017

Take margin into account on max dimension
Summary:
We need to take the margin into account if we clip on max dimension. Fixes #466.
Closes facebook/yoga#467

Differential Revision: D4681342

Pulled By: emilsjolander

fbshipit-source-id: 56311df9864a284d553c31f1c6db382f337f1fad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment