New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 707224 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Presentation API: don't fire availability events when page is foregrounded unless needed

Project Member Reported by mlamouri@chromium.org, Mar 31 2017

Issue description

When a page uses Presentation API with an availability remote device, it will receive an availability change (to false) then another one (to true). We should not fire an event if a page is foregrounded and there was no change between the previous known value and the current one.
 
Cc: mfo...@chromium.org imch...@chromium.org
I don't think we can solve it for every case and we'd have to use some timeouts there. What happens when we have a single page is the following:

1. the page requests the availability object
2. the renderer starts observing media sinks
3. the backend immediately reports false since the discovery always assumes false until it eventually finds something
4. the backend reports true if/when it finds a device
5. the renderer receives true and sends an event to the page
6. the page is backgrounded
7. the renderer stops observing media sinks to save resources
8. the backend stops discovery to save resources
9. the page is foregrounded
Steps 2-5 repeat, hence the false -> true events.

I imagine to fix this we'd have to give step 3 a timeout:

3. the backend starts discovery and sets a timeout of say 5s
4. either the backend finds something within the timeout and reports true or it doesn't and it reports false
5. the backend reports true if it finds something after the timeout expired

In the cases where successful discovery takes more than the timeout (IIRC it may take up to 30s sometimes?), the page will still get false and true events. In the cases where the discovery is fast, I don't think the sites suffer much.

Moreover, if there're two pages and only one is backgrounded, the issue shouldn't reproduce as the discovery won't be stopped and return the most recent known availability vs the default false.

FWIW, the spec doesn't prevent having a timeout unless I misread it. It would be a part of step 5 here: http://w3c.github.io/presentation-api/#monitoring-the-list-of-available-presentation-displays - which is implementation-specific.
That sounds reasonable.

- Does that mean the initial getAvailability() could take up to 5s in the negative case as well?
- If we are changing Media Router itself, we should make sure UI components that depend on it are not affected. 
Yes, we will wait for 5s to detect the negative case too. Most pages would not show the UI until the positive response anyway so it shouldn't have much impact I think.

We could change the MRPs where needed (e.g. if Hangouts "discovery" is symmetrical in terms of positive/negative result, we don't need to impose the timeout on them).

Comment 4 by mfo...@chromium.org, Jan 10 2018

Cc: mlamouri@chromium.org
 Issue 799032  has been merged into this issue.

Comment 5 by mfo...@chromium.org, Jan 10 2018

Cc: -mfo...@chromium.org
Owner: mfo...@chromium.org
Status: Assigned (was: Available)
Labels: -Pri-3 M-69 Pri-2
This is a lingering correctness/ergonomics issue with the API that would be good to address in Q2.

Status: Started (was: Assigned)
Labels: -M-69 Target-71
Labels: -Target-71 Target-72
Status: Assigned (was: Started)
Cc: -imch...@chromium.org
Status: Started (was: Assigned)
Cc: taku...@chromium.org
+takumif@

I instrumented the code and verified that the analysis from Comment #1 by avayvod@ is essentially correct.

When a PresentationAvailability goes in the background, it signals the Media Router to unregister its observer for the corresponding MediaSource.  If that was the only observer, it removes the sink query and expunges the cache for that source.

When the PresentationAvailability goes back in the foreground, if it's the first to register an observer for the MediaSource again, the observer is fired with a cached empty list here:

https://cs.chromium.org/chromium/src/chrome/browser/media/router/mojo/media_router_mojo_impl.cc?rcl=0803a8bb25489af4db4a54542663f9fad87f2697&l=581

The quick fix is to not fire the observer until at least one MRP returns a sink for a new sink query (or all return none).  This is correct and will eliminate most spurious state changes.

However, this may add an arbitrary delay until a getAvailability() promise is resolved.  There might be user experience issues as well depending on whether various bits of UX trigger actions based on the sink observer firing.

If necessary, we can add a second code path to determine screen availability within a timeout.  This is more involved and unclear if this is a Blink change, an MR change or both.

I didn't analyze the Android media router code path.  A similar fix may be needed there.

takumif@, who has refactored a lot of the desktop MRP code, might have opinions here...
Labels: -OS-All OS-Chrome OS-Linux OS-Mac OS-Windows
> The quick fix is to not fire the observer until at least one MRP returns a sink for a new sink query (or all return none).

This sounds right to me. I don't think we currently keep track of which MRPs have already responded to a sink query, so we'd need to add that to MediaSinksQuery. We could also combine this with a timeout (within MediaSinksQuery), in case the MRPs don't respond in a timely manner.
Adding a global timeout to MediaSinkQuery would be most backwards compatible.

I'm just not sure which observers need a timeout and whether it would be the same across observers.   I'm leaning towards implementing any timeouts outside of the MR.
I have a WIP patch for this that fixes the positive case: when sinks are available, the availability object will no longer fire a spurious false event when the page is foregrounded.   Need to verify that other cases are handled correctly w.r.t. changes in page visibility and device availability.

It's tricky because the same code path is used to resolve the getAvailability() promise (which needs a definite true/false answer) and trigger the change event (which just needs to signal transitions).

For a new URL (where we don't have any cached sinks to fall back on), it might be worth resolving the promise early with 'availability = false', then waiting for a future 'true' transition if a MRP finds a compatible sink, versus introducing some arbitrary timeout to resolve the promise with 'availability = false'.  That might be something to work on in a separate issue.




Labels: -Target-72 Target-73

Sign in to add a comment