Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1666363005:
Switching audio clients to using RestartableAudioRendererSink interface as a sink. (Closed)
|
Created:
4 years, 10 months ago by o1ka Modified:
4 years, 8 months ago Reviewers:
Henrik Grunell, Avi (use Gerrit), tommi (sloooow) - chröme, DaleCurtis, sandersd (OOO until July 31) CC:
vanellope-cl_google.com, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, miu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice.
This is the first step towards switching to mixing WebTRC/WebAudio audio
output together with media elements audio on the renderer side.
Interface is instantiated through AudioOutputFactory,
which as of now produces mixer AudioRendererMixerInput for media elements
and AudioOutputDevice for the rest of the clients.
The plan is in a follow-up CL to have it to produce AudioRendererMixerInput
instances for the rest of the clients basing on Finch experiment parameters
and OS (we won't mix on ChromOS and Android in the near future).
******Used to be (changed during the review) - this is if anybody wants to understand the first patches.
Switching audio clients to using RestartableAudioOutputDevice interface as a sink.
This is the first step towards switching to mixing WebTRC/WebAudio audio
output together with media elements audio on the renderer side.
1) A common interface RestartableAudioOutputDevice (which is
RestartableAudioRendererSink and AudioOutputDevice) is introduced.
2) AudioRendererMixerInput now inherits from it.
3) RestartableAudioOutputDeviceImpl implements it on top of
AudioOutputDevice (aggregated).
4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl
are switched to use this interface instead of AudioOutputDevice.
5) Interface is instantiated through RestartableAudioOutputDeviceFactory,
which as of now produces mixer AudioRendererMixerInput for media elements
and RestartableAudioOutputDeviceImpl for the rest of the clients.
The plan is in a follow-up CL to have it to produce AudioRendererMixerInput
instances for the rest of the clients basing on Finch experiment parameters
and OS (we won't mix on ChromOS and Android in the near future).
Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput
behavior allows to seamlessly switch between both implementations in run-time.
At this step the goal was to have as less client code modifications as
possible, in order to not overlay upcoming (hopefully few) mixing bugs with
integration bugs.
BUG=560378
Committed: https://crrev.com/b819869839848e8d4f5488fb9f3ff5bed254fb26
Cr-Commit-Position: refs/heads/master@{#376125}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fixing file name in GN build #Patch Set 3 : another fix for gn build #Patch Set 4 : Removing RestartableAudioOutputDevice interface #
Total comments: 10
Patch Set 5 : Getting rid of restartable audio output device #Patch Set 6 : export fix #
Total comments: 34
Patch Set 7 : addressing review comments #Patch Set 8 : Rebase #
Total comments: 8
Patch Set 9 : nit fixes #Messages
Total messages: 46 (16 generated)
Patchset #2 (id:20001) has been deleted
On 2016/02/05 14:57:31, o1ka wrote: > Patchset #2 (id:20001) has been deleted Lower similarity does not seem to work well. mock_audo_output_ipc.h and mock_render_callback.h are borrowed from audio_output_device_unittest.cc; restartable_audio_output_device_factory* follows audio_device_factory*
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, grunell@chromium.org, tommi@chromium.org
Could you take a look? Thanks! https://codereview.chromium.org/1666363005/diff/1/content/renderer/media/rest... File content/renderer/media/restartable_audio_output_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/1/content/renderer/media/rest... content/renderer/media/restartable_audio_output_device_factory.h:54: // TODO(olka): find a better home for this function. Could you suggest a better place for it? A dedicated file seems to be an overkill, doesn't it?
Will address build issues on Monday
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org
Do we really want a new interface and implementation for this? Dan created the restartable interface to avoid yak-shaving all the implementations temporarily. Long term I think we should only have the non-restartable interfaces and those should be fixed such that they support the restart semantics. +sandersd if he has further recollections.
On 2016/02/05 18:47:16, DaleCurtis wrote: > Do we really want a new interface and implementation for this? Dan created the > restartable interface to avoid yak-shaving all the implementations temporarily. > Long term I think we should only have the non-restartable interfaces and those > should be fixed such that they support the restart semantics. > > +sandersd if he has further recollections. Yes, it's a valid point, we would also like to see AudioOutputDevice restartable and were looking into it. And another problem with AOD that it exposes AudioOutputIPCDelegate and ScopedTaskRunnerObserver to the clients who are not interested in the details of implementation and communication. However, the main purpose of the new interface is not adding a "restartable" property, but introducing a single common base class for audio output device and mixer inputs (now they have two: OutputDevice and AudioRendererSink), because we want to let a factory to decide whether to create a mixer input or an AOD (see CL description item #5 and below). This to enable finch experiment for Vanellope "mix all: approach. It will also allow, for example, to never use mixers for WebRTC on ChromOs/Android. We don't want to introduce any changes those are not absolutely necessary, to have a smoother transition to mixing and a better bug localization (i.e. we want to minimize the risks of the change which touches most of Chrome audio), so we are leaving making AOD restartable out of Vanellope scope right now (but that may change). When AOD is fixed to be restartable it can inherit from this interface directly, or we can rename it to be AOD interface. Another plus of having a new interface is hiding AudioOutputIPCDelegate and ScopedTaskRunnerObserver interfaces from the clients, which at least simplifies mocks for client unit testing. All in all, we do need a new interface, and a new implementation is a temporary solution.
Actually, I just realized that we can access OutputDevice interface through (Restartable)AudioRendererSink<->GetOutputDevice() interface so we can throw away this new interface by switching clients to use RestartableAudioRendererSink and AudioRendererSink::GetOutputDevice()->... instead of calling OutputDevice methods directly. Please hold on with the review, I'll fix it.
Patchset #4 (id:80001) has been deleted
Please take a look- RestartableAudioOutputDevice is removed. I haven't renamed RestartableAudioOutputDeviceImpl so far (because not sure if it stays), as well as RestartableAudioOutputDeviceFactory (because can't come up with a good name). We can probably remove RestartableAudioOutputDeviceImpl as well, but I would prefer to have it since it's behavior is similar to AudioRendererMixerInput one.
Generally when I think of temporary solutions, I'm not thinking of 1100+ lines of code :) I don't think making AOD restartable vs always wrapping it in a restartable sink reduces your bug surface in any meaningful way. Instead I think it does add another layer of complexity to an already complex pipeline. It's much easier to reason about an AOD which is always restartable. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:14: // Required for RestartableAudioOutputDeviceFactory::GetOutputHWConfig(): We don't typically do comments like this in headers. Remove? https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:42: base::Bind(&RestartableAudioOutputDeviceFactory::GetOutputHWParams, This is great actually, I was just looking at doing a similar change :) With this we can remove the need for AudioRendererImpl to block on GetAudioHardwareConfig sync IPC which should save a few tens of milliseconds for video playback startup. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:80: sink_->Play(); Why Play()? Does this preserve the play on start behavior? https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/restartable_audio_output_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/restartable_audio_output_device_factory.cc:89: media::AudioParameters RestartableAudioOutputDeviceFactory::GetOutputHWParams( This adds overhead that I don't think we want. When I was looking at a similar change, I was thinking that instead the ARMM could first try to find an existing mixer that has an open sink for the given device/origin and return its parameters. Failing that it can create a sink into a temporary pool which will be kept alive for a subsequent GetMixer() request; if no request comes in within some time the pool can be released. I don't believe this ends up creating shmem or a thread (since those happen on CreateStream), but still the round trip w/ a waitable event is unfortunate to do more than necessary. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/restartable_audio_output_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/restartable_audio_output_device_factory.h:30: class CONTENT_EXPORT RestartableAudioOutputDeviceFactory { Should we just make these new methods on the existing AudioDeviceFactory::NewRestartableOutputDevice() ? I don't really see the need for a new class here; especially if we want to merge these eventually.
Thanks for the feedback, will address the comments tomorrow. A couple of clarification questions: On 2016/02/08 19:10:25, DaleCurtis wrote: > Generally when I think of temporary solutions, I'm not thinking of 1100+ lines > of code :) Yes, I was thinking about it as well :) but it was a good exercise anyways. So, your opinion is we should throw it away now? I'm still in doubt since I'm not sure if we'll have restartable AOD in the nearest future. On the other hand, since media elements will never use AOD directly, we can have a separate factory method which will produce non-restartable sinks for the rest of the clients. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > content/renderer/media/audio_renderer_mixer_manager.cc:42: > base::Bind(&RestartableAudioOutputDeviceFactory::GetOutputHWParams, > This is great actually, I was just looking at doing a similar change :) With > this we can remove the need for AudioRendererImpl to block on > GetAudioHardwareConfig sync IPC which should save a few tens of milliseconds for > video playback startup. Great! I haven't though of performance implications actually, but this is also required for non-default device usage. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > content/renderer/media/renderer_webaudiodevice_impl.cc:80: sink_->Play(); > Why Play()? Does this preserve the play on start behavior? Yes, for mixer inputs (We don't want to touch client code yet, so we preserve the behavior for now). > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > File content/renderer/media/restartable_audio_output_device_factory.cc (right): > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > content/renderer/media/restartable_audio_output_device_factory.cc:89: > media::AudioParameters RestartableAudioOutputDeviceFactory::GetOutputHWParams( > This adds overhead that I don't think we want. When I was looking at a similar > change, I was thinking that instead the ARMM could first try to find an existing > mixer that has an open sink for the given device/origin and return its > parameters. Failing that it can create a sink into a temporary pool which will > be kept alive for a subsequent GetMixer() request; if no request comes in within > some time the pool can be released. > > I don't believe this ends up creating shmem or a thread (since those happen on > CreateStream), but still the round trip w/ a waitable event is unfortunate to do > more than necessary. Great idea, thanks! Will look into it. > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > File content/renderer/media/restartable_audio_output_device_factory.h (right): > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > content/renderer/media/restartable_audio_output_device_factory.h:30: class > CONTENT_EXPORT RestartableAudioOutputDeviceFactory { > Should we just make these new methods on the existing > AudioDeviceFactory::NewRestartableOutputDevice() ? I don't really see the need > for a new class here; especially if we want to merge these eventually. Yes, sounds good. Is the method name still ok? - It will produce a restartable sink, not a device.
On 2016/02/08 20:43:10, o1ka wrote: > Thanks for the feedback, will address the comments tomorrow. > A couple of clarification questions: > > On 2016/02/08 19:10:25, DaleCurtis wrote: > > Generally when I think of temporary solutions, I'm not thinking of 1100+ lines > > of code :) > Yes, I was thinking about it as well :) but it was a good exercise anyways. > So, your opinion is we should throw it away now? I'm still in doubt since I'm > not > sure if we'll have restartable AOD in the nearest future. On the other hand, > since media elements > will never use AOD directly, we can have a separate factory method which will > produce > non-restartable sinks for the rest of the clients. Yes, sorry, I think we should remove it unless there is a requirement at present. If we ever need it in this form we can restore an older patch set :) > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > content/renderer/media/audio_renderer_mixer_manager.cc:42: > > base::Bind(&RestartableAudioOutputDeviceFactory::GetOutputHWParams, > > This is great actually, I was just looking at doing a similar change :) With > > this we can remove the need for AudioRendererImpl to block on > > GetAudioHardwareConfig sync IPC which should save a few tens of milliseconds > for > > video playback startup. > Great! I haven't though of performance implications actually, but this is also > required for non-default device usage. > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > content/renderer/media/renderer_webaudiodevice_impl.cc:80: sink_->Play(); > > Why Play()? Does this preserve the play on start behavior? > Yes, for mixer inputs (We don't want to touch client code yet, so we preserve > the behavior for now). > > > > > > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > File content/renderer/media/restartable_audio_output_device_factory.cc > (right): > > > > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > content/renderer/media/restartable_audio_output_device_factory.cc:89: > > media::AudioParameters RestartableAudioOutputDeviceFactory::GetOutputHWParams( > > This adds overhead that I don't think we want. When I was looking at a similar > > change, I was thinking that instead the ARMM could first try to find an > existing > > mixer that has an open sink for the given device/origin and return its > > parameters. Failing that it can create a sink into a temporary pool which will > > be kept alive for a subsequent GetMixer() request; if no request comes in > within > > some time the pool can be released. > > > > I don't believe this ends up creating shmem or a thread (since those happen on > > CreateStream), but still the round trip w/ a waitable event is unfortunate to > do > > more than necessary. > Great idea, thanks! Will look into it. > > > > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > File content/renderer/media/restartable_audio_output_device_factory.h (right): > > > > > https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... > > content/renderer/media/restartable_audio_output_device_factory.h:30: class > > CONTENT_EXPORT RestartableAudioOutputDeviceFactory { > > Should we just make these new methods on the existing > > AudioDeviceFactory::NewRestartableOutputDevice() ? I don't really see the need > > for a new class here; especially if we want to merge these eventually. > Yes, sounds good. Is the method name still ok? - It will produce a restartable > sink, not a device. Yes I think the name is fine.
On 2016/02/08 22:05:27, DaleCurtis wrote: > On 2016/02/08 20:43:10, o1ka wrote: > > Thanks for the feedback, will address the comments tomorrow. > > A couple of clarification questions: > > > > On 2016/02/08 19:10:25, DaleCurtis wrote: > > > Generally when I think of temporary solutions, I'm not thinking of 1100+ > lines > > > of code :) > > Yes, I was thinking about it as well :) but it was a good exercise anyways. > > So, your opinion is we should throw it away now? I'm still in doubt since I'm > > not > > sure if we'll have restartable AOD in the nearest future. On the other hand, > > since media elements > > will never use AOD directly, we can have a separate factory method which will > > produce > > non-restartable sinks for the rest of the clients. > > Yes, sorry, I think we should remove it unless there is a requirement at > present. If we ever need it in this form we can restore an older patch set :) Olga, is this whole CL temporary code if we later make AOD restartable? Or some part? How much?
On 2016/02/09 07:41:05, Henrik Grunell wrote: > On 2016/02/08 22:05:27, DaleCurtis wrote: > > On 2016/02/08 20:43:10, o1ka wrote: > > > Thanks for the feedback, will address the comments tomorrow. > > > A couple of clarification questions: > > > > > > On 2016/02/08 19:10:25, DaleCurtis wrote: > > > > Generally when I think of temporary solutions, I'm not thinking of 1100+ > > lines > > > > of code :) > > > Yes, I was thinking about it as well :) but it was a good exercise anyways. > > > So, your opinion is we should throw it away now? I'm still in doubt since > I'm > > > not > > > sure if we'll have restartable AOD in the nearest future. On the other hand, > > > since media elements > > > will never use AOD directly, we can have a separate factory method which > will > > > produce > > > non-restartable sinks for the rest of the clients. > > > > Yes, sorry, I think we should remove it unless there is a requirement at > > present. If we ever need it in this form we can restore an older patch set :) > > Olga, is this whole CL temporary code if we later make AOD restartable? Or some > part? How much? Restartable AOD is(was) temporary.
Please take a look - A poor little restartable AOD is removed. As for Dale's idea of using existing AODs owned by mixers to obtain HW parameters and caching temporary AODs - I think it's great, but I need to give it some more thoughts with regard of upcoming mixer changes for "mix all" approach. Would prefer to have a separate CL for that if possible. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:14: // Required for RestartableAudioOutputDeviceFactory::GetOutputHWConfig(): On 2016/02/08 19:10:25, DaleCurtis wrote: > We don't typically do comments like this in headers. Remove? Done. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:42: base::Bind(&RestartableAudioOutputDeviceFactory::GetOutputHWParams, On 2016/02/08 19:10:25, DaleCurtis wrote: > This is great actually, I was just looking at doing a similar change :) With > this we can remove the need for AudioRendererImpl to block on > GetAudioHardwareConfig sync IPC which should save a few tens of milliseconds for > video playback startup. Great! Since we are moving towards output device selection and AudioHardwareConfig is for default device only, probably we should retire its output part at some point. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/renderer_webaudiodevice_impl.cc:80: sink_->Play(); On 2016/02/08 19:10:25, DaleCurtis wrote: > Why Play()? Does this preserve the play on start behavior? Yes, for mixer inputs https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/restartable_audio_output_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/restartable_audio_output_device_factory.cc:89: media::AudioParameters RestartableAudioOutputDeviceFactory::GetOutputHWParams( On 2016/02/08 19:10:25, DaleCurtis wrote: > This adds overhead that I don't think we want. When I was looking at a similar > change, I was thinking that instead the ARMM could first try to find an existing > mixer that has an open sink for the given device/origin and return its > parameters. Failing that it can create a sink into a temporary pool which will > be kept alive for a subsequent GetMixer() request; if no request comes in within > some time the pool can be released. > > I don't believe this ends up creating shmem or a thread (since those happen on > CreateStream), but still the round trip w/ a waitable event is unfortunate to do > more than necessary. This is a great idea. I'll take care of it. https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... File content/renderer/media/restartable_audio_output_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/100001/content/renderer/media... content/renderer/media/restartable_audio_output_device_factory.h:30: class CONTENT_EXPORT RestartableAudioOutputDeviceFactory { On 2016/02/08 19:10:25, DaleCurtis wrote: > Should we just make these new methods on the existing > AudioDeviceFactory::NewRestartableOutputDevice() ? I don't really see the need > for a new class here; especially if we want to merge these eventually. Done.
https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:145: scoped_refptr<media::AudioOutputDevice> device = Hmm, why do we need to make up a device status if no device exists? Seems this should just return an error if a device hasn't been created. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:61: static scoped_refptr<media::AudioRendererSink> NewOutputDevice( Can you give this a different name? We should avoid overloading where possible. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:21: media::AudioParameters AudioRendererMixerManager::GetOutputHWParams( Put this in function order relative to the header file. Only internal static functions go at the top of the file. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:29: // to some mixer) and reuse it. Can you file a bug and list it here too? https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:130: // A helper to get output HW parameters in the absence of AudioOutputDevice. Methods always go above variables. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:131: static media::AudioParameters GetOutputHWParams( Naming wise this should be something like GetHardwareOutputParams. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/webrtc_audio_renderer.cc:228: media::OUTPUT_DEVICE_STATUS_OK) Add {} around multi-line conditionals.
First round. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:24: // This is where we decide which audio will go to mixers and wich one to Nit: wich -> which https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:120: NOTIMPLEMENTED(); What should be implemented here? https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:38: kSourceWebAudio, When the categories or explicit latencies for WebAudio is in place, how would that be reflected here? No change? Since there is one source called "HighLatency". https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:61: static scoped_refptr<media::AudioRendererSink> NewOutputDevice( On 2016/02/10 23:25:33, DaleCurtis wrote: > Can you give this a different name? We should avoid overloading where possible. Maybe simply call it NewAudioRendererSink? (That's what it returns.) https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:72: NewRestartableOutputDevice(SourceType source_type, Name it NewRestartableAudioRendererSink, assuming the above is changed to the suggestion. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:78: // A helper to get HW device status in the absence of AudioOutputDevice. Nit: HW -> hardware https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:105: virtual media::AudioRendererSink* CreateOutputDevice( Change name according to the public versions if they are changed. https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_input.h (right): https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input.h:38: GetHWParamsCB; Name it "GetHardwareParamsCallback". https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input.h:87: const GetHWParamsCB get_hw_params_cb_; Same here, spell out the abbreviations in the member variable.
Addressed review comments, however I need to rebase after WebRtcLocalAudioRenderer was renamed by mui@ - please hold on with the review until than. dalecurtis@ could you take a look at my questions? https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:24: // This is where we decide which audio will go to mixers and wich one to On 2016/02/11 12:01:26, Henrik Grunell wrote: > Nit: wich -> which Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:120: NOTIMPLEMENTED(); On 2016/02/11 12:01:26, Henrik Grunell wrote: > What should be implemented here? We'll add AOD creation here as soon as it's restartable. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:145: scoped_refptr<media::AudioOutputDevice> device = On 2016/02/10 23:25:33, DaleCurtis wrote: > Hmm, why do we need to make up a device status if no device exists? Seems this > should just return an error if a device hasn't been created. This method is the code I moved from RenderFrameImpl::checkIfAudioSinkExistsAndIsAuthorized to keep an eye on it. However, the question: here https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... If we switch to mixer inputs, there is no mixer/AOD available until we start playing it out, but the sink(mixer input) is fine. Now if there is no mixer, we return OK if the mixer input is not started yet, and error if the input is started. Is it ok? What about the case when device is not authorized, for example, but we are returning OK until we get the mixer? What the problem might be if we get a mixer reference from the beginning, and just add a mixer input on Play? It could eliminate the need for caching (i.e. we'll have caching out of the box) Is it about opening several IPC channels and not using them when a page is loaded but media element play out is not started? https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:38: kSourceWebAudio, On 2016/02/11 12:01:26, Henrik Grunell wrote: > When the categories or explicit latencies for WebAudio is in place, how would > that be reflected here? No change? Since there is one source called > "HighLatency". renamed https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:61: static scoped_refptr<media::AudioRendererSink> NewOutputDevice( On 2016/02/11 12:01:26, Henrik Grunell wrote: > On 2016/02/10 23:25:33, DaleCurtis wrote: > > Can you give this a different name? We should avoid overloading where > possible. > > Maybe simply call it NewAudioRendererSink? (That's what it returns.) Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:72: NewRestartableOutputDevice(SourceType source_type, On 2016/02/11 12:01:26, Henrik Grunell wrote: > Name it NewRestartableAudioRendererSink, assuming the above is changed to the > suggestion. Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:78: // A helper to get HW device status in the absence of AudioOutputDevice. On 2016/02/11 12:01:26, Henrik Grunell wrote: > Nit: HW -> hardware Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.h:105: virtual media::AudioRendererSink* CreateOutputDevice( On 2016/02/11 12:01:26, Henrik Grunell wrote: > Change name according to the public versions if they are changed. Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:21: media::AudioParameters AudioRendererMixerManager::GetOutputHWParams( On 2016/02/10 23:25:33, DaleCurtis wrote: > Put this in function order relative to the header file. Only internal static > functions go at the top of the file. Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:29: // to some mixer) and reuse it. On 2016/02/10 23:25:33, DaleCurtis wrote: > Can you file a bug and list it here too? Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.h (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:130: // A helper to get output HW parameters in the absence of AudioOutputDevice. On 2016/02/10 23:25:33, DaleCurtis wrote: > Methods always go above variables. Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.h:131: static media::AudioParameters GetOutputHWParams( On 2016/02/10 23:25:33, DaleCurtis wrote: > Naming wise this should be something like GetHardwareOutputParams. Done. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/webrtc_audio_renderer.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/webrtc_audio_renderer.cc:228: media::OUTPUT_DEVICE_STATUS_OK) On 2016/02/10 23:25:33, DaleCurtis wrote: > Add {} around multi-line conditionals. Done. https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... File media/base/audio_renderer_mixer_input.h (right): https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input.h:38: GetHWParamsCB; On 2016/02/11 12:01:26, Henrik Grunell wrote: > Name it "GetHardwareParamsCallback". Done. https://codereview.chromium.org/1666363005/diff/140001/media/base/audio_rende... media/base/audio_renderer_mixer_input.h:87: const GetHWParamsCB get_hw_params_cb_; On 2016/02/11 12:01:26, Henrik Grunell wrote: > Same here, spell out the abbreviations in the member variable. Done.
Description was changed from ========== Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Old Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ==========
https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:145: scoped_refptr<media::AudioOutputDevice> device = On 2016/02/11 17:18:23, o1ka wrote: > On 2016/02/10 23:25:33, DaleCurtis wrote: > > Hmm, why do we need to make up a device status if no device exists? Seems this > > should just return an error if a device hasn't been created. > This method is the code I moved from > RenderFrameImpl::checkIfAudioSinkExistsAndIsAuthorized to keep an eye on it. > > However, the question: here > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > If we switch to mixer inputs, there is no mixer/AOD available until we start > playing it out, but the sink(mixer input) is fine. > Now if there is no mixer, we return OK if the mixer input is not started yet, > and error if the input is started. Is it ok? What about the case when device is > not authorized, for example, but we are returning OK until we get the mixer? > > What the problem might be if we get a mixer reference from the beginning, and > just add a mixer input on Play? It could eliminate the need for caching (i.e. > we'll have caching out of the box) Is it about opening several IPC channels and > not using them when a page is loaded but media element play out is not started? As soon as AudioRendererMixer is created it will auto start the stream, so we don't want to blindly request one unless we're about to use it. I haven't checked the call sites but it seems like it'd be fine to return OKAY when no device is connected. An error will still be generated later if the sink can't be used. You might check with guidou@ though, I don't remember all the details here. Another option is similar to what I think we should do for requesting the parameters, try to find an existing one, but if not lazily create one that will be used by the mixer in the future or killed after some timeout.
On 2016/02/11 17:37:04, DaleCurtis wrote: > https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... > File content/renderer/media/audio_device_factory.cc (right): > > https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... > content/renderer/media/audio_device_factory.cc:145: > scoped_refptr<media::AudioOutputDevice> device = > On 2016/02/11 17:18:23, o1ka wrote: > > On 2016/02/10 23:25:33, DaleCurtis wrote: > > > Hmm, why do we need to make up a device status if no device exists? Seems > this > > > should just return an error if a device hasn't been created. > > This method is the code I moved from > > RenderFrameImpl::checkIfAudioSinkExistsAndIsAuthorized to keep an eye on it. > > > > However, the question: here > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > If we switch to mixer inputs, there is no mixer/AOD available until we start > > playing it out, but the sink(mixer input) is fine. > > Now if there is no mixer, we return OK if the mixer input is not started yet, > > and error if the input is started. Is it ok? What about the case when device > is > > not authorized, for example, but we are returning OK until we get the mixer? > > > > What the problem might be if we get a mixer reference from the beginning, and > > just add a mixer input on Play? It could eliminate the need for caching (i.e. > > we'll have caching out of the box) Is it about opening several IPC channels > and > > not using them when a page is loaded but media element play out is not > started? > > As soon as AudioRendererMixer is created it will auto start the stream, so we > don't want to blindly request one unless we're about to use it. I haven't > checked the call sites but it seems like it'd be fine to return OKAY when no > device is connected. An error will still be generated later if the sink can't be > used. You might check with guidou@ though, I don't remember all the details > here. > > Another option is similar to what I think we should do for requesting the > parameters, try to find an existing one, but if not lazily create one that will > be used by the mixer in the future or killed after some timeout. I see. Can we make a mixer to start the stream only when the first input is added? What would be the problem with that? Agree on another option, that's what I thought, too.
olka@chromium.org changed reviewers: + avi@chromium.org
Rebased, PTAL. Adding avi@ for render_frame_impl.cc
On 2016/02/11 18:08:41, o1ka wrote: > > I see. Can we make a mixer to start the stream only when the first input is > added? > What would be the problem with that? > Agree on another option, that's what I thought, too. Stream startup takes some time, so ideally we want to start it slightly ahead of when we'll actually use it. We also immediately add an input to the mixer once requested, so it wouldn't change much. That said I don't think we're actually starting early today. Instead we only request the mixer when an input receives a Play() call, which is right before we expect to play data. In that case the only overhead is the extra IPC calls and small amount of memory used by the class. It's probably fine to have ARMM maintain a temporary pool of sinks which it uses for both vending mixers and fulfilling early requests for status and hardware parameters. That pool can be drained to create new mixers and if they fall into disuse.
Looking good, just minor comment. Also, update the description to only include the current information. (You could add any obsolete info to the bug if you want it for reference.) https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_device_factory.h:36: kSourceWebRTC, Nit: RTC -> Rtc https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:151: // TODO(olka): first try to lookup an existing device (cached or belonging Nit: first -> First https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:164: device->Stop(); // TODO(olka): temporary cash for future reuse. Seems like the same todo as above. If yes, remove this one. https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/track_audio_renderer.cc:331: sink_->Play(); // Not all the sinks play on start. Ahh, the fact that sinks behave differently is horrible. Well, we'll have to look over this in the future.
lgtm with comments fixed.
content/renderer/render_frame_impl.cc LGTM
lgtm % one extra comment. https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:145: scoped_refptr<media::AudioOutputDevice> device = On 2016/02/11 17:37:03, DaleCurtis wrote: > On 2016/02/11 17:18:23, o1ka wrote: > > On 2016/02/10 23:25:33, DaleCurtis wrote: > > > Hmm, why do we need to make up a device status if no device exists? Seems > this > > > should just return an error if a device hasn't been created. > > This method is the code I moved from > > RenderFrameImpl::checkIfAudioSinkExistsAndIsAuthorized to keep an eye on it. > > > > However, the question: here > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > If we switch to mixer inputs, there is no mixer/AOD available until we start > > playing it out, but the sink(mixer input) is fine. > > Now if there is no mixer, we return OK if the mixer input is not started yet, > > and error if the input is started. Is it ok? What about the case when device > is > > not authorized, for example, but we are returning OK until we get the mixer? > > > > What the problem might be if we get a mixer reference from the beginning, and > > just add a mixer input on Play? It could eliminate the need for caching (i.e. > > we'll have caching out of the box) Is it about opening several IPC channels > and > > not using them when a page is loaded but media element play out is not > started? > > As soon as AudioRendererMixer is created it will auto start the stream, so we > don't want to blindly request one unless we're about to use it. I haven't > checked the call sites but it seems like it'd be fine to return OKAY when no > device is connected. An error will still be generated later if the sink can't be > used. You might check with guidou@ though, I don't remember all the details > here. > > Another option is similar to what I think we should do for requesting the > parameters, try to find an existing one, but if not lazily create one that will > be used by the mixer in the future or killed after some timeout. Can you add a comment w/ a bug link here that we should find a more efficient way to handle this?
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Old Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody whant to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ==========
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody whant to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody whant to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ==========
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody whant to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ==========
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666363005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666363005/200001
Review comments addressed https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/1666363005/diff/140001/content/renderer/media... content/renderer/media/audio_device_factory.cc:145: scoped_refptr<media::AudioOutputDevice> device = On 2016/02/16 21:39:12, DaleCurtis wrote: > On 2016/02/11 17:37:03, DaleCurtis wrote: > > On 2016/02/11 17:18:23, o1ka wrote: > > > On 2016/02/10 23:25:33, DaleCurtis wrote: > > > > Hmm, why do we need to make up a device status if no device exists? Seems > > this > > > > should just return an error if a device hasn't been created. > > > This method is the code I moved from > > > RenderFrameImpl::checkIfAudioSinkExistsAndIsAuthorized to keep an eye on it. > > > > > > However, the question: here > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > If we switch to mixer inputs, there is no mixer/AOD available until we start > > > playing it out, but the sink(mixer input) is fine. > > > Now if there is no mixer, we return OK if the mixer input is not started > yet, > > > and error if the input is started. Is it ok? What about the case when device > > is > > > not authorized, for example, but we are returning OK until we get the mixer? > > > > > > What the problem might be if we get a mixer reference from the beginning, > and > > > just add a mixer input on Play? It could eliminate the need for caching > (i.e. > > > we'll have caching out of the box) Is it about opening several IPC channels > > and > > > not using them when a page is loaded but media element play out is not > > started? > > > > As soon as AudioRendererMixer is created it will auto start the stream, so we > > don't want to blindly request one unless we're about to use it. I haven't > > checked the call sites but it seems like it'd be fine to return OKAY when no > > device is connected. An error will still be generated later if the sink can't > be > > used. You might check with guidou@ though, I don't remember all the details > > here. > > > > Another option is similar to what I think we should do for requesting the > > parameters, try to find an existing one, but if not lazily create one that > will > > be used by the mixer in the future or killed after some timeout. > > Can you add a comment w/ a bug link here that we should find a more efficient > way to handle this? Done. https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/audio_device_factory.h (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_device_factory.h:36: kSourceWebRTC, On 2016/02/15 12:21:17, Henrik Grunell wrote: > Nit: RTC -> Rtc Done. https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:151: // TODO(olka): first try to lookup an existing device (cached or belonging On 2016/02/15 12:21:17, Henrik Grunell wrote: > Nit: first -> First Done. https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:164: device->Stop(); // TODO(olka): temporary cash for future reuse. On 2016/02/15 12:21:17, Henrik Grunell wrote: > Seems like the same todo as above. If yes, remove this one. Not the same/ The first is about using a cached device, and the second is about cashing a created device. https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... File content/renderer/media/track_audio_renderer.cc (right): https://codereview.chromium.org/1666363005/diff/180001/content/renderer/media... content/renderer/media/track_audio_renderer.cc:331: sink_->Play(); // Not all the sinks play on start. On 2016/02/15 12:21:17, Henrik Grunell wrote: > Ahh, the fact that sinks behave differently is horrible. Well, we'll have to > look over this in the future. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, avi@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1666363005/#ps200001 (title: "nit fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666363005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666363005/200001
Message was sent while issue was closed.
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 ========== to ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 Committed: https://crrev.com/b819869839848e8d4f5488fb9f3ff5bed254fb26 Cr-Commit-Position: refs/heads/master@{#376125} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b819869839848e8d4f5488fb9f3ff5bed254fb26 Cr-Commit-Position: refs/heads/master@{#376125}
Message was sent while issue was closed.
Description was changed from ========== ******Updated Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 Committed: https://crrev.com/b819869839848e8d4f5488fb9f3ff5bed254fb26 Cr-Commit-Position: refs/heads/master@{#376125} ========== to ========== Switching audio clients to using RestartableAudioRendererSink instead of AudioOutputDevice. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. Interface is instantiated through AudioOutputFactory, which as of now produces mixer AudioRendererMixerInput for media elements and AudioOutputDevice for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). ******Used to be (changed during the review) - this is if anybody wants to understand the first patches. Switching audio clients to using RestartableAudioOutputDevice interface as a sink. This is the first step towards switching to mixing WebTRC/WebAudio audio output together with media elements audio on the renderer side. 1) A common interface RestartableAudioOutputDevice (which is RestartableAudioRendererSink and AudioOutputDevice) is introduced. 2) AudioRendererMixerInput now inherits from it. 3) RestartableAudioOutputDeviceImpl implements it on top of AudioOutputDevice (aggregated). 4) WebRTCAudioRenderer, WebRTCLocalAudioRenderer and RendererWebAudioDeviceImpl are switched to use this interface instead of AudioOutputDevice. 5) Interface is instantiated through RestartableAudioOutputDeviceFactory, which as of now produces mixer AudioRendererMixerInput for media elements and RestartableAudioOutputDeviceImpl for the rest of the clients. The plan is in a follow-up CL to have it to produce AudioRendererMixerInput instances for the rest of the clients basing on Finch experiment parameters and OS (we won't mix on ChromOS and Android in the near future). Having RestartableAudioOutputDeviceImpl mimicing AudioRendererMixerInput behavior allows to seamlessly switch between both implementations in run-time. At this step the goal was to have as less client code modifications as possible, in order to not overlay upcoming (hopefully few) mixing bugs with integration bugs. BUG=560378 Committed: https://crrev.com/b819869839848e8d4f5488fb9f3ff5bed254fb26 Cr-Commit-Position: refs/heads/master@{#376125} ========== |
