New issue
Advanced search Search tips

Issue 752310 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

[Media Router] Improve cast discovery reliability during flaky network conditions

Project Member Reported by zhaobin@chromium.org, Aug 3 2017

Issue description

Two things to think about to improve reliability during flaky network conditions:

1) Porting over the reconnect-on-error logic from the extension will help keep the sink around instead of waiting for next signal from mDNS.
2) Dynamically adjusting the connect timeout / ping timeout values for a reconnect could help in some cases. Since this is speculative, it might be best to implement this under an experiment and observe whether metrics show a positive impact (e.g. channel open success rate, ping timeout rate, etc.)

To resolve code review comments in: https://chromium-review.googlesource.com/c/575247
 
Components: -Blink>PresentationAPI Internals>Cast

Comment 2 by sko...@chromium.org, Aug 14 2017

Status: Assigned (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9439571e71419c717dffd16bc6b9722b1e27762d

commit 9439571e71419c717dffd16bc6b9722b1e27762d
Author: Bin Zhao <zhaobin@chromium.org>
Date: Mon Sep 11 18:59:25 2017

[Media Router] Force to open cast channels on user gesture

- Added CastMediaSinkService::cast_sinks_ to store cast sinks found in last round of mDNS discovery
- Added CastMediaSinkImpl::ForceDiscovery() to open channels for sinks found in last round of mDNS
discovery, but without an opened cast channel

Bug: 752310
Change-Id: I6dcb3f829ebbf85578fdeeca5a2ba80a59589d8d
Reviewed-on: https://chromium-review.googlesource.com/651447
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500989}
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc
[modify] https://crrev.com/9439571e71419c717dffd16bc6b9722b1e27762d/chrome/browser/media/router/mojo/media_router_mojo_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07a1fb4be2e15b568f1cb453bdf810418b19e777

commit 07a1fb4be2e15b568f1cb453bdf810418b19e777
Author: Bin Zhao <zhaobin@chromium.org>
Date: Tue Sep 12 21:00:39 2017

[Media Router] Add feature "EnableCastChannelRetry"

Usage:
"--enable-features=EnableCastDiscovery,EnableCastChannelRetry<MyStudyName
 --force-fieldtrials=MyStudyName/Group1 
 --force-fieldtrial-params=
    MyStudyName.Group1:initial_delay_ms/20000/max_retry_attempts/20/exponential/3.0"

- Added a finch feature "EnableCastChannelRetry" and made retry strategy parameters configurable
- Added CastMediaSinkServiceImpl::InitRetryParameters to parse feature parameters

Bug: 752310
Change-Id: I13d4c3eb8d27de32bbe08fe62a8d4f3cd096776d
Reviewed-on: https://chromium-review.googlesource.com/639671
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501392}
[modify] https://crrev.com/07a1fb4be2e15b568f1cb453bdf810418b19e777/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/07a1fb4be2e15b568f1cb453bdf810418b19e777/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h
[modify] https://crrev.com/07a1fb4be2e15b568f1cb453bdf810418b19e777/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/07a1fb4be2e15b568f1cb453bdf810418b19e777/chrome/browser/media/router/media_router_feature.cc
[modify] https://crrev.com/07a1fb4be2e15b568f1cb453bdf810418b19e777/chrome/browser/media/router/media_router_feature.h

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f89822b546ecb36d3799443172adba8e12c26fdc

commit f89822b546ecb36d3799443172adba8e12c26fdc
Author: Bin Zhao <zhaobin@chromium.org>
Date: Thu Sep 28 03:22:47 2017

[Media Router] Added metrics for in-browser cast discovery

Keep parity with mr.CastAnalytics (https://cs.corp.google.com/piper///depot/google3/chrome/dongle/chrome_mrp/cast_analytics.js).

Added:

RecordCastChannelConnectResult()
RecordDeviceDiscovery()
RecordDeviceChannelError()
RecordDeviceChannelOpenDuration()

Bug: 752310
Change-Id: I961114b65347d6d8dc33a6aae6e027f520e567f7
Reviewed-on: https://chromium-review.googlesource.com/673354
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504884}
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/media_sink_discovery_metrics.cc
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/media_sink_discovery_metrics.h
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/chrome/browser/media/router/discovery/media_sink_discovery_metrics_unittest.cc
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/f89822b546ecb36d3799443172adba8e12c26fdc/tools/metrics/histograms/histograms.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e469acd927e9406b056cd957d07fc128a042670

