Suspend media after navigation #24223

ferjm · 2019-09-16T19:08:49Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #21989 and fix #22763
I am not sure how to test this

This depends on servo/media#310

This change is

highfive · 2019-09-16T19:08:54Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/document.rs
@KiChjang: components/script/dom/document.rs

highfive · 2019-09-16T19:08:55Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

ferjm · 2019-09-16T19:09:06Z

r? @jdm

jdm · 2019-09-16T19:37:13Z

Have you checked if this works with going forwards and backwards in session history as well?

ferjm · 2019-09-17T14:26:11Z

Unfortunately, it only works with navigations :\ I'm trying to fix session history browsing as well.

ferjm · 2019-09-18T09:25:54Z

r? @jdm

It now works with session history browsing as well. It suspends/resume the media action based on the document activity, so browsing to a different context while playing some audio or video pauses playback and it resumes it where you left it if you browse back to that context.

jdm · 2019-09-18T15:16:29Z

This looks nice and clean!

bors-servo · 2019-09-19T03:58:50Z

☔ The latest upstream changes (presumably #24236) made this pull request unmergeable. Please resolve the merge conflicts.

gterzian

Have you considered that this might suffer from a similar issue as noted at #24211, in the sense that, in multiprocess mode, the operations performed in the constellation might not affect the media instance used in script since ServoMedia::get() returns an in-process instance.

The proposed changes could make the current workflow more robust to an eventual refactoring required to make media fully support multiprocess.

gterzian · 2019-09-19T05:25:04Z

components/constellation/constellation.rs

            let child_activity = if activity == DocumentActivity::Inactive {
+                media.suspend(&client_context_id);


The problem with doing those operations in the constellation is that the media instance will differ from the one in script, if Servo is running in multiprocess mode.

Have you considered doing the suspend and resume operations in script when the document activity changes?

You could move those operations to

servo/components/script/dom/document.rs

Line 496 in 7b653ca

pub fn set_activity(&self, activity: DocumentActivity) {

Good point! I'm applying your suggestion. Thanks

ceyusa · 2019-09-19T10:15:23Z

Cargo.toml

+servo-media-gstreamer = { git = "https://github.com/ferjm/media", branch = "suspend" }
+servo-media-audio = { git = "https://github.com/ferjm/media", branch = "suspend" }
+servo-media-player = { git = "https://github.com/ferjm/media", branch = "suspend" }
+servo-media-streams = { git = "https://github.com/ferjm/media", branch = "suspend" }


this is temporal, right?

suspend branch is behind current media master

Yes. I will remove the patch as soon as servo/media#310 is merged


        Register pipeline instead of browsing contexts for media instances


        Suspend/resume media activity based on document activity

ferjm · 2019-09-19T16:45:20Z

@bors-servo r=jdm,gterzian

bors-servo · 2019-09-19T16:45:21Z

📌 Commit d7bebce has been approved by jdm,gterzian

bors-servo · 2019-09-19T17:08:36Z

⌛ Testing commit d7bebce with merge 9c74d14...


        Auto merge of #24223 - ferjm:suspend.media.playback, r=jdm,gterzian

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24223)

bors-servo · 2019-09-19T18:57:22Z

💔 Test failed - status-taskcluster

jdm · 2019-09-19T19:16:42Z

@bors-servo retry

network issue?

bors-servo · 2019-09-19T19:34:03Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-09-19T19:34:08Z

⌛ Testing commit d7bebce with merge 9047ae9...


        Auto merge of #24223 - ferjm:suspend.media.playback, r=jdm,gterzian

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24223)

bors-servo · 2019-09-19T20:10:22Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm,gterzian
Pushing 9047ae9 to master...

highfive assigned SimonSapin Sep 16, 2019

highfive added the S-awaiting-review label Sep 16, 2019

highfive assigned jdm and unassigned SimonSapin Sep 16, 2019

jdm added the S-awaiting-answer label Sep 16, 2019

ferjm removed S-awaiting-answer S-awaiting-review labels Sep 17, 2019

ferjm force-pushed the ferjm:suspend.media.playback branch from c070dc8 to ad798bd Sep 18, 2019

highfive added the S-awaiting-review label Sep 18, 2019

jdm approved these changes Sep 18, 2019

View changes

jdm added S-blocked-on-external and removed S-awaiting-review labels Sep 18, 2019

highfive added the S-needs-rebase label Sep 19, 2019

gterzian suggested changes Sep 19, 2019

View changes

ferjm force-pushed the ferjm:suspend.media.playback branch from ad798bd to d7daaf5 Sep 19, 2019

highfive added the S-awaiting-review label Sep 19, 2019

gterzian approved these changes Sep 19, 2019

View changes

ceyusa reviewed Sep 19, 2019

View changes

Register pipeline instead of browsing contexts for media instances

51ed44c

ferjm force-pushed the ferjm:suspend.media.playback branch from d7daaf5 to d5c52c7 Sep 19, 2019

Suspend/resume media activity based on document activity

Loading status checks…

d7bebce

ferjm force-pushed the ferjm:suspend.media.playback branch from d5c52c7 to d7bebce Sep 19, 2019

ferjm removed S-awaiting-review S-blocked-on-external S-needs-rebase labels Sep 19, 2019

highfive added the S-awaiting-merge label Sep 19, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 19, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 19, 2019

bors-servo merged commit d7bebce into servo:master Sep 19, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 19, 2019

ferjm deleted the ferjm:suspend.media.playback branch Sep 20, 2019

servo / servo

Suspend media after navigation #24223

Suspend media after navigation #24223

ferjm commented Sep 16, 2019 •

edited by atouchet

highfive commented Sep 16, 2019

highfive commented Sep 16, 2019

ferjm commented Sep 16, 2019

jdm commented Sep 16, 2019

ferjm commented Sep 17, 2019

ferjm commented Sep 18, 2019

jdm commented Sep 18, 2019

bors-servo commented Sep 19, 2019

gterzian left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

		let child_activity = if activity == DocumentActivity::Inactive {
		media.suspend(&client_context_id);

servo / servo

Join GitHub today

Suspend media after navigation #24223

Suspend media after navigation #24223

Conversation

ferjm commented Sep 16, 2019 • edited by atouchet

highfive commented Sep 16, 2019

highfive commented Sep 16, 2019

ferjm commented Sep 16, 2019

jdm commented Sep 16, 2019

ferjm commented Sep 17, 2019

ferjm commented Sep 18, 2019

jdm commented Sep 18, 2019

bors-servo commented Sep 19, 2019

gterzian left a comment • edited

This comment has been minimized.

gterzian Sep 19, 2019 • edited

This comment has been minimized.

ferjm Sep 19, 2019

This comment has been minimized.

ceyusa Sep 19, 2019

This comment has been minimized.

ferjm Sep 19, 2019

ferjm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

bors-servo commented Sep 19, 2019

ferjm commented Sep 16, 2019 •

edited by atouchet

gterzian left a comment •

edited

gterzian Sep 19, 2019 •

edited