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

Fix tvOS picture-in-picture compilation regression. #1518

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

FullstackJack
Copy link
Contributor

@FullstackJack FullstackJack commented Mar 12, 2019

A recent PR that introduced Picture-in-Picture via iOS APIs failed to test for tvOS regressions.
#1325

There has been discussion after the code was merged and pulled by the community that discusses wrapping the offending APIs in directives. This PR does exactly that and has been tested with a tvOS 12.1 simulator.

Here is a related issue that I created because I wasn't sure if that was protocol for this repo (maybe it will help others that come across this issue before and after it is merged).
#1517

Tested this fix with the following code snippet and plays videos on iOS and tvOS.

export default class VideoPlayer extends React.PureComponent {
    render() {
        return (
            <View style={styles.container}>
                <Video
                    style={styles.video}
                    ref={this._getRef}
                    pictureInPicture={true} 
                    source={{
                        uri: 'https://bitdash-a.akamaihd.net/content/MI201109210084_1/m3u8s/f08e80da-bf1d-4e3d-8899-f0f6155f6efa.m3u8'
                    }}
                />
            </View>
        );
    }
}

@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate, DVAssetLoaderDelegatesDelegate>
@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate, DVAssetLoaderDelegatesDelegate, AVAssetResourceLoaderDelegate>
#elif TARGET_OS_TV
@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the DVAssetLoaderDelegatesDelegate also incompatible with tvOS?

Copy link
Contributor Author

@FullstackJack FullstackJack Mar 12, 2019

Choose a reason for hiding this comment

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

Yes, it appears so since it compiles. Also made me realize I shouldn't have copied in AVAssetResourceLoaderDelegate when I started working so I removed it. That delegate does not exist in master.

@FullstackJack
Copy link
Contributor Author

FullstackJack commented Mar 12, 2019

@cobarx Could you please review this change? It's blocking teams from building tvOS with latest version of react-native-video. It follows recommended method for fencing unsupported APIs as noted in Building for Apple TV. I've tested it with our test project and it works. https://facebook.github.io/react-native/docs/building-for-apple-tv#code-changes

@cobarx
Copy link
Contributor

cobarx commented Mar 13, 2019

@danielmarino24i You've been quite active and engaged with the project, so I'd like to see if you'd be interested in being a maintainer. I couldn't locate your email on GitHub, so I'm reaching out here. Can you email me at me@hamptonmaxwell.com so we can discuss.

@cobarx cobarx merged commit 9016d19 into TheWidlarzGroup:master Mar 13, 2019
@cobarx
Copy link
Contributor

cobarx commented Mar 13, 2019

@FullstackJack Thanks for figuring this out and creating the PR. Creating an issue is a great idea for the reasons you mentioned, having people be able to search for the problem is quite helpful.

@gtebbutt
Copy link

Just hit this issue this morning - great to see it already fixed! @cobarx is there a plan for an imminent npm release, or better to just grab master for now?

@talesporto
Copy link

Are you have a date to put the next release with it fixed?

@jdmueller
Copy link

I am running into this same build failure problem. I am following the directions exactly using the latest npm install react-native-video. Trying to run on 12.2 simulator but it won't even build throwing out this error. I have tried installing the latest ^3.0.0 but that version also has some errors, and it will not function correctly. It builds with ^3.0.0 but then gets this error in the console: Accessing view manager configs directly off UIManager via UIManager['RCTVideo'] is no longer supported. Use UIManager.getViewManagerConfig('RCTVideo') instead. I found a commit recommended but after applying that it throws out yet another error. I would really love to be able to use this to finish my tvOS app!

@FullstackJack
Copy link
Contributor Author

FullstackJack commented Apr 3, 2019

@jdmueller How are you installing the latest? The fix has not been released as a version. In you npm package.json, you need to reference the master branch.

"dependencies": {
  "package": "git://github.com/username/package.git#commit"
}

In our case, you can get the merged code at...

"dependencies": {
  "react-native-video": "git://github.com/react-native-community/react-native-video.git#933bbae77b1ee55cafc060840b85cc0074f46ca7"
}

https://stackoverflow.com/questions/14187956/npm-install-from-git-in-a-specific-version

@cobarx
Copy link
Contributor

cobarx commented Apr 4, 2019

Ok I've pushed out 4.4.1 with this fix in it.

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.

6 participants