Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 1599533003: MediaCaptureFromElement: add support for audio captureStream(). (Closed)

Created:
3 years, 4 months ago by mcasas
Modified:
2 years, 12 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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+445 lines, -54 lines) Patch
M content/content_renderer.gypi View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A content/renderer/media/html_audio_element_capturer_source.h View 1 2 3 4 5 1 chunk +63 lines, -0 lines 0 comments Download
A content/renderer/media/html_audio_element_capturer_source.cc View 1 2 3 4 5 1 chunk +89 lines, -0 lines 0 comments Download
A content/renderer/media/html_audio_element_capturer_source_unittest.cc View 1 2 3 4 5 1 chunk +154 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 1 2 3 4 3 chunks +29 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M media/blink/webaudiosourceprovider_impl.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M media/blink/webaudiosourceprovider_impl.cc View 1 2 3 chunks +8 lines, -4 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-EME-content.html View 1 1 chunk +28 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-capture.html View 1 chunk +35 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-creation.html View 2 chunks +22 lines, -40 lines 0 comments Download
M third_party/WebKit/Source/modules/mediacapturefromelement/HTMLMediaElementCapture.cpp View 2 chunks +6 lines, -7 lines 0 comments Download
M third_party/WebKit/public/platform/Platform.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 68 (47 generated)
mcasas
miu@ PTAL ( working now thanks to https://crrev.com/1834323002/ yay! )
3 years ago (2016-05-13 18:58:24 UTC) #35
miu
Comments on Patch Set 1: https://codereview.chromium.org/1599533003/diff/540001/content/content_renderer.gypi File content/content_renderer.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_renderer.gypi#newcode653 content/content_renderer.gypi:653: 'renderer/media/html_audio_element_capturer_source.cc', Seems like none ...
3 years ago (2016-05-13 23:40:35 UTC) #36
mcasas
miu@ PTAL https://codereview.chromium.org/1599533003/diff/540001/content/content_renderer.gypi File content/content_renderer.gypi (right): https://codereview.chromium.org/1599533003/diff/540001/content/content_renderer.gypi#newcode653 content/content_renderer.gypi:653: 'renderer/media/html_audio_element_capturer_source.cc', On 2016/05/13 23:40:35, miu wrote: > ...
3 years ago (2016-05-14 02:23:47 UTC) #38
miu
Looks good. Unfortunately, I did not realize earlier that there is a much simpler way ...
3 years ago (2016-05-17 21:00:42 UTC) #42
mcasas
miu@, followed your advice and it looks now orders of magnitude better, PTAL (also moved ...
3 years ago (2016-05-18 00:00:11 UTC) #45
miu
content/... and media/... lgtm, with minor considerations: https://codereview.chromium.org/1599533003/diff/700001/content/renderer/renderer_blink_platform_impl.cc File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/700001/content/renderer/renderer_blink_platform_impl.cc#newcode11 content/renderer/renderer_blink_platform_impl.cc:11: #include "base/guid.h" ...
3 years ago (2016-05-19 22:44:30 UTC) #46
mcasas
dalecurtis@ PTAL at minor changes in webaudiosourceprovider_impl.* haraken@ PTAL at the WebKit changes (splitting/beefing up ...
3 years ago (2016-05-19 23:57:55 UTC) #48
mcasas
avi@ PTAL at renderer_blink_platform_impl changes
3 years ago (2016-05-19 23:59:23 UTC) #50
haraken
WebKit/Source/ LGTM
3 years ago (2016-05-20 00:31:43 UTC) #51
Avi (use Gerrit)
On 2016/05/19 23:59:23, mcasas wrote: > avi@ PTAL at renderer_blink_platform_impl changes LGTM, though I'm struck ...
3 years ago (2016-05-20 01:59:07 UTC) #52
mcasas
dalecurtis@ PTAL at WASPImpl minichanges https://codereview.chromium.org/1599533003/diff/720001/content/renderer/renderer_blink_platform_impl.cc File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1599533003/diff/720001/content/renderer/renderer_blink_platform_impl.cc#newcode1000 content/renderer/renderer_blink_platform_impl.cc:1000: // Takes ownership of|media_stream_source|. ...
3 years ago (2016-05-20 02:08:05 UTC) #53
DaleCurtis
https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudiosourceprovider_impl.h File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudiosourceprovider_impl.h#newcode75 media/blink/webaudiosourceprovider_impl.h:75: base::WeakPtr<WebAudioSourceProviderImpl> AsWeakPtr() { This is not right, the class ...
3 years ago (2016-05-20 18:48:56 UTC) #54
mcasas
dalecurtis@ PTAL. https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudiosourceprovider_impl.h File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/1599533003/diff/740001/media/blink/webaudiosourceprovider_impl.h#newcode75 media/blink/webaudiosourceprovider_impl.h:75: base::WeakPtr<WebAudioSourceProviderImpl> AsWeakPtr() { On 2016/05/20 18:48:56, DaleCurtis ...
2 years, 12 months ago (2016-05-20 21:39:59 UTC) #55
mcasas
esprehn@ PTAL at Platform.h one line addition.
2 years, 12 months ago (2016-05-20 21:48:00 UTC) #57
DaleCurtis
media/wasp lgtm
2 years, 12 months ago (2016-05-20 22:20:51 UTC) #58
esprehn
lgtm, what's the plan for moving this stuff into blink platform and out of content? ...
2 years, 12 months ago (2016-05-20 22:32:28 UTC) #59
mcasas
On 2016/05/20 22:32:28, esprehn wrote: > lgtm, what's the plan for moving this stuff into ...
2 years, 12 months ago (2016-05-20 22:54:06 UTC) #60
commit-bot: I haz the power
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
2 years, 12 months ago (2016-05-20 23:34:40 UTC) #63
commit-bot: I haz the power
Committed patchset #6 (id:760001)
2 years, 12 months ago (2016-05-20 23:39:31 UTC) #65
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/77d0d446e58afbf7fab215113fcf9fe9c97e94e3 Cr-Commit-Position: refs/heads/master@{#395205}
2 years, 12 months ago (2016-05-20 23:41:11 UTC) #67
sof
2 years, 12 months ago (2016-05-23 07:38:48 UTC) #68
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....

Powered by Google App Engine
This is Rietveld 408576698