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

Issue 647185 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
OOO Dec 22 - Jan 8
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 677008
issue 647200



Sign in to add a comment

Remove WebrtcAudioPrivate(Set/Get)ActiveSinkFunction

Project Member Reported by olka@chromium.org, Sep 15 2016

Issue description

Output device selection functionality provided by WebrtcAudioPrivate(Set/Get)ActiveSinkFunction is covered by setSinkId, and the clients should switch to it.

Implementation of those (Set/Get)ActiveSinkFunction has the following problems:
1) What it does is that it iterates all controllers in the process and changes the device, regardless of discrepancies in sample rates or buffer sizes.
2) It introduces shared ownership of AudioOutputController objects we want to avoid.
3) Here (https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc?sq=package:chromium&dr=CSs&l=338)
it stores shared references to all the output controllers. But output controllers point to SyncReaders which they do not own
(https://cs.chromium.org/chromium/src/media/audio/audio_output_controller.cc?cl=GROK&gsn=Create&rcl=1473909537&l=23) and which are owned by AudioEntries (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audio_renderer_host.cc?cl=GROK&gsn=Create&rcl=1473909537&l=203).
So, webrtc private api may end up holding a reference to a partially corrupted controller (i.e. the one which points to a deleted sync reader - which does not manifest itself, because webrtc private api does not access sync reader).
 

Comment 1 by olka@chromium.org, Sep 15 2016

Cc: guidou@chromium.org tommi@chromium.org maxmorin@chromium.org

Comment 2 by olka@chromium.org, Sep 15 2016

Blocking: 647200

Comment 3 by olka@chromium.org, Sep 20 2016

Labels: M-56
Blocking: 677008
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 21 2017

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

commit bc189a38337d30fd6a5450658ab3547126d97485
Author: olka <olka@chromium.org>
Date: Fri Apr 21 11:19:43 2017

WebRTC Audio private API: removing WebRtcAudioPrivate(Set/Get)ActiveSinkFunction
together with GetOutputControllers() logic from renderer hosts.

They are meant to be replaced with HTMLMediaElement's sinkId and setSinkId()

*PENDING on external change - do not submit yet*

BUG= 647185 , 672468 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2784563003
Cr-Commit-Position: refs/heads/master@{#466312}

[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/browser/resources/hangout_services/manifest.json
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/browser/resources/hangout_services/thunk.js
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/common/extensions/api/webrtc_audio_private.idl
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/test/data/extensions/hangout_services_test.html
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/chrome/test/data/extensions/hangout_services_test.js
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/media/audio_output_delegate_impl.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/media/audio_output_delegate_impl.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/media/audio_output_delegate_impl_unittest.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/media/audio_renderer_host.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/media/audio_renderer_host.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/public/browser/render_process_host.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/media/audio/audio_output_controller.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/media/audio/audio_output_controller.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/media/audio/audio_output_controller_unittest.cc
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/media/audio/audio_output_delegate.h
[modify] https://crrev.com/bc189a38337d30fd6a5450658ab3547126d97485/tools/metrics/histograms/histograms.xml

Comment 6 by olka@chromium.org, Apr 21 2017

Labels: -M-56 M-60
Status: Fixed (was: Assigned)

Sign in to add a comment