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

Issue 787806 link

Starred by 2 users
Status: Assigned
Owner:
maxmorin@chromium.org
Cc:
█ olka@chromium.org
solenberg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac , Fuchsia
Pri: 2
Type: Feature

Blocked on:
issue 672469
issue 830493

Participants' hotlists:
AudioService-FixIt


Sign in to add a comment

Restart audio streams automatically on audio process restart.

Project Member Reported by maxmorin@chromium.org, Nov 22 2017 
If an audio stream is terminated due to the process hosting it restarting, we should start the stream again without any action being required from the user.

Design doc: https://docs.google.com/document/d/1BUrJS0re5JcSuhpwLyMXzBjm5JsNVS_0GxctHuOVt9M/edit (Google only due to some internal stats being shared).

Filing bug now just for tracking.

Comment 1 by maxmorin@chromium.org, Nov 22 2017

Blockedon: 672469

Comment 2 by maxmorin@chromium.org, Apr 9 2018

Blockedon: 830493
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 17 2018 

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

commit 901f4b6391a52e92371668ed3f59c09ad856df26
Author: Max Morin <maxmorin@chromium.org>
Date: Tue Apr 17 13:59:56 2018

Refactor flow of audio platform errors to renderer.

Currently, errors from the platform (e.g. device disconnected), are sent directly to the renderer
(AudioOutputStreamClient). When moving to the audio service, this becomes suboptimal. It is tricky
to signal an error occurring before the ClientPtr is set up, and sometimes we may not want it
torn down due to a platform error (in case we want to try some sort of recovery operation such as
restarting the audio service). This CL instead signals the error to the browser
(AudioOutputStreamObserver), which signals it to the client through the new
AudioOutputStreamProviderClient interface. That interface also gets the "stream created" callback
as a method rather than a mojo response, so that we will be able to call it multiple times in the
future.

Bug: 787806, 830493 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I0fca093e2676d96f0f397c0fb59dd6c31eae1384
Reviewed-on: https://chromium-review.googlesource.com/973367
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551314}
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/content/renderer/media/mojo_audio_output_ipc.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/content/renderer/media/mojo_audio_output_ipc.h
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/content/renderer/media/mojo_audio_output_ipc_unittest.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/audio/audio_output_device.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/audio/audio_output_device.h
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/interfaces/audio_output_stream.mojom
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/OWNERS
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream.h
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream_provider.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream_provider.h
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream_provider_unittest.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/media/mojo/services/mojo_audio_output_stream_unittest.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/output_stream.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/output_stream.h
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/output_stream_unittest.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/public/mojom/stream_factory.mojom
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/stream_factory.cc
[modify] https://crrev.com/901f4b6391a52e92371668ed3f59c09ad856df26/services/audio/stream_factory.h
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 22 2018 

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

commit 8a65cd0985a844e8824e6ac452e590892f0d1a06
Author: Noel Gordon <noel@chromium.org>
Date: Sun Apr 22 06:15:24 2018

Revert "Refactor flow of audio platform errors to renderer."

This reverts commit 901f4b6391a52e92371668ed3f59c09ad856df26.

Reason for revert: Sequence confusion about which thread a Mojo
binding_ receives on_connect_error{with_reason} relative to the
sequence_checker_ thread.

No browser tests covers this case: the sequence_checker_ thread
and the binding_ connection error thread might not be the same.
The DCHECK on sequence_checker_ added in this CL can cause test
failures, but no active tests cover the change, sadly.

Browser tests that could cover this work are disabled for other
reasons (blanket sheriff flaky test disables done badly, or not
investigated). The root cause of those test flakes was recently
investigated, and resolved.

Apologies for the revert, Max. I think we'll come back and land
this change after some browser test coverage on ChromeOS is re-
enabled, so you'll have some tests to support your work. Please
bear with this will we get those tests going again.

Original change's description:
> Refactor flow of audio platform errors to renderer.
>
> Currently, errors from the platform (e.g. device disconnected), are sent directly to the renderer
> (AudioOutputStreamClient). When moving to the audio service, this becomes suboptimal. It is tricky
> to signal an error occurring before the ClientPtr is set up, and sometimes we may not want it
> torn down due to a platform error (in case we want to try some sort of recovery operation such as
> restarting the audio service). This CL instead signals the error to the browser
> (AudioOutputStreamObserver), which signals it to the client through the new
> AudioOutputStreamProviderClient interface. That interface also gets the "stream created" callback
> as a method rather than a mojo response, so that we will be able to call it multiple times in the
> future.
>
> Bug: 787806, 830493 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: I0fca093e2676d96f0f397c0fb59dd6c31eae1384
> Reviewed-on: https://chromium-review.googlesource.com/973367
> Commit-Queue: Max Morin <maxmorin@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Yuri Wiitala <miu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551314}

TBR=dalecurtis@chromium.org,kinuko@chromium.org,miu@chromium.org,maxmorin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

