Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219933003:
Use LocalMediaStreamAudioSource for screen-casting use cases. (Closed)
|
Created:
4 years, 4 months ago by miu Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse LocalMediaStreamAudioSource for screen-casting use cases.
Adds a new MediaStreamAudioSource implementation for taking input from a
local source, and passing it directly to sinks without going through the
WebRTC MediaStreamAudioProcessor. This new implementation will be used
by the screen casting use cases (e.g., tab or desktop audio capture).
Before this change, even if the MediaStreamAudioProcessor was disabled,
a rather large graph of objects, extra threads (for libjingle), and
additional audio buffers were being created; with audio being copied
into and out of the buffers; even though no processing was being done.
BUG=577881, 638081
Committed: https://crrev.com/81c7ecbbe69b2be50bf2a6a30b4e88d72b09e249
Cr-Commit-Position: refs/heads/master@{#412333}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments on patch set 1 (mostly rewrote comments). #
Total comments: 2
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miu@chromium.org changed reviewers: + olka@chromium.org, sievers@chromium.org
olka: PTAL. This is the last piece to finally resolve https://crbug.com/577881. Better late than never! ;-) sievers: Need OWNERS RS for additions to content/public/common/media_stream_request.*.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
miu@chromium.org changed reviewers: + henrika@chromium.org - olka@chromium.org
henrika: Can you PTAL? (Olga's OOO it seems.)
miu@chromium.org changed reviewers: + xjz@chromium.org - henrika@chromium.org
Hmm...Having trouble finding a code reviewer from the Stockholm team. So, will try a local reviewer since this is a pretty straightforward change. xjz: PTAL. :-)
LGTM https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:457: // MediaStreamAudioProcessor::InitializeAudioProcessingModule(). nit: The logic here is in sync with that in MediaStreamAudioProcessor::IniitializeAudioProcessingModule() except for the GetGoogAudioMirroring() on Line 461. Can we move this comment below that logic, e.g., to Line 463, and s/This method/The following ?
Olga is back today and she knows this code base much better than I do ;-)
Description was changed from ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881 ========== to ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881 ==========
henrika@chromium.org changed reviewers: + olka@chromium.org
Sorry for the delay. A couple of questions: https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.cc (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.cc:29: device_info.device.input.sample_rate / 100)); Should we probably unify ProcessedLocalAudioSource::GetBufferSize() (https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/processed_...) and use it here? Do we probably want to keep Android behavior unchanged in this CL (2 * sample_rate / 100)? https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.h (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.h:18: class CONTENT_EXPORT LocalMediaStreamAudioSource Should probably the naming match ProcessedLocalAudioSource? Like, "UnprocessedLocalAudioSource"? https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:550: // Audio mirroring can be enabled even though audio processing is otherwise ShouldRouteAudioThroughProcessor() always returns true when audio mirroring is enabled. Do you mean by this comment that audio processing is on if mirroring is on? Or do you meant that even if mirroring is on audio processing may be off? (If latter, than why ShouldRouteAudioThroughProcessor() returns true?) https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:579: DCHECK_EQ(audio_mirroring_, ShouldRouteAudioThroughProcessor( See the comment above: it's confusing when ShouldRouteAudioThroughProcessor() is true, but AUDIO_PROCESSING_DISABLED is set. What is the actual meaning of this "Should..." method then?
o1ka: Thanks, PTAL. sievers: Need OWNERS RS for additions to content/public/common/media_stream_request.*. https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.cc (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.cc:29: device_info.device.input.sample_rate / 100)); On 2016/08/15 15:15:00, o1ka wrote: > Should we probably unify ProcessedLocalAudioSource::GetBufferSize() > (https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/processed_...) > and use it here? Do we probably want to keep Android behavior unchanged in this > CL (2 * sample_rate / 100)? Done. Opened a crbug for tracking (http://crbug.com/638081) with TODO comment. https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.h (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.h:18: class CONTENT_EXPORT LocalMediaStreamAudioSource On 2016/08/15 15:15:00, o1ka wrote: > Should probably the naming match ProcessedLocalAudioSource? Like, > "UnprocessedLocalAudioSource"? I don't know if it's necessary to call-out in its name the fact that this implementation doesn't process the audio. Unless you feel super-strongly about it, IMHO, the current naming is accurate (because this is a MediaStreamAudioSource of locally-generated audio). https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:457: // MediaStreamAudioProcessor::InitializeAudioProcessingModule(). On 2016/08/13 00:43:56, xjz wrote: > nit: The logic here is in sync with that in > MediaStreamAudioProcessor::IniitializeAudioProcessingModule() except for the > GetGoogAudioMirroring() on Line 461. Can we move this comment below that logic, > e.g., to Line 463, and s/This method/The following ? Actually, it's perfectly in-sync. If GetGoogAudioMirroring() returns true, MediaStreamAudioProcessor will reverse the left and right channels of the audio. So, the method should return true because the constraints would cause the audio signal to be modified. https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:550: // Audio mirroring can be enabled even though audio processing is otherwise On 2016/08/15 15:15:00, o1ka wrote: > ShouldRouteAudioThroughProcessor() always returns true when audio mirroring is > enabled. Do you mean by this comment that audio processing is on if mirroring is > on? Or do you meant that even if mirroring is on audio processing may be off? > (If latter, than why ShouldRouteAudioThroughProcessor() returns true?) This comment was confusing; and I've fixed it to correctly explain what the implementation will do. https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:579: DCHECK_EQ(audio_mirroring_, ShouldRouteAudioThroughProcessor( On 2016/08/15 15:15:00, o1ka wrote: > See the comment above: it's confusing when ShouldRouteAudioThroughProcessor() is > true, but AUDIO_PROCESSING_DISABLED is set. What is the actual meaning of this > "Should..." method then? I made some clarifications on these comments too. Also, I've renamed the ShouldRouteAudioThroughProcessor() function to WouldModifyAudio() and updated the header file comments.
Description was changed from ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881 ========== to ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881,638081 ==========
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you, the changes in naming and comments do make a difference. lgtm https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.h (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.h:18: class CONTENT_EXPORT LocalMediaStreamAudioSource On 2016/08/16 04:30:02, miu wrote: > On 2016/08/15 15:15:00, o1ka wrote: > > Should probably the naming match ProcessedLocalAudioSource? Like, > > "UnprocessedLocalAudioSource"? > > I don't know if it's necessary to call-out in its name the fact that this > implementation doesn't process the audio. Unless you feel super-strongly about > it, IMHO, the current naming is accurate (because this is a > MediaStreamAudioSource of locally-generated audio). I just find it easier to read the code when naming reflects inheritance hierarchy. (ProcessedLocalMediaStreamAudioSource is unfortunately too long :) ) No, I don't feel super-strongly, just a little annoyed that I have to embrace and memorize that those classes are siblings :) https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... File content/renderer/media/local_media_stream_audio_source.cc (right): https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... content/renderer/media/local_media_stream_audio_source.cc:22: int frames_per_buffer = device_info.device.input.frames_per_buffer; I would prefer to avoid the duplication: now we have this weird Android hack in two places. Can it be a function which calculates it? (I still think it's a good idea to keep buffer size changes separated from this CL: we already had some problems with buffer size changes before, it requires a separate attention)
https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... File content/renderer/media/local_media_stream_audio_source.h (right): https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... content/renderer/media/local_media_stream_audio_source.h:18: class CONTENT_EXPORT LocalMediaStreamAudioSource On 2016/08/16 14:18:11, o1ka wrote: > I just find it easier to read the code when naming reflects inheritance > hierarchy. (ProcessedLocalMediaStreamAudioSource is unfortunately too long :) ) > No, I don't feel super-strongly, just a little annoyed that I have to embrace > and memorize that those classes are siblings :) I can understand this concern. FWIW, the other two implementations of MediaStreamAudioSource, ExternalMediaStreamAudioSource and PeerConnectionRemoteAudioSource, don't do processing either. So, if anything, the ProcessedLocalAudioSource is the only "special" one to remember. ;-) https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... File content/renderer/media/local_media_stream_audio_source.cc (right): https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... content/renderer/media/local_media_stream_audio_source.cc:22: int frames_per_buffer = device_info.device.input.frames_per_buffer; On 2016/08/16 14:18:11, o1ka wrote: > I would prefer to avoid the duplication: now we have this weird Android hack in > two places. Can it be a function which calculates it? (I still think it's a good > idea to keep buffer size changes separated from this CL: we already had some > problems with buffer size changes before, it requires a separate attention) Yeah, the duplication bothered me a little too. The options here are: 1. Find a centralized place for the GetBufferSize() code (only two of the MSAudioSources actually need it). It's not exactly clear where this should live. Maybe as a static function in the base class, even though other subclasses don't need it? 2. Keep it in two places, both referencing the same crbug; and hope we can delete it soon. Based on henrika's recent comments in the crbug, option 2 looks much more attractive to me. That makes this duplicated code temporary. In fact, I'll just follow-up on this now and see if I can get this stuff deleted in a follow-up CL...
content/public lgtm stamp
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org Link to the patchset: https://codereview.chromium.org/2219933003/#ps20001 (title: "Addressed comments on patch set 1 (mostly rewrote comments).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881,638081 ========== to ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881,638081 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881,638081 ========== to ========== Use LocalMediaStreamAudioSource for screen-casting use cases. Adds a new MediaStreamAudioSource implementation for taking input from a local source, and passing it directly to sinks without going through the WebRTC MediaStreamAudioProcessor. This new implementation will be used by the screen casting use cases (e.g., tab or desktop audio capture). Before this change, even if the MediaStreamAudioProcessor was disabled, a rather large graph of objects, extra threads (for libjingle), and additional audio buffers were being created; with audio being copied into and out of the buffers; even though no processing was being done. BUG=577881,638081 Committed: https://crrev.com/81c7ecbbe69b2be50bf2a6a30b4e88d72b09e249 Cr-Commit-Position: refs/heads/master@{#412333} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/81c7ecbbe69b2be50bf2a6a30b4e88d72b09e249 Cr-Commit-Position: refs/heads/master@{#412333}
Message was sent while issue was closed.
On 2016/08/16 20:13:39, miu wrote: > https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... > File content/renderer/media/local_media_stream_audio_source.h (right): > > https://codereview.chromium.org/2219933003/diff/1/content/renderer/media/loca... > content/renderer/media/local_media_stream_audio_source.h:18: class > CONTENT_EXPORT LocalMediaStreamAudioSource > On 2016/08/16 14:18:11, o1ka wrote: > > I just find it easier to read the code when naming reflects inheritance > > hierarchy. (ProcessedLocalMediaStreamAudioSource is unfortunately too long :) > ) > > No, I don't feel super-strongly, just a little annoyed that I have to embrace > > and memorize that those classes are siblings :) > > I can understand this concern. FWIW, the other two implementations of > MediaStreamAudioSource, ExternalMediaStreamAudioSource and > PeerConnectionRemoteAudioSource, don't do processing either. So, if anything, > the ProcessedLocalAudioSource is the only "special" one to remember. ;-) > > https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... > File content/renderer/media/local_media_stream_audio_source.cc (right): > > https://codereview.chromium.org/2219933003/diff/20001/content/renderer/media/... > content/renderer/media/local_media_stream_audio_source.cc:22: int > frames_per_buffer = device_info.device.input.frames_per_buffer; > On 2016/08/16 14:18:11, o1ka wrote: > > I would prefer to avoid the duplication: now we have this weird Android hack > in > > two places. Can it be a function which calculates it? (I still think it's a > good > > idea to keep buffer size changes separated from this CL: we already had some > > problems with buffer size changes before, it requires a separate attention) > > Yeah, the duplication bothered me a little too. The options here are: > > 1. Find a centralized place for the GetBufferSize() code (only two of the > MSAudioSources actually need it). It's not exactly clear where this should live. > Maybe as a static function in the base class, even though other subclasses don't > need it? > > 2. Keep it in two places, both referencing the same crbug; and hope we can > delete it soon. > > Based on henrika's recent comments in the crbug, option 2 looks much more > attractive to me. That makes this duplicated code temporary. In fact, I'll just > follow-up on this now and see if I can get this stuff deleted in a follow-up > CL... Sounds good! |