commit 8e469acd927e9406b056cd957d07fc128a042670
Author: Bin Zhao <zhaobin@chromium.org>
Date: Mon Oct 02 20:57:43 2017

[cast_channel] Add observer to cast socket in chrome.cast.channel.send function

We are trying to not call chrome.cast.channel.open / chrome.cast.channel.close if in-browser cast discovery is enabled. Currently message observer is only added in open function, if we skip opening, observer wont be added.

This patch registered cast socket message observer in send function as well.

Extension side change: cl/169416458

Bug: 752310
Change-Id: I96231c98bfbcf7163eee1f90869e4b5ae0941260
Reviewed-on: https://chromium-review.googlesource.com/664085
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505780}
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/components/cast_channel/cast_socket_service.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/components/cast_channel/cast_socket_service.h
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/components/cast_channel/cast_socket_service_unittest.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/components/cast_channel/cast_test_util.h
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/extensions/browser/api/cast_channel/cast_channel_api.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/extensions/browser/api/cast_channel/cast_channel_api.h
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/extensions/browser/api/cast_channel/cast_channel_apitest.cc
[modify] https://crrev.com/8e469acd927e9406b056cd957d07fc128a042670/extensions/browser/browser_context_keyed_service_factories.cc

Additional ideas to improve reliability in situations where mDNS stopped working:

(1) The problem is if DIAL discovery reaches a steady state (no new or updated devices) then Cast does not get notified even if Cast lost devices. This is because DIAL is not aware of Cast's state of devices.

A reasonable approach would be to "sync" devices from DIAL to Cast when we detect a Media Router user gesture, i.e. when dialog is opened. Then Cast will get a chance to re-establish devices that were lost previously. I believe this will make device availability much more reliable in cases where DIAL is working well but mDNS is not.

(2) Another approach is to leverage the caching work that Brandon has added support for.  Once we find a device on a given network, we can cache it and attempt reconnection on user gesture (this applies for Cast or DIAL equally).   We do need a way to expunge devices eventually, but this should make discovery more robust to transient problems with mDNS/DIAL.
I am working on a patch to implement approach (1) above.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2d88405e8a90290f805360456ee330d5d1b6a91

commit e2d88405e8a90290f805360456ee330d5d1b6a91
Author: Derek Cheng <imcheng@chromium.org>
Date: Wed Oct 18 01:35:06 2017

[MediaRouter]Force dual discovery on user gesture.

- Added OnUserGesture() method to MediaSinkService, which gets called
on MRDesktop::OnUserGesture().
- The method is meant to signal the MediaSinkService that a user
gesture has occurred. It can then use this signal to "force" sinks to
be discovered right away, if supported.
- CastMediaSinkService::ForceDiscovery is renamed to OnUserGesture().
- DialMediaSinkService::OnUserGesture will re-sync DIAL-discovered
sinks to CastMediaSinkService. This helps in a very specific scenario:
if mDNS stopped working and a Cast sink that was discovered before was
lost due to network flakiness, then this gives CastMediaSinkService a
chance to recover sinks via this "forced" dual discovery mechanism.

- Another thing we can try in a future patch is to have
CastMediaSinkService retry connections for sinks stored in the
per-networkID cache. We should also have a mechanism to evict stale
entries from the cache.

Bug: 752310,  753175 
Change-Id: I77cdca84e607699b581d189d8d00380979ef5451
Reviewed-on: https://chromium-review.googlesource.com/714639
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Reviewed-by: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509646}
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.h
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/dial/dial_media_sink_service_proxy.h
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/browser/media/router/mojo/media_router_desktop.cc
[modify] https://crrev.com/e2d88405e8a90290f805360456ee330d5d1b6a91/chrome/common/media_router/discovery/media_sink_service.h

Cc: powerb@chromium.org
Owner: imch...@chromium.org
Owner: x...@chromium.org
We've landed a few code changes to improve responsiveness reliability through network caches and user gesture plumbing. Right now we are working on experimenting with different parameters for Cast channels. Assigning to xjz@ to track the work.
Owner: imch...@chromium.org
Experiments were done tuning each individual parameter and the findings were documented. In general, increasing the max number of retry, increasing the exponential, increasing initial delay, or increasing the connect timeout can all improve the connect success rate, and some improvements are significantly.

Assigning this back to imcheng@ to track any further work that may test the combination of the parameters.
Owner: ----
Status: Available (was: Assigned)

Sign in to add a comment