No-Presubmit: true
Bug: 787806,  830493 
Change-Id: If6607bac536f46048bdddbd24faf5180aafbd51b
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1023570
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552600}
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/content/renderer/media/mojo_audio_output_ipc.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/content/renderer/media/mojo_audio_output_ipc.h
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/content/renderer/media/mojo_audio_output_ipc_unittest.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/audio/audio_output_device.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/audio/audio_output_device.h
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/interfaces/audio_output_stream.mojom
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/OWNERS
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream.h
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream_provider.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream_provider.h
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream_provider_unittest.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/media/mojo/services/mojo_audio_output_stream_unittest.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/output_stream.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/output_stream.h
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/output_stream_unittest.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/public/mojom/stream_factory.mojom
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/stream_factory.cc
[modify] https://crrev.com/8a65cd0985a844e8824e6ac452e590892f0d1a06/services/audio/stream_factory.h
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 22 2018 

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

commit b44ed3c60b2d1558f6149dfc962e73aff3ee0afc
Author: Noel Gordon <noel@chromium.org>
Date: Sun Apr 22 09:59:28 2018

[ChromeOS] Re-enable AudioPlayer FileManagerBrowserTests

Cause of the flakinessin the FileManagerBrowserTest suite was resolved
on  issue 831074   issue 804413   issue 829310   issue 618198 .

Re-enable the AudioPlayer tests in this browser test suite on all bots
RELEASE/DEBUG/MSAN/ASAN on ChromeOS.

OpenAudioOnDrive, OpenAudioOnDownloads were disabled on MSAN (in issue
508949). Re-enabling here to see if MSAN is still a problem.

Mash doesn't support these tests (see mash browser tests patch set #1)
so disable these AudioPlayer FileManagerBrowserTests in mash.

Tbr: yamaguchi@chromium.org
Tbr: maxmorin@chromium.org
Tbr: kbr@chromium.org
Bug:  835626 ,508949,787806, 830493 
Change-Id: I966bc9123b15fec453fe85fec09059af091c916f
Reviewed-on: https://chromium-review.googlesource.com/1023072
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552603}
[modify] https://crrev.com/b44ed3c60b2d1558f6149dfc962e73aff3ee0afc/chrome/browser/chromeos/file_manager/audio_player_browsertest.cc
[modify] https://crrev.com/b44ed3c60b2d1558f6149dfc962e73aff3ee0afc/testing/buildbot/filters/mash.browser_tests.filter
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 26 2018 

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

commit 6b683a83b02f13943182e187d30be567389eedfb
Author: Max Morin <maxmorin@chromium.org>
Date: Thu Apr 26 09:40:04 2018

Reland "Refactor flow of audio platform errors to renderer".

Original description, (original code is PS 1):

Currently, errors from the platform (e.g. device disconnected), are sent
directly to the renderer (AudioOutputStreamClient). When moving to the audio
service, this becomes suboptimal. It is tricky to signal an error occurring
before the ClientPtr is set up, and sometimes we may not want it torn down due
to a platform error (in case we want to try some sort of recovery operation
such as restarting the audio service). This CL instead signals the error to the
browser (AudioOutputStreamObserver), which signals it to the client through the
new AudioOutputStreamProviderClient interface. That interface also gets the
"stream created" callback as a method rather than a mojo response, so that we
will be able to call it multiple times in the future.

Diff PS 1 -> PS 2 removes a call to a sequence checker that was accidentally
added when rebasing the original CL.

Diff PS 2 -> last PS fixes an issue when pepper would call Start before the
stream is created. This is solved by storing the state that the client
expects in the MojoAudioOutputIPC object. That object then initializes the
state when the stream has been created. As an aside, it is a great preparation
for restarting stream, since MojoAudioOutputIPC will be able to restore the
volume and playing state without involvement from AudioOutputDevice.

Bug: 787806, 830493 ,834242
Change-Id: Iee3a3902e9086cc9df4c5d210e43e65c162273fe
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1023397
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553965}
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/media/audio_output_ipc_factory_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/media/mojo_audio_output_ipc.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/media/mojo_audio_output_ipc.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/media/mojo_audio_output_ipc_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/pepper/pepper_platform_audio_output.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/pepper/pepper_platform_audio_output.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/pepper/pepper_platform_audio_output_dev.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/content/renderer/pepper/pepper_platform_audio_output_dev.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/audio/audio_output_device.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/audio/audio_output_device.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/audio/audio_output_device_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/audio/audio_output_ipc.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/interfaces/audio_output_stream.mojom
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/OWNERS
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream_provider.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream_provider.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream_provider_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/media/mojo/services/mojo_audio_output_stream_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/output_stream.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/output_stream.h
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/output_stream_unittest.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/public/mojom/stream_factory.mojom
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/stream_factory.cc
[modify] https://crrev.com/6b683a83b02f13943182e187d30be567389eedfb/services/audio/stream_factory.h
►

Sign in to add a comment