Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upExpose methods to suspend and resume all media actions #310
Conversation
|
r? @jdm |
| fn suspend(&self, id: &ClientContextId) { | ||
| let mut instances = self.instances.lock().unwrap(); | ||
| match instances.get_mut(id) { | ||
| Some(vec) => vec.retain(|(_, weak)| { |
This comment has been minimized.
This comment has been minimized.
jdm
Sep 16, 2019
Member
Should we abstract this operation (iterating over all known instances to perform an operation while dropping any dead ones) into a helper function that takes a closure argument, since it's used in two different places now?
This comment has been minimized.
This comment has been minimized.
ferjm
changed the title
ferjm
changed the title
|
r? @jdm I have added an additional |
| fn media_instance_action( | ||
| &self, | ||
| id: &ClientContextId, | ||
| cb: Rc<dyn Fn(Arc<Mutex<dyn MediaInstance>>) -> Result<(), ()>>, |
This comment has been minimized.
This comment has been minimized.
jdm
Sep 18, 2019
Member
It's not clear why the Rc is needed here - what if we use &dyn Fn(...) -> ... instead, and don't add move to each closure? Also, why not call lock().unwrap() in this method and pass &mut dyn MediaInstance to the callback instead?
|
@bors-servo r+ |
|
|
Expose methods to suspend and resume all media actions This is required for fixing servo/servo#22763 and servo/servo#21989 ServoMedia shuts down AudioContexts and Players when these are dropped, but because Servo keeps a bfcache, DOM objects are not always released immediately after navigation. So we need a way to manually suspend playback. A similar issue occurs with `getUserMedia`, where we keep input streams active after navigation. I'll fix it in a follow-up.
|
|
Suspend media after navigation - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #21989 and fix #22763 - [ ] I am not sure how to test this This depends on servo/media#310 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24223) <!-- Reviewable:end -->
Suspend media after navigation - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #21989 and fix #22763 - [ ] I am not sure how to test this This depends on servo/media#310 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24223) <!-- Reviewable:end -->
ferjm commentedSep 16, 2019
•
edited by atouchet
This is required for fixing servo/servo#22763 and servo/servo#21989
ServoMedia shuts down AudioContexts and Players when these are dropped, but because Servo keeps a bfcache, DOM objects are not always released immediately after navigation. So we need a way to manually suspend playback.
A similar issue occurs with
getUserMedia, where we keep input streams active after navigation. I'll fix it in a follow-up.