Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1599533003:
MediaCaptureFromElement: add support for audio captureStream(). (Closed)
|
Created:
4 years, 11 months ago by mcasas Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaCaptureFromElement: add support for audio captureStream().
This CL extends support for capturing the audio part of
a <video> or <audio> tags ( "capture" here means creating
a MediaStream out of the HTMLElement)
It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource
wrapped into an ExternalMediaStreamAudioSource to produce data
towards the audio track.
HtmlAudioCapturerSource also plugs into the
WebMediaPlayer's WebAudioSourceProviderImpl to get
a copy of the audio being rendered.
Unit tests are added, and the existing LayouTests
revamped (and split into several files for clarity).
BUG=569976, 575492
TEST= run chromium with
--enable-blink-features=MediaCaptureFromVideo
against e.g.
https://rawgit.com/Miguelao/demos/master/videoelementcapture.html
Committed: https://crrev.com/77d0d446e58afbf7fab215113fcf9fe9c97e94e3
Cr-Commit-Position: refs/heads/master@{#395205}
Patch Set 1 : #
Total comments: 30
Patch Set 2 : miu@s comments, including using AudioPushFifo #Patch Set 3 : Changed HTMLAudioElementCapturerSource from AudioCapturerSource to MediaStreamAudioSource #
Total comments: 6
Patch Set 4 : miu@s comments #
Total comments: 2
Patch Set 5 : avi@ comment #
Total comments: 2
Patch Set 6 : using refptr iso WeakPtr #Messages
Total messages: 68 (47 generated)
Description was changed from ========== git cl i# Enter a description of the change. ble BUG= ========== to ========== MediaCaptureFromElement: add support for audio captureStream(). ** WIP ** BUG=569976, 575492 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Description was changed from ========== MediaCaptureFromElement: add support for audio captureStream(). ** WIP ** BUG=569976, 575492 ========== to ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags. This "capture" means creating a MediaStream out of the HTMLElement. It introduces a HtmlAudioCapturerSource that is wrapped into a Media Stream Source. This one plugs as a listener in <to be determined, AudioRendererMixerInput?> and gets a copy of the audio being rendered. BUG=569976, 575492 ==========
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Description was changed from ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags. This "capture" means creating a MediaStream out of the HTMLElement. It introduces a HtmlAudioCapturerSource that is wrapped into a Media Stream Source. This one plugs as a listener in <to be determined, AudioRendererMixerInput?> and gets a copy of the audio being rendered. BUG=569976, 575492 ========== to ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags. This "capture" means creating a MediaStream out of the HTMLElement. It introduces a HtmlAudioCapturerSource that is wrapped into a Media Stream Source. This one plugs as a listener in <to be determined, AudioRendererMixerInput?> and gets a copy of the audio being rendered. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromElement against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ==========
Patchset #2 (id:200001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:260001) has been deleted
Description was changed from ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags. This "capture" means creating a MediaStream out of the HTMLElement. It introduces a HtmlAudioCapturerSource that is wrapped into a Media Stream Source. This one plugs as a listener in <to be determined, AudioRendererMixerInput?> and gets a copy of the audio being rendered. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromElement against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ========== to ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource to be wrapped into a MediaStreamAudioSource. This one plugs as a listener in the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. WebAudioSourceProviderImpl routes audio to either WebAudio or to AudioRendererMixer using different paths: this CL makes both use the AudioRendererSink::RenderCallback interface. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromElement against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ==========
Patchset #1 (id:280001) has been deleted
Description was changed from ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource to be wrapped into a MediaStreamAudioSource. This one plugs as a listener in the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. WebAudioSourceProviderImpl routes audio to either WebAudio or to AudioRendererMixer using different paths: this CL makes both use the AudioRendererSink::RenderCallback interface. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromElement against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ========== to ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource to be wrapped into a MediaStreamAudioSource. This one plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ==========
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
Patchset #1 (id:420001) has been deleted
Patchset #1 (id:440001) has been deleted
Patchset #1 (id:460001) has been deleted
Patchset #1 (id:480001) has been deleted
Patchset #1 (id:500001) has been deleted
Patchset #1 (id:520001) has been deleted
Description was changed from ========== [WIP] MediaCaptureFromElement: add support for audio captureStream(). *** very experimental *** This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource to be wrapped into a MediaStreamAudioSource. This one plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ========== to ========== MediaCaptureFromElement: add support for audio captureStream(). This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource wrapped into an ExternalMediaStreamAudioSource to produce data towards the audio track. HtmlAudioCapturerSource also plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. Unit tests are added, and the existing LayouTests revamped (and split into several files for clarity). BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ==========
mcasas@chromium.org changed reviewers: + miu@chromium.org
miu@ PTAL ( working now thanks to https://crrev.com/1834323002/ yay! )
Comments on Patch Set 1: https://codereview.chromium.org/1599533003/diff/540001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_render... content/content_renderer.gypi:653: 'renderer/media/html_audio_element_capturer_source.cc', Seems like none of the code in these new files references any third-party/webrtc or libjingle code. So, please move these to the 'private_renderer_sources' list. https://codereview.chromium.org/1599533003/diff/540001/content/content_tests.... File content/content_tests.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_tests.... content/content_tests.gypi:784: 'renderer/media/html_audio_element_capturer_source_unittest.cc', ditto: Please move to 'content_unittests_sources' list. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... File content/renderer/media/html_audio_element_capturer_source.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:8: #include "base/timer/elapsed_timer.h" Seems this is not used. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:20: static const size_t kMaxNumberOfBuffers = 10; This can be deleted if you agree w/ my advice about using AudioPushFifo instead. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:58: audio_source_->SetCopyAudioCallback(audio_input_cb_); By creating the closure in the ctor, HtmlAudioElementCapturerSource will hold a ref-count to itself forever; and therefore it will never be destroyed. Instead, can you just eliminate |audio_input_cb_|, and do the base::Bind(...) here? https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:100: fifo_->Push(audio_bus.get()); All this code from here to the end of this method would be greatly simplified if you use media::AudioPushFifo instead (I wrote it so we wouldn't have to keep writing this code over and over). It even manages the audio delay calculation for you, and avoids unnecessary copying when input_params==output_params. Example usage: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... For this case, you'd construct the AudioPushFifo to callback a method that looks like: void DeliverRebufferedAudio( const media::AudioBus& audio_bus, int frame_delay) { converter_input_bus_ = &audio_bus; // ProvideInput() reads from this. if (!converter_output_bus_) { converter_output_bus_ = media::AudioBus::Create(...); } const int output_block_delay = frame_delay * MillisPerSecond / input_sample_rate; converter_->Convert(converter_output_bus_.get(), output_block_delay, 1.0, false); DCHECK(!converter_input_bus_); // Cleared by ProvideInput(). } And ProvideInput() becomes: void ProvideInput(media::AudioBus* audio_bus, ...) { DCHECK(converter_input_bus_); converter_input_bus_->CopyTo(audio_bus); converter_input_bus_ = nullptr; return 1.0; } And somewhere prior to all this: fifo_.Reset(min_fifo_frames_for_conversion_); https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... File content/renderer/media/html_audio_element_capturer_source.h (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.h:57: void OnAudioBus(std::unique_ptr<media::AudioBus> audio_bus, Seems like this should be a private method since it should only be called via the internally-created callback. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.h:62: double ProvideInput(media::AudioBus* audio_bus, Looks like this is for exclusive use by |converter_|, so consider making it a private method too. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:982: #if defined(ENABLE_WEBRTC) Consider omitting the "#if defined(ENABLE_WEBRTC)" since this should all compile and run w/o webrtc dependencies. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:987: const int kInputNumChannels = 1; Actually, this is the *output* format (from ExternalMediaStreamAudioSource). It will call HtmlAudioElementCapturerSource::Initialize() with these params. So, you can make this whatever format you would want the MediaStreamTrack to provide. Also, note that you could eliminate the need for any audio conversion if you just used the audio format of the HTMLMediaElement. It feels like it should be easy to pass that information to this method. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:991: AddAudioTrackToMediaStream( Nice! :) https://codereview.chromium.org/1599533003/diff/540001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/540001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:72: virtual void SetCopyAudioCallback(const CopyAudioCB& callback); Why are these methods being made virtual? https://codereview.chromium.org/1599533003/diff/540001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-EME-content.html (right): https://codereview.chromium.org/1599533003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-EME-content.html:11: const onEncrypted = this.step_func_done(); I didn't know JavaScript had const! Cool! :) However, since you only reference this once, on line 15, can you delete this line and make line 15: video.onencrypted = this.step_func_done();
Patchset #2 (id:560001) has been deleted
miu@ PTAL https://codereview.chromium.org/1599533003/diff/540001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_render... content/content_renderer.gypi:653: 'renderer/media/html_audio_element_capturer_source.cc', On 2016/05/13 23:40:35, miu wrote: > Seems like none of the code in these new files references any third-party/webrtc > or libjingle code. So, please move these to the 'private_renderer_sources' list. Please see my reply in renderer_blink_platform_impl.cc https://codereview.chromium.org/1599533003/diff/540001/content/content_tests.... File content/content_tests.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_tests.... content/content_tests.gypi:784: 'renderer/media/html_audio_element_capturer_source_unittest.cc', On 2016/05/13 23:40:35, miu wrote: > ditto: Please move to 'content_unittests_sources' list. Yeah, please see my reply in renderer_blink_platform_impl.cc https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... File content/renderer/media/html_audio_element_capturer_source.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:8: #include "base/timer/elapsed_timer.h" On 2016/05/13 23:40:35, miu wrote: > Seems this is not used. Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:20: static const size_t kMaxNumberOfBuffers = 10; On 2016/05/13 23:40:35, miu wrote: > This can be deleted if you agree w/ my advice about using AudioPushFifo instead. Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:58: audio_source_->SetCopyAudioCallback(audio_input_cb_); On 2016/05/13 23:40:35, miu wrote: > By creating the closure in the ctor, HtmlAudioElementCapturerSource will hold a > ref-count to itself forever; and therefore it will never be destroyed. > > Instead, can you just eliminate |audio_input_cb_|, and do the base::Bind(...) > here? Absolutely! Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.cc:100: fifo_->Push(audio_bus.get()); On 2016/05/13 23:40:35, miu wrote: > All this code from here to the end of this method would be greatly simplified if > you use media::AudioPushFifo instead (I wrote it so we wouldn't have to keep > writing this code over and over). It even manages the audio delay calculation > for you, and avoids unnecessary copying when input_params==output_params. > > Example usage: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > For this case, you'd construct the AudioPushFifo to callback a method that looks > like: > > void DeliverRebufferedAudio( > const media::AudioBus& audio_bus, > int frame_delay) { > converter_input_bus_ = &audio_bus; // ProvideInput() reads from this. > if (!converter_output_bus_) { > converter_output_bus_ = media::AudioBus::Create(...); > } > const int output_block_delay = frame_delay * MillisPerSecond / > input_sample_rate; > converter_->Convert(converter_output_bus_.get(), output_block_delay, 1.0, > false); > DCHECK(!converter_input_bus_); // Cleared by ProvideInput(). > } > > And ProvideInput() becomes: > > void ProvideInput(media::AudioBus* audio_bus, ...) { > DCHECK(converter_input_bus_); > converter_input_bus_->CopyTo(audio_bus); > converter_input_bus_ = nullptr; > return 1.0; > } > > And somewhere prior to all this: > > fifo_.Reset(min_fifo_frames_for_conversion_); Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... File content/renderer/media/html_audio_element_capturer_source.h (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.h:57: void OnAudioBus(std::unique_ptr<media::AudioBus> audio_bus, On 2016/05/13 23:40:35, miu wrote: > Seems like this should be a private method since it should only be called via > the internally-created callback. Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/media... content/renderer/media/html_audio_element_capturer_source.h:62: double ProvideInput(media::AudioBus* audio_bus, On 2016/05/13 23:40:35, miu wrote: > Looks like this is for exclusive use by |converter_|, so consider making it a > private method too. Done. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:982: #if defined(ENABLE_WEBRTC) On 2016/05/13 23:40:35, miu wrote: > Consider omitting the "#if defined(ENABLE_WEBRTC)" since this should all compile > and run w/o webrtc dependencies. It should be like that indeed, but by extension (and history), all MediaStream and GetUserMedia APIs and code are considered behind this WebRtc-grouping. IOW, if a (possibly, downstream) build has no WebRtc, it would have no MediaStreams, hence no use for this API. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:987: const int kInputNumChannels = 1; On 2016/05/13 23:40:35, miu wrote: > Actually, this is the *output* format (from ExternalMediaStreamAudioSource). It > will call HtmlAudioElementCapturerSource::Initialize() with these params. So, > you can make this whatever format you would want the MediaStreamTrack to > provide. Yeah, |output| from the Source but |input| from the Track POV, and since we're adding a Track in l.991, I thought this naming was more representative. > > Also, note that you could eliminate the need for any audio conversion if you > just used the audio format of the HTMLMediaElement. It feels like it should be > easy to pass that information to this method. Turns out it's actually quite hard since is buried in the pipeline decoding the actual data, and depending on the video, the decoding library, and the network, it can take a while before we actually could get these parameters, even up to the time of the first frame arrival... https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:991: AddAudioTrackToMediaStream( On 2016/05/13 23:40:35, miu wrote: > Nice! :) Acknowledged. https://codereview.chromium.org/1599533003/diff/540001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/540001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:72: virtual void SetCopyAudioCallback(const CopyAudioCB& callback); On 2016/05/13 23:40:35, miu wrote: > Why are these methods being made virtual? For MockWebAudioSourceProvider in html_audio_element_capturer_unittest.cc https://codereview.chromium.org/1599533003/diff/540001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-EME-content.html (right): https://codereview.chromium.org/1599533003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-EME-content.html:11: const onEncrypted = this.step_func_done(); On 2016/05/13 23:40:35, miu wrote: > I didn't know JavaScript had const! Cool! :) > const-ness rules! > However, since you only reference this once, on line 15, can you delete this > line and make line 15: > > video.onencrypted = this.step_func_done(); Done.
Patchset #4 (id:620001) has been deleted
Patchset #3 (id:600001) has been deleted
Patchset #3 (id:640001) has been deleted
Looks good. Unfortunately, I did not realize earlier that there is a much simpler way to accomplish what you're doing (2nd comment below). Comments on Patch Set 3: https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:982: #if defined(ENABLE_WEBRTC) On 2016/05/14 02:23:47, mcasas wrote: > On 2016/05/13 23:40:35, miu wrote: > > Consider omitting the "#if defined(ENABLE_WEBRTC)" since this should all > compile > > and run w/o webrtc dependencies. > > It should be like that indeed, but by > extension (and history), all MediaStream > and GetUserMedia APIs and code are considered > behind this WebRtc-grouping. This may have been true in the past, but not anymore. For example, in content/content_renderer.gypi, there are media_stream_*.* files listed in both 'public_renderer_sources' and 'private_renderer_sources'; both of which are built even when ENABLE_WEBRTC is false. Historically, the MediaStream API was designed and built at a time when WebRTC was the only game in town. That's why there's all this code that mixes-in WebRTC dependencies and conditionally compiles around ENABLE_WEBRTC. Nowadays, there are multiple other non-WebRTC product features that rely on it (e.g., Cast Streaming, WebAudio, etc.). So, it's a general TODO/FixIt issue in the content/renderer/media code to start enabling more of the MediaStream code (the modules without WebRTC dependencies), moving their compile/link outside of the protected "ENABLE_WEBRTC" realm so that the code can be re-used by other product features. OOC, I tried building your patch locally with GN arg 'enable_webrtc=false'. Everything compiled, except for one issue: Linking failed on the call to AddAudioTrackToMediaStream() (line 991 in this change). It seems that's only because media_stream_utils.cc is not in 'public_renderer_sources', which is an oversight of mine. It should be moved since my recent "big 8000 line change" removed the last of the WebRTC dependencies from the media_stream_utils functions. However, this is all a moot point: Take a look at my comment below, and you *definitely* won't need to conditionally compile this code via "#ifdef ENABLE_WEBRTC" anymore. Generally, I would suggest that if code can compile and link successfully when ENABLE_WEBRTC is false, you shouldn't add this guard. It's self-documenting code for a problem that doesn't actually exist, which can mislead future maintainers. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:987: const int kInputNumChannels = 1; On 2016/05/14 02:23:47, mcasas wrote: > On 2016/05/13 23:40:35, miu wrote: > > Also, note that you could eliminate the need for any audio conversion if you > > just used the audio format of the HTMLMediaElement. It feels like it should be > > easy to pass that information to this method. > > Turns out it's actually quite hard since is buried in > the pipeline decoding the actual data, and depending > on the video, the decoding library, and the network, > it can take a while before we actually could get these > parameters, even up to the time of the first frame > arrival... Oh! I don't know why I didn't notice this before: You have HtmlAudioElementCapturerSource extending media::AudioCapturerSource. That's requiring you to fix the output audio format ahead of time and have to convert whatever happens to come in from the WebMediaPlayerImpl. Instead, what you want to do is have HtmlAudioElementCapturerSource extend MediaStreamAudioSource (i.e., don't use ExternalMediaStreamAudioSource, but write your own). The format of the audio can then change at any time. Also, the whole class becomes much simpler: class CONTENT_EXPORT HtmlAudioElementSource final : NON_EXPORTED_BASE(public MediaStreamAudioSource) { public: ...ctor... ~HtmlAudioElementSource() final { EnsureSourceIsStopped(); } bool EnsureSourceIsStarted() final { if (audio_source_ && !is_started_) { audio_source_->SetCopyAudioCallback( base::Bind(OnAudioBus, this)); is_started_ = true; } return is_started_; } void EnsureSourceIsStopped() final { if (is_started_) { if (audio_source_) { audio_source_->ClearCopyAudioCallback(); audio_source_.reset(); } is_started_ = false; } } void OnAudioBus(std::unique_ptr<media::AudioBus> audio_bus, uint32_t delay_milliseconds, int sample_rate) { const base::TimeTicks capture_time = base::TimeTicks::Now() - base::TimeDelta::FromMilliseconds(delay_milliseconds); if (sample_rate != last_sample_rate_ || audio_bus->channels() != last_num_channels_ || audio_bus->frames() != last_bus_frames_) { MediaStreamAudioSource::SetFormat(media::AudioParameters( media::AudioParameters::AUDIO_PCM_LOW_LATENCY, media::GuessChannelLayout(audio_bus->channels()), sample_rate, 16, audio_bus->frames())); last_sample_rate_ = sample_rate; last_num_channels_ = audio_bus->channels(); last_bus_frames_ = audio_bus->frames(); } MediaStreamAudioSource::DeliverDataToTracks(*audio_bus_, capture_time); } base::WeakPtr<media::WebAudioSourceProviderImpl> audio_source_; bool is_started_ = false; base::ThreadChecker thread_checker_; DISALLOW_COPY_AND_ASSIGN(HtmlAudioElementSource); };
Patchset #3 (id:660001) has been deleted
Patchset #3 (id:680001) has been deleted
miu@, followed your advice and it looks now orders of magnitude better, PTAL (also moved the files in the .gyp(i)s ) https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:982: #if defined(ENABLE_WEBRTC) On 2016/05/17 21:00:41, miu wrote: > On 2016/05/14 02:23:47, mcasas wrote: > > On 2016/05/13 23:40:35, miu wrote: > > > Consider omitting the "#if defined(ENABLE_WEBRTC)" since this should all > > compile > > > and run w/o webrtc dependencies. > > > > It should be like that indeed, but by > > extension (and history), all MediaStream > > and GetUserMedia APIs and code are considered > > behind this WebRtc-grouping. > > This may have been true in the past, but not anymore. For example, in > content/content_renderer.gypi, there are media_stream_*.* files listed in both > 'public_renderer_sources' and 'private_renderer_sources'; both of which are > built even when ENABLE_WEBRTC is false. > > Historically, the MediaStream API was designed and built at a time when WebRTC > was the only game in town. That's why there's all this code that mixes-in WebRTC > dependencies and conditionally compiles around ENABLE_WEBRTC. Nowadays, there > are multiple other non-WebRTC product features that rely on it (e.g., Cast > Streaming, WebAudio, etc.). So, it's a general TODO/FixIt issue in the > content/renderer/media code to start enabling more of the MediaStream code (the > modules without WebRTC dependencies), moving their compile/link outside of the > protected "ENABLE_WEBRTC" realm so that the code can be re-used by other product > features. > > OOC, I tried building your patch locally with GN arg 'enable_webrtc=false'. > Everything compiled, except for one issue: Linking failed on the call to > AddAudioTrackToMediaStream() (line 991 in this change). It seems that's only > because media_stream_utils.cc is not in 'public_renderer_sources', which is an > oversight of mine. It should be moved since my recent "big 8000 line change" > removed the last of the WebRTC dependencies from the media_stream_utils > functions. > > However, this is all a moot point: Take a look at my comment below, and you > *definitely* won't need to conditionally compile this code via "#ifdef > ENABLE_WEBRTC" anymore. > > Generally, I would suggest that if code can compile and link successfully when > ENABLE_WEBRTC is false, you shouldn't add this guard. It's self-documenting code > for a problem that doesn't actually exist, which can mislead future maintainers. Acknowledged. https://codereview.chromium.org/1599533003/diff/540001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:987: const int kInputNumChannels = 1; On 2016/05/17 21:00:41, miu wrote: > On 2016/05/14 02:23:47, mcasas wrote: > > On 2016/05/13 23:40:35, miu wrote: > > > Also, note that you could eliminate the need for any audio conversion if you > > > just used the audio format of the HTMLMediaElement. It feels like it should > be > > > easy to pass that information to this method. > > > > Turns out it's actually quite hard since is buried in > > the pipeline decoding the actual data, and depending > > on the video, the decoding library, and the network, > > it can take a while before we actually could get these > > parameters, even up to the time of the first frame > > arrival... > > Oh! I don't know why I didn't notice this before: You have > HtmlAudioElementCapturerSource extending media::AudioCapturerSource. That's > requiring you to fix the output audio format ahead of time and have to convert > whatever happens to come in from the WebMediaPlayerImpl. > > Instead, what you want to do is have HtmlAudioElementCapturerSource extend > MediaStreamAudioSource (i.e., don't use ExternalMediaStreamAudioSource, but > write your own). The format of the audio can then change at any time. Also, the > whole class becomes much simpler: > > class CONTENT_EXPORT HtmlAudioElementSource final > : NON_EXPORTED_BASE(public MediaStreamAudioSource) { > public: > ...ctor... > > ~HtmlAudioElementSource() final { > EnsureSourceIsStopped(); > } > > bool EnsureSourceIsStarted() final { > if (audio_source_ && !is_started_) { > audio_source_->SetCopyAudioCallback( > base::Bind(OnAudioBus, this)); > is_started_ = true; > } > return is_started_; > } > > void EnsureSourceIsStopped() final { > if (is_started_) { > if (audio_source_) { > audio_source_->ClearCopyAudioCallback(); > audio_source_.reset(); > } > is_started_ = false; > } > } > > void OnAudioBus(std::unique_ptr<media::AudioBus> audio_bus, > uint32_t delay_milliseconds, > int sample_rate) { > const base::TimeTicks capture_time = base::TimeTicks::Now() - > base::TimeDelta::FromMilliseconds(delay_milliseconds); > > if (sample_rate != last_sample_rate_ || > audio_bus->channels() != last_num_channels_ || > audio_bus->frames() != last_bus_frames_) { > MediaStreamAudioSource::SetFormat(media::AudioParameters( > media::AudioParameters::AUDIO_PCM_LOW_LATENCY, > media::GuessChannelLayout(audio_bus->channels()), > sample_rate, 16, audio_bus->frames())); > last_sample_rate_ = sample_rate; > last_num_channels_ = audio_bus->channels(); > last_bus_frames_ = audio_bus->frames(); > } > > MediaStreamAudioSource::DeliverDataToTracks(*audio_bus_, capture_time); > } > > base::WeakPtr<media::WebAudioSourceProviderImpl> audio_source_; > bool is_started_ = false; > > base::ThreadChecker thread_checker_; > > DISALLOW_COPY_AND_ASSIGN(HtmlAudioElementSource); > }; Brilliant! SO -oh- Done!
content/... and media/... lgtm, with minor considerations: https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:11: #include "base/guid.h" Is this new #include still needed? https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1004: web_media_stream->addTrack(web_media_stream_track); I'm not sure, but I think you *might* want to unconditionally add the track to the web_media_stream even if ConnectToTrack() returns false. The reason is that ConnectToTrack() will return false if the source couldn't be started, but it will also still create a MediaStreamAudioTrack and call web_media_stream_track.setExtraData(media_stream_audio_track). In this case, a MediaStreamTrack will be presented to the user JavaScript in the "ended" state. So, when the source cannot be started, do you want the user's JavaScript to see: 1) an "ended" track added to the MediaStream? Or, 2) no audio track added to the MediaStream? I suspect #1 would be more robust (and consistent with the other MediaStream specs). https://codereview.chromium.org/1599533003/diff/700001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/700001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:79: int RenderForTesting(AudioBus* audio_bus); Not sure you should have made this public. Instead, could your test code just call provideInput()?
mcasas@chromium.org changed reviewers: + dalecurtis@chromium.org, haraken@chromium.org
dalecurtis@ PTAL at minor changes in webaudiosourceprovider_impl.* haraken@ PTAL at the WebKit changes (splitting/beefing up LayoutTests; sadly extending platform) https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:11: #include "base/guid.h" On 2016/05/19 22:44:30, miu wrote: > Is this new #include still needed? Yes, for l.988 const WebString track_id = WebString::fromUTF8(base::GenerateGUID()); https://codereview.chromium.org/1599533003/diff/700001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1004: web_media_stream->addTrack(web_media_stream_track); On 2016/05/19 22:44:30, miu wrote: > I'm not sure, but I think you *might* want to unconditionally add the track to > the web_media_stream even if ConnectToTrack() returns false. The reason is that > ConnectToTrack() will return false if the source couldn't be started, but it > will also still create a MediaStreamAudioTrack and call > web_media_stream_track.setExtraData(media_stream_audio_track). In this case, a > MediaStreamTrack will be presented to the user JavaScript in the "ended" state. > > So, when the source cannot be started, do you want the user's JavaScript to see: > 1) an "ended" track added to the MediaStream? Or, 2) no audio track added to > the MediaStream? I suspect #1 would be more robust (and consistent with the > other MediaStream specs). Makes sense, done. https://codereview.chromium.org/1599533003/diff/700001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/700001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:79: int RenderForTesting(AudioBus* audio_bus); On 2016/05/19 22:44:30, miu wrote: > Not sure you should have made this public. Instead, could your test code just > call provideInput()? provideInput() is only available for the render-to-WebAudio path: WASPImpl can only pass audio onwards to either |renderer_| or |client_|, but not both. provideAudio() is used by the second path (hence the check in [1]). I could modify the unittests to use this particular path, but IMHO is not worth it. (Also I can't easily make me friend of this class because HTMLAudioElementCapturerSourceTest is in content/, argh!). [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webaud...
mcasas@chromium.org changed reviewers: + avi@chromium.org
avi@ PTAL at renderer_blink_platform_impl changes
WebKit/Source/ LGTM
On 2016/05/19 23:59:23, mcasas wrote: > avi@ PTAL at renderer_blink_platform_impl changes LGTM, though I'm struck at how different the implementations of createHTMLAudioElementCapturer and createHTMLVideoElementCapturer are, even though I imagine they should be similar. https://codereview.chromium.org/1599533003/diff/720001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/720001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1000: // Takes ownership of|media_stream_source|. Space before "|media".
dalecurtis@ PTAL at WASPImpl minichanges https://codereview.chromium.org/1599533003/diff/720001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/720001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1000: // Takes ownership of|media_stream_source|. On 2016/05/20 01:59:07, Avi wrote: > Space before "|media". Done.
https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:75: base::WeakPtr<WebAudioSourceProviderImpl> AsWeakPtr() { This is not right, the class is already ref-counted, the internal weak factory should not be exposed to external callers. Probably instead of even having a WeakFactory the one use for the set_format_cb_ should just be a cancellable closure too.
dalecurtis@ PTAL. https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.h:75: base::WeakPtr<WebAudioSourceProviderImpl> AsWeakPtr() { On 2016/05/20 18:48:56, DaleCurtis wrote: > This is not right, the class is already ref-counted, the internal weak factory > should not be exposed to external callers. Probably instead of even having a > WeakFactory the one use for the set_format_cb_ should just be a cancellable > closure too. Using scoped_refptr<>.
mcasas@chromium.org changed reviewers: + esprehn@chromium.org
esprehn@ PTAL at Platform.h one line addition.
media/wasp lgtm
lgtm, what's the plan for moving this stuff into blink platform and out of content? This goes against the onion soup plan of making all the plumbing out of platform smaller. :)
On 2016/05/20 22:32:28, esprehn wrote: > lgtm, what's the plan for moving this stuff into blink platform and out of > content? This goes against the onion soup plan of making all the plumbing out of > platform smaller. :) esprehn@ IPC migration spreadsheet [1] lists, sadly, content/renderer/media/ as available, so I guess there's no plan. I too believe all this audio piping could and should be done inside Blink. [1] https://goo.gl/809bwy
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, haraken@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1599533003/#ps760001 (title: "using refptr iso WeakPtr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1599533003/760001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1599533003/760001
Message was sent while issue was closed.
Description was changed from ========== MediaCaptureFromElement: add support for audio captureStream(). This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource wrapped into an ExternalMediaStreamAudioSource to produce data towards the audio track. HtmlAudioCapturerSource also plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. Unit tests are added, and the existing LayouTests revamped (and split into several files for clarity). BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ========== to ========== MediaCaptureFromElement: add support for audio captureStream(). This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource wrapped into an ExternalMediaStreamAudioSource to produce data towards the audio track. HtmlAudioCapturerSource also plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. Unit tests are added, and the existing LayouTests revamped (and split into several files for clarity). BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ==========
Message was sent while issue was closed.
Committed patchset #6 (id:760001)
Message was sent while issue was closed.
Description was changed from ========== MediaCaptureFromElement: add support for audio captureStream(). This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource wrapped into an ExternalMediaStreamAudioSource to produce data towards the audio track. HtmlAudioCapturerSource also plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. Unit tests are added, and the existing LayouTests revamped (and split into several files for clarity). BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html ========== to ========== MediaCaptureFromElement: add support for audio captureStream(). This CL extends support for capturing the audio part of a <video> or <audio> tags ( "capture" here means creating a MediaStream out of the HTMLElement) It introduces an HtmlAudioCapturerSource is-a AudioCapturerSource wrapped into an ExternalMediaStreamAudioSource to produce data towards the audio track. HtmlAudioCapturerSource also plugs into the WebMediaPlayer's WebAudioSourceProviderImpl to get a copy of the audio being rendered. Unit tests are added, and the existing LayouTests revamped (and split into several files for clarity). BUG=569976, 575492 TEST= run chromium with --enable-blink-features=MediaCaptureFromVideo against e.g. https://rawgit.com/Miguelao/demos/master/videoelementcapture.html Committed: https://crrev.com/77d0d446e58afbf7fab215113fcf9fe9c97e94e3 Cr-Commit-Position: refs/heads/master@{#395205} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/77d0d446e58afbf7fab215113fcf9fe9c97e94e3 Cr-Commit-Position: refs/heads/master@{#395205}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:760001) has been created in https://codereview.chromium.org/2007433002/ by sigbjornf@opera.com. The reason for reverting is: The layout tests added are flakily crashing on various bots, https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6.... |
