Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1781043002:
media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses (Closed)
Descriptionmedia::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses
This is needed for https://crrev.com/1599533003/,
which introduces an HtmlAudioCapturerSource to
_copy_ the audio being played in a WebMediaPlayerImpl
and send it into a MediaStream.
On ToT, WASPImpl routes audio buses from |renderer_|
to a RestartableAudioRendererSink passed on ctor (this
is a "content" audio sink). This routing happens away
from WASPImpl, i.e. they |renderer_| and |sink_| talk
directly to each other.
This CL basically forces all audio to pass via an internal
class to WASPImpl so a client can register a callback to
get a copy of the data.
(The idea of calling such class TeeFilter comes from
e.g. [1])
BUG=569976, 575492
TEST= added new unit test entry, and in the mentioned dependent CL
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx
Committed: https://crrev.com/3c7a35d354e0ecf10fba26822ea833d78d132285
Cr-Commit-Position: refs/heads/master@{#380989}
Patch Set 1 #
Total comments: 8
Patch Set 2 : dalecurtis@ comments. Added inner class for Audio tapping: TeeFilter. #
Total comments: 9
Patch Set 3 : dalecurtis@ comments #
Total comments: 4
Patch Set 4 : last round of comments from dalecurtis@ #
Messages
Total messages: 22 (10 generated)
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@ please fwd appropriately or PTAL.
Description was changed from ========== media::WebAudioSourceProviderImpl, add listener to get a copy of passing AudioBuses This is needed for https://codereview.chromium.org/1599533003/, which introduces an HtmlAudioCapturerSource to capture audio being played in a WebMediaPlayerImpl and send it into a MediaStream. BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL ========== to ========== media::WebAudioSourceProviderImpl, add listener to get a copy of passing AudioBuses This is needed for https://codereview.chromium.org/1599533003/, which introduces an HtmlAudioCapturerSource to capture audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via WASPImpl so a registered OnAudioBusCB can get a copy of the data if registered. BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL ==========
mcasas@chromium.org changed reviewers: + rtoy@chromium.org
rtoy@ PTAL as well
https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:118: const int frames = renderer_->Render(bus_wrapper_.get(), 0, 0); What do you expect to happen in this case? https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:195: DCHECK(!frames_skipped); Is this reasonable? https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.h:43: NON_EXPORTED_BASE(public AudioRendererSink::RenderCallback) { Instead of inheriting from this can you create a new RenderCallbackCopier (naming up to you) which WASPI can own that implements the RenderCallback interface? This allows to prevent further inheritance bloat by this class. https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.h:45: using OnAudioBusCB = OnAudioBusCB is a bit specific. How about something like CopyAudioCB? This also needs the sample rate of the audio at least. Probably you'll want the channel layout too, but could add that later.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== media::WebAudioSourceProviderImpl, add listener to get a copy of passing AudioBuses This is needed for https://codereview.chromium.org/1599533003/, which introduces an HtmlAudioCapturerSource to capture audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via WASPImpl so a registered OnAudioBusCB can get a copy of the data if registered. BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL ========== to ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL ==========
Description was changed from ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL ========== to ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. (The idea of calling such class TeeFilter comes from e.g. [1]) BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx ==========
Dale PTAL. https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:118: const int frames = renderer_->Render(bus_wrapper_.get(), 0, 0); On 2016/03/11 01:41:42, DaleCurtis wrote: > What do you expect to happen in this case? Done. https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:195: DCHECK(!frames_skipped); On 2016/03/11 01:41:42, DaleCurtis wrote: > Is this reasonable? Prob remainder of some test. Removed. https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.h:43: NON_EXPORTED_BASE(public AudioRendererSink::RenderCallback) { On 2016/03/11 01:41:42, DaleCurtis wrote: > Instead of inheriting from this can you create a new RenderCallbackCopier > (naming up to you) which WASPI can own that implements the RenderCallback > interface? This allows to prevent further inheritance bloat by this class. I think is messier but done. Called the inner thing TeeFilter which is quite understandable, otherwise please suggest. https://codereview.chromium.org/1781043002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.h:45: using OnAudioBusCB = On 2016/03/11 01:41:42, DaleCurtis wrote: > OnAudioBusCB is a bit specific. How about something like CopyAudioCB? This also > needs the sample rate of the audio at least. Done > Probably you'll want the channellayout too, but could add that later. Why wait? Now that I've got some review time I'd rather add that too ;)
https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:116: // Note: using always |set_format_cb_| ensures we have the same locking You flipped using and always? Intentional? Doesn't make sense now. https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:27: // WebAudioSourceProviderImpl is either one of two things (but not both): Is there anything preventing WebAudio from being activated when a mediastream is attached? The comment says it can't happen, but I don't see how it couldn't. https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:47: int channels, Channel count isn't what's necessary, it's the channel layout. You can get the channel count from the audio_bus. https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:70: void ResetCopyAudioCallback(); Clear?
PTAL https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:116: // Note: using always |set_format_cb_| ensures we have the same locking On 2016/03/11 18:45:59, DaleCurtis wrote: > You flipped using and always? Intentional? Doesn't make sense now. Done. https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:27: // WebAudioSourceProviderImpl is either one of two things (but not both): On 2016/03/11 18:45:59, DaleCurtis wrote: > Is there anything preventing WebAudio from being activated when a mediastream is > attached? The comment says it can't happen, but I don't see how it couldn't. Done. https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:47: int channels, On 2016/03/11 18:45:59, DaleCurtis wrote: > Channel count isn't what's necessary, it's the channel layout. You can get the > channel count from the audio_bus. Done. Channel layout can be guessed from channel count right? Using GuessChannelLayout(int channels) https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:70: void ResetCopyAudioCallback(); On 2016/03/11 18:45:59, DaleCurtis wrote: > Clear? Done.
https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/40001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:47: int channels, On 2016/03/11 19:40:32, mcasas wrote: > On 2016/03/11 18:45:59, DaleCurtis wrote: > > Channel count isn't what's necessary, it's the channel layout. You can get the > > channel count from the audio_bus. > > Done. > > Channel layout can be guessed from channel > count right? Using > GuessChannelLayout(int channels) No, some layouts will have the same channel count. It's rare, but does happen. Guessing should only be done when the layout information isn't provided. https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:56: TeeFilter(AudioRendererSink::RenderCallback* const renderer, RenderCallback* const doesn't mean anything as a parameter. I.e drop const. https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:68: void RegisterCopyAudioCallback(const CopyAudioCB& callback); s/Register/Set/ since there can only be one.
lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1781043002/#ps80001 (title: "last round of comments from dalecurtis@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1781043002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1781043002/80001
Message was sent while issue was closed.
Description was changed from ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. (The idea of calling such class TeeFilter comes from e.g. [1]) BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx ========== to ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. (The idea of calling such class TeeFilter comes from e.g. [1]) BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. (The idea of calling such class TeeFilter comes from e.g. [1]) BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx ========== to ========== media::WebAudioSourceProviderImpl, add support for getting a copy of passing AudioBuses This is needed for https://crrev.com/1599533003/, which introduces an HtmlAudioCapturerSource to _copy_ the audio being played in a WebMediaPlayerImpl and send it into a MediaStream. On ToT, WASPImpl routes audio buses from |renderer_| to a RestartableAudioRendererSink passed on ctor (this is a "content" audio sink). This routing happens away from WASPImpl, i.e. they |renderer_| and |sink_| talk directly to each other. This CL basically forces all audio to pass via an internal class to WASPImpl so a client can register a callback to get a copy of the data. (The idea of calling such class TeeFilter comes from e.g. [1]) BUG=569976, 575492 TEST= added new unit test entry, and in the mentioned dependent CL [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd377612(v=vs.85).aspx Committed: https://crrev.com/3c7a35d354e0ecf10fba26822ea833d78d132285 Cr-Commit-Position: refs/heads/master@{#380989} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3c7a35d354e0ecf10fba26822ea833d78d132285 Cr-Commit-Position: refs/heads/master@{#380989}
Message was sent while issue was closed.
old "Done"s. https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:56: TeeFilter(AudioRendererSink::RenderCallback* const renderer, On 2016/03/12 03:24:42, DaleCurtis wrote: > RenderCallback* const doesn't mean anything as a parameter. I.e drop const. Done. https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1781043002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:68: void RegisterCopyAudioCallback(const CopyAudioCB& callback); On 2016/03/12 03:24:42, DaleCurtis wrote: > s/Register/Set/ since there can only be one. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
