WIP for output stream support: https://chromium-review.googlesource.com/c/chromium/src/+/982058. To settle: keep factory client in BrowserMainLoop or WebContentsImpl? Leaning towards client-per-WebContents now, unless there's some reason not to.
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
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
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
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
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3e13ebae39d49db75fd5659cfbf8daa9a538137 commit a3e13ebae39d49db75fd5659cfbf8daa9a538137 Author: Max Morin <maxmorin@chromium.org> Date: Wed May 02 09:26:05 2018 Audio service stream factory client in content/. Stuff of note: - Code is for the UI thread. All the state we care about (permissions, frame/WebContents events, UI indicators, ...) is on the UI thread, and there are some tricky ordering problems with having the code on the IO thread if we want to observe frames/wc. As a bonus, we can get rid of a bunch of code posting to the UI thread in permissions and monitoring code. - In order to not need helper WebContentsObservers that proxies to the ContentAudioStreamFactoryClient, we have one ContentAudioStreamFactoryClient per WebContents. This also simplifies keeping the group id for the WebContents (just instantiate it in the constructor. - State is put in AudioOutputStreamBroker (and AudioInputStreamBroker, LoopbackStreamBroker, ...). We can have super simple per-frame factories thread that just checks permissions and forwards the request to the FactoryClient. - AudioOutputController errors are sent to content/ through the AudioOutputStreamObserver interface, so that it knows if we had a real error, service crash, or the client terminated the stream peacefully. content/ then forwards the error to the client. This also eliminates the AudioOutputStreamClient interface, simplifying a bit. This is done in the dependency CL. Next up, a RendererAudioOutputStreamFactory implementation (living in RenderFrameHost) which propagates requests to the ForwardingStreamFactory will be added. Class diagram involving some of these things: https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit Bug: 830493 Change-Id: I7e3804fa4a6ec6de8a2d0a971ecd5be69d267b1c Reviewed-on: https://chromium-review.googlesource.com/982058 Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Tommi <tommi@chromium.org> Reviewed-by: Yuri Wiitala <miu@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#555331} [modify] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc [modify] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/BUILD.gn [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/audio_output_stream_broker.cc [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/audio_output_stream_broker.h [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/audio_output_stream_broker_unittest.cc [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/audio_stream_broker.cc [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/audio_stream_broker.h [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/forwarding_audio_stream_factory.cc [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/forwarding_audio_stream_factory.h [add] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/browser/media/forwarding_audio_stream_factory_unittest.cc [modify] https://crrev.com/a3e13ebae39d49db75fd5659cfbf8daa9a538137/content/test/BUILD.gn
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802 commit 5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802 Author: Max Morin <maxmorin@chromium.org> Date: Wed May 09 07:00:21 2018 Add ForwardingAudioStreamFactory to WebContentsImpl. Also add accessor to ForwardingAudioStreamFactory. Approximate diagram of stuff: https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit Bug: 830493 Change-Id: I309aedf43fed5e646545eb94d00a77abc6ac66ea Reviewed-on: https://chromium-review.googlesource.com/1039707 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#557111} [modify] https://crrev.com/5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802/content/browser/media/forwarding_audio_stream_factory.cc [modify] https://crrev.com/5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802/content/browser/media/forwarding_audio_stream_factory.h [modify] https://crrev.com/5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/5bc74f5fff976f92f3b9ff3a71c9d1d8ffc58802/content/browser/web_contents/web_contents_impl.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59f892fc9d3c442f4381ebd220658b19644812ba commit 59f892fc9d3c442f4381ebd220658b19644812ba Author: Max Morin <maxmorin@chromium.org> Date: Thu May 10 10:55:21 2018 Add a new RenderFrameAudioOutputStreamFactory It checks that the stream is allowed and forwards the request to the relevant ForwardingAudioOutputStreamFactory if so. This will cause the stream to be served by the audio service. The old RenderFrameAudioOutputStreamFactory which creates streams living in content/ is renamed to OldRenderFrameAudioOutputStreamFactory. Since the class was renamed, the files were moved (by adding "old_" to the beginning). No need to review those files. Also note that replacement is diffed against the previous implementation. It's probably best to just ignore the diff and review render_frame_audio_output_stream_factory{.cc,.h,_unittest.cc} as new files. A flag is added to switch between the old factory and the new one. Approximate diagram of stuff: https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit Tbr since there was a "verbal" LGTM in cl comments. Tbr: nasko Bug: 830493 Change-Id: I38887bb97c817cc9182d71edd89d5ff0193b2504 Reviewed-on: https://chromium-review.googlesource.com/1032751 Commit-Queue: Max Morin <maxmorin@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Cr-Commit-Position: refs/heads/master@{#557492} [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/BUILD.gn [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/media/forwarding_audio_stream_factory.h [add] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/old_render_frame_audio_output_stream_factory.cc [add] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/old_render_frame_audio_output_stream_factory.h [add] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/old_render_frame_audio_output_stream_factory_unittest.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/render_frame_audio_output_stream_factory.h [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/render_frame_audio_output_stream_factory_unittest.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/renderer_audio_output_stream_factory_context.h [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/browser/renderer_host/media/renderer_audio_output_stream_factory_context_impl.h [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/public/app/mojo/content_browser_manifest.json [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/public/common/content_features.cc [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/public/common/content_features.h [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/content/test/BUILD.gn [modify] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/services/audio/public/cpp/BUILD.gn [add] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/services/audio/public/cpp/fake_stream_factory.cc [add] https://crrev.com/59f892fc9d3c442f4381ebd220658b19644812ba/services/audio/public/cpp/fake_stream_factory.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/539d243b087e6e31281c3229929f5a1fc97cd82f commit 539d243b087e6e31281c3229929f5a1fc97cd82f Author: Max Morin <maxmorin@chromium.org> Date: Fri May 11 10:02:07 2018 Add a new RenderFrameAudioInputStreamFactory It checks that the stream is allowed and forwards the request to the relevant ForwardingAudioStreamFactory if so. This will cause the stream to be served by the audio service. The old RenderFrameAudioInputStreamFactory which creates streams living in content/ is renamed to OldRenderFrameAudioInputStreamFactory. Since the class was renamed, the files were moved (by adding "old_" to the beginning). No need to review those files. Also note that replacement is diffed against the previous implementation. It's probably best to just ignore the diff and review render_frame_audio_input_stream_factory{.cc,.h,_unittest.cc} as new files. The same flag as for output is used to switch between the old factory and the new one. Approximate diagram of stuff: https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit Bug: 830493 , 836226 Change-Id: Ib0f58a52d849a48ba79405f0d499444c66029b58 Reviewed-on: https://chromium-review.googlesource.com/1050245 Commit-Queue: Max Morin <maxmorin@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Cr-Commit-Position: refs/heads/master@{#557832} [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/BUILD.gn [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/media_stream_manager.cc [add] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.cc [add] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.h [add] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.cc [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.h [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/media/render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/539d243b087e6e31281c3229929f5a1fc97cd82f/content/test/BUILD.gn
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/80cbe682e287f7562c7fd7e7b0a24927de78675d commit 80cbe682e287f7562c7fd7e7b0a24927de78675d Author: Findit <findit-for-me@appspot.gserviceaccount.com> Date: Fri May 11 10:55:25 2018 Revert "Add a new RenderFrameAudioInputStreamFactory" This reverts commit 539d243b087e6e31281c3229929f5a1fc97cd82f. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 557832 as the culprit for flakes in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNTM5ZDI0M2IwODdlNmUzMTI4MWMzMjI5OTI5ZjVhMWZjOTdjZDgyZgw Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Cast%20Linux/None Sample Failed Step: content_unittests Sample Failed test: RenderFrameAudioInputStreamFactoryTest.CreateOpenedStream_ForwardsCall Original change's description: > Add a new RenderFrameAudioInputStreamFactory > > It checks that the stream is allowed and forwards the request to the > relevant ForwardingAudioStreamFactory if so. This will cause > the stream to be served by the audio service. > The old RenderFrameAudioInputStreamFactory which creates streams > living in content/ is renamed to > OldRenderFrameAudioInputStreamFactory. Since the class was renamed, > the files were moved (by adding "old_" to the beginning). No need to > review those files. Also note that replacement is diffed against > the previous implementation. It's probably best to just ignore the diff > and review render_frame_audio_input_stream_factory{.cc,.h,_unittest.cc} > as new files. > > The same flag as for output is used to switch between the old factory > and the new one. > > Approximate diagram of stuff: > https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit > > Bug: 830493 , 836226 > Change-Id: Ib0f58a52d849a48ba79405f0d499444c66029b58 > Reviewed-on: https://chromium-review.googlesource.com/1050245 > Commit-Queue: Max Morin <maxmorin@chromium.org> > Reviewed-by: Nasko Oskov <nasko@chromium.org> > Reviewed-by: Guido Urdaneta <guidou@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557832} No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 830493 , 836226 , 842115 Flaky step name: content_unittests Flaky test name: RenderFrameAudioInputStreamFactoryTest.CreateOpenedStream_ForwardsCall Change-Id: If1683461af7fcbe0a9be633db62087dafe66235b Reviewed-on: https://chromium-review.googlesource.com/1054162 Cr-Commit-Position: refs/heads/master@{#557838} [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/BUILD.gn [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/renderer_host/media/media_stream_manager.cc [delete] https://crrev.com/a550e3e2c31893045d97adad488ecb92c5cd76ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.cc [delete] https://crrev.com/a550e3e2c31893045d97adad488ecb92c5cd76ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.h [delete] https://crrev.com/a550e3e2c31893045d97adad488ecb92c5cd76ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.cc [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.h [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/renderer_host/media/render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/80cbe682e287f7562c7fd7e7b0a24927de78675d/content/test/BUILD.gn
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca commit 7f99801283d0fbf2261926f49e5c9cd34c6ee7ca Author: Max Morin <maxmorin@chromium.org> Date: Fri May 11 17:45:40 2018 Reland "Add a new RenderFrameAudioInputStreamFactory" This is a reland of 539d243b087e6e31281c3229929f5a1fc97cd82f Tests are updated to wait for a signal from the specific even they're waiting for, rather than just running a run loop. Ran a few hundred iterations each on debug, release, tsan, and asan to verify that the test is working as expected now. Original change's description: > Add a new RenderFrameAudioInputStreamFactory > > It checks that the stream is allowed and forwards the request to the > relevant ForwardingAudioStreamFactory if so. This will cause > the stream to be served by the audio service. > The old RenderFrameAudioInputStreamFactory which creates streams > living in content/ is renamed to > OldRenderFrameAudioInputStreamFactory. Since the class was renamed, > the files were moved (by adding "old_" to the beginning). No need to > review those files. Also note that replacement is diffed against > the previous implementation. It's probably best to just ignore the diff > and review render_frame_audio_input_stream_factory{.cc,.h,_unittest.cc} > as new files. > > The same flag as for output is used to switch between the old factory > and the new one. > > Approximate diagram of stuff: > https://docs.google.com/drawings/d/1_ZIKj6lihGKRjq4Mflduitmkn_REqpHFeqVNelBGHHk/edit > > Bug: 830493 , 836226 > Change-Id: Ib0f58a52d849a48ba79405f0d499444c66029b58 > Reviewed-on: https://chromium-review.googlesource.com/1050245 > Commit-Queue: Max Morin <maxmorin@chromium.org> > Reviewed-by: Nasko Oskov <nasko@chromium.org> > Reviewed-by: Guido Urdaneta <guidou@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557832} // Tbr since only tests were changed. Tbr: Bug: 830493 , 836226 Change-Id: Ib0dbb37f7e4b170d0bf6defef081b867214a6740 Reviewed-on: https://chromium-review.googlesource.com/1054237 Commit-Queue: Max Morin <maxmorin@chromium.org> Reviewed-by: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#557925} [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/BUILD.gn [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/media_stream_manager.cc [add] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.cc [add] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory.h [add] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/old_render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.cc [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/render_frame_audio_input_stream_factory.h [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/media/render_frame_audio_input_stream_factory_unittest.cc [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/7f99801283d0fbf2261926f49e5c9cd34c6ee7ca/content/test/BUILD.gn
[bulk-edit: disregard if N/A] Can the owner please set milestone to this bug if applicable?
Comment 1 by maxmorin@chromium.org
, Apr 9 2018