Fix/Add content_unittests to work for the code behind --enable-audio-track-processing

Fix/Add unittest to work for the code behind --enable-audio-track-processing.



BUG=264611
TEST=content_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256640

[no longer working on chromium, 2014-03-11 13:02:15]

Hi guys,

This patch makes the content_unittests work with the code behind
--enable-audio-track-processing flag. I am not able to run the all the tests
with and without the flag since it will touch all the test code. So I just
modified some important tests which are directly related to the
MediaStreamAudioProcessor to run with and without the flag.

Please take a look.

Tommi, I know you are busy, feel free to take a look or bypass this CL.

Thanks,
SX

[tommi (sloooow) - chröme, 2014-03-11 13:30:58]

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:467:
!audio_processor_->IsAudioTrackProcessingEnabled() : false;
This isn't the same behavior as before.  IsAudioTrackProcessingEnabled checks a
command line flag whereas has_audio_processing() was for the track's
constraints, right?

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:68: const
media::AudioParameters& params() const { return params_; }
nit: audio_parameters() const

[no longer working on chromium, 2014-03-11 14:34:44]

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:467:
!audio_processor_->IsAudioTrackProcessingEnabled() : false;
On 2014/03/11 13:30:58, tommi wrote:
> This isn't the same behavior as before.  IsAudioTrackProcessingEnabled checks
a
> command line flag whereas has_audio_processing() was for the track's
> constraints, right?

Yes. 
When the command line flag is on, |need_audio_processing| should be always false
since we don't want the APM in WebRtc anyway.

The code here is behaving the same as before, but I think it is more correct to
do it this way.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:68: const
media::AudioParameters& params() const { return params_; }
On 2014/03/11 13:30:58, tommi wrote:
> nit: audio_parameters() const

:) Just realized that it was not used. Removed.

[phoglund_chromium, 2014-03-11 14:43:20]

Sorry, I did the review earlier but forgot to send the comments :/

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:232: // Setup
the audio processor with enabling the flag.
Nit: Set up the audio processor with the flag enabled.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:123: // MaxVolume() in
WebRtcAudioCapturer is hard-coded to return 255, we add
Nit: prefer blank line before comment.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:124: // 0.5 to do the
correct truncation as how the production code does.
Nit: like the production code does.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:126:
scoped_ptr<media::AudioBus> audio_bus = media::AudioBus::Create(params_);
Alright, umm, so you do 0.9 * 255 = 229.5 and add the 0.5 to get to 230. I think
I would just hard-code the 230 and explain the value in a comment. If MaxVolume
changes for whatever reason, I think it's fine for this test to break.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:131: // Verify the sink
is getting the correct values.
Nit: blank line before comment.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:169: // Turn off the
default constraints.
Perhaps explain why you turn off the default constraints?

[no longer working on chromium, 2014-03-11 15:22:30]

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_unittest.cc:232: // Setup
the audio processor with enabling the flag.
On 2014/03/11 14:43:21, phoglund wrote:
> Nit: Set up the audio processor with the flag enabled.

Done.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:123: // MaxVolume() in
WebRtcAudioCapturer is hard-coded to return 255, we add
On 2014/03/11 14:43:21, phoglund wrote:
> Nit: prefer blank line before comment.

Done.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:124: // 0.5 to do the
correct truncation as how the production code does.
On 2014/03/11 14:43:21, phoglund wrote:
> Nit: like the production code does.

Done.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:126:
scoped_ptr<media::AudioBus> audio_bus = media::AudioBus::Create(params_);
On 2014/03/11 14:43:21, phoglund wrote:
> Alright, umm, so you do 0.9 * 255 = 229.5 and add the 0.5 to get to 230. I
think
> I would just hard-code the 230 and explain the value in a comment. If
MaxVolume
> changes for whatever reason, I think it's fine for this test to break.

