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 promise call in setDescription to make compatible with safari #32

Merged
merged 2 commits into from Jun 9, 2020

Conversation

oscar-mnfuentes
Copy link
Contributor

@oscar-mnfuentes oscar-mnfuentes commented May 15, 2020

Fix a little bug that makes safari never receive the stream, we change the promise call to able compatible with safari.

Tested on Safari 13.1 and chrome 81.
MacOs Catalina (10.15.4)

Types of changes

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature / enhancement (non-breaking change which improves the project)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • My change requires a change to the documentation
  • My change requires a change in other repository

Checklist

  • [ x] I have read the Contribution Guidelines
  • [ x] I have added an explanation of what the changes do and why they should be included
  • I have written new tests for the changes, as applicable, and have successfully run them locally

@jenkinskurento
Copy link
Contributor

@jenkinskurento jenkinskurento commented May 15, 2020

Hi there, thanks for your Pull Request!

A Kurento member needs to verify that this patch is reasonable to test. In case it is, they should write a comment with the phrase test this please. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by Kurento members will still work. Regular contributors can be whitelisted to skip this step.

@PorkoBravo
Copy link

@PorkoBravo PorkoBravo commented May 27, 2020

Hi, Any news about this?
Thanks!

@oscar-mnfuentes
Copy link
Contributor Author

@oscar-mnfuentes oscar-mnfuentes commented May 27, 2020

Seems to be related with this PR #30

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 5, 2020

We're experimenting Safari doesn't receive video stream starting on Safari 13.1 (working 13.0.5) and iOS 13.4+ Safari. Maybe this is related, but changing our kurento-utils by @oscar-mnfuentes repository doesn't work either.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 5, 2020

I have it fixed and APIs updated in https://github.com/piranna/kurento-utils-js, hope to do a pull-request soon. Note, it's not fully API compatible.

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 5, 2020

Interestingly, @piranna your repo cannot be pointed to if using yarn, but works correctly by using npm, due to an error on KurentoForks qunit repo.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 5, 2020

I never use yarn, so I don't know :-)

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 5, 2020

Well, let's wait for the PR.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 5, 2020

I would need to find time to fix automated tests and tests coverage first, by using Jest. If anybody can help me here, it's welcome :-)

@micaelgallego
Copy link
Member

@micaelgallego micaelgallego commented Jun 5, 2020

If the PR breaks the API, it is unlikely to be accepted.

Take this into account and try to maintain the API blackguard compatible.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 6, 2020

Take this into account and try to maintain the API blackguard compatible.

Thanks for the advice :-) I will provide a list of the API changes with the pull-request, so we can discuss and fix them. Main differences are regarding usage of Promises instead of callbacks, since WebRTC callbacks-based APIs are deprecated. Backwards compatibility could be easily maintained here, but TheGoodWay(tm) would be to move to a Promises-only API.

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 8, 2020

@piranna Since we need these changes ASAP (at least to make sure our project will run in Safari 13.1+ and iOS 13.4+, as those are the first broken versions), what do you think about pointing our kurento-utils URL to your repo and testing those changes?

@piranna
Copy link
Contributor

@piranna piranna commented Jun 8, 2020

@piranna Since we need these changes ASAP (at least to make sure our project will run in Safari 13.1+ and iOS 13.4+, as those are the first broken versions), what do you think about pointing our kurento-utils URL to your repo and testing those changes?

That would be awesome :-D If fact, i put the link to my repo since it could be useful to others. Tests are not passing since they need to be upgraded, but I'm using my repo as dependency in a Vue project without problems :-) If you see something not working as expected, please notify me it :-)

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 8, 2020

@piranna We tested your repo. Needed to clone it and do some changes due to KurentoForks problem (your repo points to version v1.0.3 but their package.json in that tag says 1.0.2pre so yarn doesn't allow installation).

After cloning both repos, changing qunit-reporter-junit package.json's to 1.0.3 for tag matching, your repo allowed installation. Pointed your repo to my local qunit-reporter-junit and our proyect to my local clone of your repo.

Result is that Safari is still broken. Starting from iOS 13.4, we don't receive any video (works on 13.3 through 13.0), and macOS Safari 13.1 is also broken in the same way (Safari 13.0.5 works flawlessly).

Hope this helps.

Edit: Of course, Apple does not provide any info on WebRTC changes on iOS 13.4 and Safari 13.1 changelogs. Trying some combinations of experimental webrtc related features does not fix anything.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 8, 2020

@piranna We tested your repo. Needed to clone it and do some changes due to KurentoForks problem (your repo points to version v1.0.3 but their package.json in that tag says 1.0.2pre so yarn doesn't allow installation).

Thanks for your feedback, I'm not using yarn so I've by-passed this. Can you be able to do a pull-request against my repo so I can add your changes? :-)

Result is that Safari is still broken. Starting from iOS 13.4, we don't receive any video (works on 13.3 through 13.0), and macOS Safari 13.1 is also broken in the same way (Safari 13.0.5 works flawlessly).

I don't own an iOS device nor Safari browser, but thank you so much for pointing this, maybe my client can be able to check that instead (he's using iOS and Mac).

Edit: Of course, Apple does not provide any info on WebRTC changes on iOS 13.4 and Safari 13.1 changelogs. Trying some combinations of experimental webrtc related features does not fix anything.

Of course... :-/

@pedroadame
Copy link

@pedroadame pedroadame commented Jun 8, 2020

@piranna In this case it's a fault in the KurentoForks/qunit-reporter-junit repository, and it's related to versioning inside package.json, so I would leave it to a contributor of that repo who knows the exact versioning they're using.

@piranna
Copy link
Contributor

@piranna piranna commented Jun 8, 2020

@piranna In this case it's a fault in the KurentoForks/qunit-reporter-junit repository, and it's related to versioning inside package.json, so I would leave it to a contributor of that repo who knows the exact versioning they're using.

Looks who is the one of the contributors... :-) In any case, I plan to change tests here from browser-side QUnit to Jest, so it will not be needed anymore. Just only, i need time to rewrite the automated tests, or help from somebody else to do it by me.

@j1elo
Copy link
Member

@j1elo j1elo commented Jun 9, 2020

@piranna In this case it's a fault in the KurentoForks/qunit-reporter-junit repository

I plan to change tests here from browser-side QUnit to Jest, so it will not be needed anymore

It would be awesome if we can get rid of KurentoForks for kurento-utils-js, right now we're keeping this dependency because "it works", but there is nobody watching over it or maintaining it... so moving to well maintained external dependencies from NPM would be a step in the right direction!

lib/WebRtcPeer.js Outdated Show resolved Hide resolved
lib/WebRtcPeer.js Outdated Show resolved Hide resolved
@j1elo j1elo marked this pull request as draft Jun 9, 2020
@j1elo j1elo self-assigned this Jun 9, 2020
@j1elo j1elo marked this pull request as ready for review Jun 9, 2020
@j1elo j1elo merged commit 5b80a37 into Kurento:master Jun 9, 2020
@j1elo j1elo mentioned this pull request Jun 9, 2020
@pedroadame
Copy link

@pedroadame pedroadame commented Jun 9, 2020

@j1elo It would be awesome if you notify somewhere when a new version with these changes is released.

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