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

Allow setting video.srcObject (should fix issues such as #262) #270

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

ibc
Copy link
Collaborator

@ibc ibc commented Apr 3, 2017

It may work (or not, since I don't have an iOS device to check it).

Copy link
Collaborator

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I need to test it before merging.

@ggozad
Copy link

ggozad commented Apr 6, 2017

I am also hit by #262 . iOS client streams fine and the video is seen on the other side, but not displayed on the iOS client. Instead I see a native play video button.
Applying this did not resolve #262, I did see though the play button using .srcObject which I would not before.

@saghul
Copy link
Collaborator

saghul commented Apr 6, 2017

@ggozad That has nothing to do with this AFAICT. See the FAQ https://github.com/BasqueVoIPMafia/cordova-plugin-iosrtc/blob/master/FAQ.md for how to remove the play button.

@ggozad
Copy link

ggozad commented Apr 6, 2017

@saghul You misunderstand. The issue is not the play button, the issue is the video does not render.

@ggozad
Copy link

ggozad commented Apr 6, 2017

@saghul perhaps you can reopen #262 as a few people have this problem. I am posting here since @ibc thought that this would actually fix #262

@saghul
Copy link
Collaborator

saghul commented Apr 6, 2017

@ggozad I cannot reproduce the problem. Unless there is a way for me to reproduce it there is nothing I can do to fix it. 🤷‍♂️

@ggozad
Copy link

ggozad commented Apr 6, 2017

@saghul not being able to reproduce a bug does not mean it does not exist. You are far more familiar with the codebase than I am, could you suggest what kind of test you would like to see? Would it be helpful to make an example app with the bug?

@saghul
Copy link
Collaborator

saghul commented Apr 6, 2017

not being able to reproduce a bug does not mean it does not exist.

I don't mean to sound smug, but video rendering is one of the primary goals of this plugin. It's the main thing I test before making a release, and we just made one, so I tested my existing app (since we had made significant changes to the plugin, for Swift 3 support) and it continues to work as expected.

Would it be helpful to make an example app with the bug?

Definitely.

@dunnky
Copy link
Contributor

dunnky commented Apr 13, 2017

I was also seeing something like #262 - PR worked great in my tests 👍

@ibc
Copy link
Collaborator Author

ibc commented Apr 13, 2017

@uunp do you mean that this PR works fine? To be clear: this PR enables videoElement.srcObject = mediaStream. Before this PR, the only way to make it work was by doing the legacy videoElement.src = URL.createObjectURL(mediaStream).

@dunnky
Copy link
Contributor

dunnky commented Apr 13, 2017

@ibc Correct, this PR works fine in my tests when using videoElement.srcObject = mediaStream. Appreciate the work!

@ibc
Copy link
Collaborator Author

ibc commented Apr 13, 2017

Nice to know. I will merge it so users can use the master branch (while waiting after holidays for @saghul to release a new NPM version).

@ibc ibc merged commit 4f485f8 into master Apr 13, 2017
@dunnky
Copy link
Contributor

dunnky commented Apr 13, 2017

@ibc Sorry, forgot about this - I did have to run gulp browserify to get your changes added to dist/cordova-plugin-iosrtc.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants