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

global mute for servo-media backend #271

Merged
merged 1 commit into from Jul 3, 2019

Conversation

Projects
None yet
4 participants
@khodzha
Copy link
Contributor

commented Jun 21, 2019

Issue description is here:
servo/servo#22359

@khodzha khodzha force-pushed the khodzha:master branch 8 times, most recently from 4d20d56 to 212e1c1 Jun 21, 2019

@ceyusa

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Thanks @khodzha!

It is a big patch.

Would you mind to explain or describe a bit your proposed solution? :)

@ferjm
Copy link
Member

left a comment

Thanks @khodzha. This is great work!

It looks good overall, but I added a few comments of things that I think that we can improve.

Show resolved Hide resolved examples/muted_player.rs Outdated
Show resolved Hide resolved audio/context.rs Outdated
Show resolved Hide resolved audio/muteable.rs Outdated
Show resolved Hide resolved audio/render_thread.rs Outdated
Show resolved Hide resolved audio/render_thread.rs Outdated
Show resolved Hide resolved audio/muteable.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs
Show resolved Hide resolved player/Cargo.toml Outdated
Show resolved Hide resolved servo-media/lib.rs Outdated
Show resolved Hide resolved examples/android/lib/src/lib.rs Outdated
Show resolved Hide resolved examples/audio_decoder.rs Outdated
Show resolved Hide resolved examples/biquad.rs Outdated
Show resolved Hide resolved examples/channels.rs Outdated

@khodzha khodzha changed the title global mute for servo-media backend WIP: global mute for servo-media backend Jun 25, 2019

@khodzha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@ceyusa its basically an implementation of changes proposed here with some caveats (related to servo-media updates) figured out

@khodzha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Shouldnt mute() receive ClientContextId argument? 🤔

Because if my understanding is correct and all Player/AudioContext instances on a single tab have same ClientContextId (BrowsingContextID) of this tab and we want to mute this tab and not all tabs, we need to mute only muteables with given ClientContextId

@ferjm

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Of course, it should! Sorry, I missed that on my review.

@khodzha khodzha force-pushed the khodzha:master branch 2 times, most recently from 0eea882 to a743b81 Jun 25, 2019

@ferjm
Copy link
Member

left a comment

Thanks @khodzha. Looking great.

I added a few more comments. Most of them are just nits, but the clean up part is important.

Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/player.rs Outdated
Show resolved Hide resolved examples/muted_audiocontext.rs
Show resolved Hide resolved player/lib.rs Outdated
Show resolved Hide resolved traits/Cargo.toml Outdated
Show resolved Hide resolved traits/lib.rs
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved traits/lib.rs Outdated
Show resolved Hide resolved traits/muteable.rs Outdated

@khodzha khodzha force-pushed the khodzha:master branch 3 times, most recently from d5bb889 to e6c519b Jul 3, 2019

@ferjm
Copy link
Member

left a comment

Almost there! Only a few minor changes left. Thanks!

Show resolved Hide resolved backends/gstreamer/lib.rs
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs Outdated
Show resolved Hide resolved backends/gstreamer/lib.rs

@khodzha khodzha force-pushed the khodzha:master branch from e6c519b to dd56e66 Jul 3, 2019

@khodzha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

not sure if implementing remove_mutable straight on GStreamerBackend is ok 🤔
(i guess it is but ¯_(ツ)_/¯)

@khodzha khodzha force-pushed the khodzha:master branch from dd56e66 to 4bc2b0d Jul 3, 2019

@khodzha khodzha changed the title WIP: global mute for servo-media backend global mute for servo-media backend Jul 3, 2019

@ferjm

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

not sure if implementing remove_mutable straight on GStreamerBackend is ok 🤔
(i guess it is but ¯_(ツ)_/¯)

Why wouldn't it be ok? :)

@ferjm

ferjm approved these changes Jul 3, 2019

Copy link
Member

left a comment

Excellent work. Thank you!

@ferjm

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

📌 Commit 4bc2b0d has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

⌛️ Testing commit 4bc2b0d with merge 4375840...

bors-servo added a commit that referenced this pull request Jul 3, 2019

Auto merge of #271 - khodzha:master, r=ferjm
global mute for servo-media backend

Issue description is here:
servo/servo#22359
@khodzha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

thank you for bearing with me ❤️

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

☀️ Test successful - checks-travis
Approved by: ferjm
Pushing 4375840 to master...

@bors-servo bors-servo merged commit 4bc2b0d into servo:master Jul 3, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@jdm jdm referenced this pull request Jul 5, 2019

Merged

Support WebAudio on Windows #23712

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.