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

Add support for multiple audio tracks #1712

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

hicom150
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Enabling Chromium experimentalFeatures I made an attempt to add support for changing between multiple audio tracks.

Is there anything you'd like reviewers to focus on?
As it need to enable experimentalFeatures, check that all other features work as expected 😅

@feross
Copy link
Member

feross commented Oct 1, 2019

Wow, this is a great! I haven't had a chance to look at the code or test it yet (hoping others can help review it in the meantime) but this is exciting!

@Borewit
Copy link
Member

Borewit commented Oct 6, 2019

@hicom150 now you are a member of the webtorrent organization, you may want to create branches for PR's directly on the webtorrent repository. That makes "jumping" and operations between branches in my opinion a bit easier.

@Borewit Borewit self-requested a review October 6, 2019 19:51
Copy link
Member

@Borewit Borewit left a comment

Choose a reason for hiding this comment

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

Check the lower right corner:
image
I love it @hicom150, great work once again!
Because this PR contains the same changes as #1711, I will do an attempt get rid of those...

Comment on lines 57 to 64
.then(() => {
dispatch('mediaSuccess')
})
.catch((err) => {
if (err.name === 'NotSupportedError') {
dispatch('mediaError', 'Codec unsupported')
}
})
Copy link
Member

Choose a reason for hiding this comment

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

This belongs to PR #1711.

Copy link
Member

Choose a reason for hiding this comment

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

I removed these changes using a force push on your branch

src/renderer/pages/player-page.js Outdated Show resolved Hide resolved
@Borewit
Copy link
Member

Borewit commented Oct 7, 2019

@hicom150 I stripped of the duplicate changes from #1711, with an amend commit and a force push to your branch. This will cause your local branch to be in conflict with your cloned repo branch. Rename your local branch to change_audio_tracks_original and git checkout origin change_audio_tracks is probably the easiest.

Please do an amend commit (maybe twice, one time with a dummy change, the second time you undo that change). Once you have done that, you can force push the branch which will put back your changes from @Borewit to @hicom150,.

@hicom150
Copy link
Contributor Author

hicom150 commented Oct 7, 2019

I think that this PR needs #1711 to be merged to work correctly in all edge cases, so I will wait just a bit 😅

@Borewit
Copy link
Member

Borewit commented Oct 7, 2019

I will wait just a bit

Makes sense, let's get #1711 in

@hicom150
Copy link
Contributor Author

Given that #1711 is already merged, I'm going to merge also this one 😉
Thank you for your help @Borewit 💪

@hicom150 hicom150 merged commit 45920e0 into webtorrent:master Oct 13, 2019
@hicom150 hicom150 deleted the change_audio_tracks branch October 13, 2019 20:55
@Borewit
Copy link
Member

Borewit commented Oct 14, 2019

Very nice @hicom150, I am glad these improvements are in.

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.

None yet

3 participants