Hard-coding the value is not preferred in any sense.
Also, the current code allows us to verify the behaviour regardless what
MaxVolume() returns. How about keeping the same behaviour?

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:131: // Verify the sink
is getting the correct values.
On 2014/03/11 14:43:21, phoglund wrote:
> Nit: blank line before comment.

Done.

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:169: // Turn off the
default constraints.
On 2014/03/11 14:43:21, phoglund wrote:
> Perhaps explain why you turn off the default constraints?

Done.

[phoglund_chromium, 2014-03-11 16:20:34]

lgtm

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer_unittest.cc (right):

https://codereview.chromium.org/190643003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer_unittest.cc:126:
scoped_ptr<media::AudioBus> audio_bus = media::AudioBus::Create(params_);
On 2014/03/11 15:22:30, xians1 wrote:
> On 2014/03/11 14:43:21, phoglund wrote:
> > Alright, umm, so you do 0.9 * 255 = 229.5 and add the 0.5 to get to 230. I
> think
> > I would just hard-code the 230 and explain the value in a comment. If
> MaxVolume
> > changes for whatever reason, I think it's fine for this test to break.
> 
> Hard-coding the value is not preferred in any sense.
> Also, the current code allows us to verify the behaviour regardless what
> MaxVolume() returns. How about keeping the same behaviour?
I think in tests it makes sense, since I think it makes sense to catch
unexpected changes to MaxVolume. Although that's not the main point of the test,
so it's your call if you want to make that change.

[perkj_chrome, 2014-03-12 06:55:06]

lgtm if the below comment in addressed. I did not look that thoroughly on the
audio tests since I am no expert.

https://codereview.chromium.org/190643003/diff/90001/content/renderer/media/m...
File content/renderer/media/mock_media_constraint_factory.cc (right):

https://codereview.chromium.org/190643003/diff/90001/content/renderer/media/m...
content/renderer/media/mock_media_constraint_factory.cc:64:
MockMediaConstraintFactory factory;
unused

[tommi (sloooow) - chröme, 2014-03-12 14:16:38]

lgtm

[no longer working on chromium, 2014-03-12 14:27:57]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-12 14:28:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190643003/90001

[commit-bot: I haz the power, 2014-03-12 14:28:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 14:28:56]

Failed to apply patch for
content/renderer/media/media_stream_audio_processor.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/media_stream_audio_processor.cc
  Hunk #1 FAILED at 264.
  Hunk #2 succeeded at 492 (offset 4 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/media/media_stream_audio_processor.cc.rej

Patch:       content/renderer/media/media_stream_audio_processor.cc
Index: content/renderer/media/media_stream_audio_processor.cc
diff --git a/content/renderer/media/media_stream_audio_processor.cc
b/content/renderer/media/media_stream_audio_processor.cc
index
ef693b241cee6b352a6c28efe5ba97d58cacd7a6..cdaa7f127d2e6a1bafc8a225fd8ef39310633071
100644
--- a/content/renderer/media/media_stream_audio_processor.cc
+++ b/content/renderer/media/media_stream_audio_processor.cc
@@ -264,10 +264,8 @@ void
MediaStreamAudioProcessor::InitializeAudioProcessingModule(
     const blink::WebMediaConstraints& constraints, int effects,
     MediaStreamType type) {
   DCHECK(!audio_processing_);
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableAudioTrackProcessing)) {
+  if (!IsAudioTrackProcessingEnabled())
     return;
-  }
 
   RTCMediaConstraints native_constraints(constraints);
 
@@ -488,4 +486,9 @@ void MediaStreamAudioProcessor::StopAudioProcessing() {
   audio_processing_.reset();
 }
 
+bool MediaStreamAudioProcessor::IsAudioTrackProcessingEnabled() const {
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableAudioTrackProcessing);
+}
+
 }  // namespace content

[no longer working on chromium, 2014-03-12 17:33:36]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-12 17:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190643003/120001

[commit-bot: I haz the power, 2014-03-12 19:26:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/190643003/120001

[commit-bot: I haz the power, 2014-03-12 21:02:43]

Change committed as 256640