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

Issue 836226 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature


Participants' hotlists:
Audio-Service


Sign in to add a comment

Switch input streams to audio service

Project Member Reported by marinaciocea@chromium.org, Apr 24 2018

Issue description

Use audio service input streams IPC to create input streams.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 9 2018

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

commit 3d4b330d6e77245cd8b5e84ed380aca06408bdea
Author: Marina Ciocea <marinaciocea@chromium.org>
Date: Wed May 09 07:15:45 2018

Map user input monitor key press count in shared memory region.

Notes:
- Key press count is written to a share memory region and used by audio::InputStream implementation
(audio service side was already landed in https://crrev.com/c/1011605/31).
- AudioInputStreamBroker passes a readonly handle to the shmem over IPC to audio::InputStream.
- Lock in UserInputMonitor was removed because Enable/Disable can only be called from UI thread
in new audio service input stream implementation, or on audio thread in old implementation.
- UserInputMonitor::EnableKeyPressMonitoring() returning void will be removed after switching to
audio service input streams.

Design doc: https://docs.google.com/document/d/1GHL4uMlIFox2eAqsjUQVA8eJ7HYU3g2AoyAqyx3pxMA/edit?usp=sharing

Bug:  836226 

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: I75b5b4cb0ed49ce17df84a03300bb9da52d86846
Reviewed-on: https://chromium-review.googlesource.com/1032733
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557115}
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/content/browser/media/audio_input_stream_broker.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/content/browser/media/audio_input_stream_broker.h
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/content/browser/renderer_host/media/audio_input_delegate_impl_unittest.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/audio/audio_input_controller_unittest.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor.h
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor_linux.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor_mac.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor_unittest.cc
[modify] https://crrev.com/3d4b330d6e77245cd8b5e84ed380aca06408bdea/media/base/user_input_monitor_win.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 10 2018

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

commit c9f8ed2cf37b9b18c96ee144bf379ee9c5414b16
Author: Max Morin <maxmorin@chromium.org>
Date: Thu May 10 14:35:00 2018

Disable key press monitoring in audio input dtor

CleanUp may not be called (it may happen that the frame owning the
stream is destroyed before the signal to clean up the stream is
received from the renderer), so move key press stuff from there to
the destructor.

Bug:  836226 
Change-Id: I141b5942296c268b43a82934ec01c744931df65d
Reviewed-on: https://chromium-review.googlesource.com/1052697
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557510}
[modify] https://crrev.com/c9f8ed2cf37b9b18c96ee144bf379ee9c5414b16/content/browser/media/audio_input_stream_broker.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 11 2018

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

Project Member

Comment 4 by bugdroid1@chromium.org, May 11 2018

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

Project Member

Comment 5 by bugdroid1@chromium.org, May 11 2018

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

Status: Fixed (was: Assigned)

Sign in to add a comment