Remove usage of VoEVolumeControl from WVoE and Audio[Send|Receive]Stream.

Remove usage of VoEVolumeControl from WVoE and Audio[Send|Receive]Stream.

BUG=webrtc:4690
Review-Url: https://codereview.webrtc.org/2721003002
Cr-Commit-Position: refs/heads/master@{#16956}
Committed: https://chromium.googlesource.com/external/webrtc/+/796b8f9d71ffa733097d95f31efe5d9a8e4d6b8e

[the sun, 2017-02-28 10:54:06]

The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 10:54:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[the sun, 2017-02-28 10:57:03]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-02-28 11:19:47]

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

[commit-bot: I haz the power, 2017-02-28 11:19:48]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-28 13:59:19]

lgtm, with some comments

https://codereview.webrtc.org/2721003002/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream_unittest.cc:70: virtual ~MockTransmitMixer() =
default;
Do you need these two lines?

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
transmit_mixer()->AudioLevel();
const? Also, why camelCase?

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
Why initialize?

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:134: int error =
channel()->GetSpeechOutputLevel(level);
const?

[the sun, 2017-03-02 00:36:25]

https://codereview.webrtc.org/2721003002/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream_unittest.cc:70: virtual ~MockTransmitMixer() =
default;
On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> Do you need these two lines?

Nope.

I'm tempted to make it a struct to remove one more. :)

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
transmit_mixer()->AudioLevel();
On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> const? Also, why camelCase?

a) Do you mean currentLevel (sic) should be const? I don't usually do that with
local variables that are set from a function call. Does the style guide mention
that as good practice?

b) Copy+paste bogus. Thanks!

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> Why initialize?

Variables should always be initialized, unless there is a particular performance
reason why not to. In particular with an interface like the below function, it's
not evident that it may always be set without looking closely at the underlying
code.

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:134: int error =
channel()->GetSpeechOutputLevel(level);
On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> const?

For error? No.

[the sun, 2017-03-02 00:37:06]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-03-02 00:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2721003002/#ps20001 (title:
"rebase+comment")

[commit-bot: I haz the power, 2017-03-02 00:37:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-02 01:02:24]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488415026670380,
"parent_rev": "d48f488bed37e8a5af47bd4be85d9b819fa284b4", "commit_rev":
"796b8f9d71ffa733097d95f31efe5d9a8e4d6b8e"}

[commit-bot: I haz the power, 2017-03-02 01:02:27]

Description was changed from

==========
Remove usage of VoEVolumeControl from WVoE and Audio[Send|Receive]Stream.

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEVolumeControl from WVoE and Audio[Send|Receive]Stream.

BUG=webrtc:4690

Review-Url: https://codereview.webrtc.org/2721003002
Cr-Commit-Position: refs/heads/master@{#16956}
Committed:
https://chromium.googlesource.com/external/webrtc/+/796b8f9d71ffa733097d95f31...
==========

[commit-bot: I haz the power, 2017-03-02 01:02:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/796b8f9d71ffa733097d95f31...

[kwiberg-webrtc, 2017-03-02 02:37:37]

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
transmit_mixer()->AudioLevel();
On 2017/03/02 00:36:25, the sun wrote:
> On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > const? Also, why camelCase?
> 
> a) Do you mean currentLevel (sic) should be const? I don't usually do that
with
> local variables that are set from a function call. Does the style guide
mention
> that as good practice?

It just says "Use const whenever it makes sense." Beyond that it gives no
explicit guidelines for local variables that I can find. Personally, I've found
it a good habit to make locals const whenever possible, which is why I recommend
it to others. The real payoff comes in places where the variable in question has
a scope of more than just three lines, though, so I don't feel strongly about it
for this particular case.

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
On 2017/03/02 00:36:25, the sun wrote:
> On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > Why initialize?
> 
> Variables should always be initialized, unless there is a particular
performance
> reason why not to. In particular with an interface like the below function,
it's
> not evident that it may always be set without looking closely at the
underlying
> code.

On the contrary, I'd argue that the call below is expected to *always* set level
unless it signals an error (this is the usual contract for output parameters).
By not initializing level, you give MemorySanitizer a chance to verify this, and
you tell the reader of the code that you expect level to always be filled in
with a real value.

Initializing to a default value you're sure won't be needed is akin to having a
default case in a switch that you don't think you need. It can hide bugs, and
readers may be fooled into thinking the default value is actually needed.

[the sun, 2017-03-02 04:57:16]

On 2017/03/02 02:37:37, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
> transmit_mixer()->AudioLevel();
> On 2017/03/02 00:36:25, the sun wrote:
> > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > const? Also, why camelCase?
> > 
> > a) Do you mean currentLevel (sic) should be const? I don't usually do that
> with
> > local variables that are set from a function call. Does the style guide
> mention
> > that as good practice?
> 
> It just says "Use const whenever it makes sense." Beyond that it gives no
> explicit guidelines for local variables that I can find. Personally, I've
found
> it a good habit to make locals const whenever possible, which is why I
recommend
> it to others. The real payoff comes in places where the variable in question
has
> a scope of more than just three lines, though, so I don't feel strongly about
it
> for this particular case.
> 
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> File webrtc/voice_engine/channel_proxy.cc (right):
> 
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
> On 2017/03/02 00:36:25, the sun wrote:
> > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > Why initialize?
> > 
> > Variables should always be initialized, unless there is a particular
> performance
> > reason why not to. In particular with an interface like the below function,
> it's
> > not evident that it may always be set without looking closely at the
> underlying
> > code.
> 
> On the contrary, I'd argue that the call below is expected to *always* set
level
> unless it signals an error (this is the usual contract for output parameters).
> By not initializing level, you give MemorySanitizer a chance to verify this,
and
> you tell the reader of the code that you expect level to always be filled in
> with a real value.
>
> Initializing to a default value you're sure won't be needed is akin to having
a
> default case in a switch that you don't think you need. It can hide bugs, and
> readers may be fooled into thinking the default value is actually needed.

I agree with half of what you say. :) In this case I should have initialized the
variable to something which is *known* to be illegal, to make sure downstream
tests/assertions fail consistently. Yes, tests *should* catch such a condition
(not saying we have enough coverage). Relying on an external tool to find this
class of bug, to me is less favorable than a programming style which prevents
them.

[kwiberg-webrtc, 2017-03-02 08:30:02]

On 2017/03/02 04:57:16, the sun wrote:
> On 2017/03/02 02:37:37, kwiberg-webrtc wrote:
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> > webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
> > transmit_mixer()->AudioLevel();
> > On 2017/03/02 00:36:25, the sun wrote:
> > > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > > const? Also, why camelCase?
> > > 
> > > a) Do you mean currentLevel (sic) should be const? I don't usually do that
> > with
> > > local variables that are set from a function call. Does the style guide
> > mention
> > > that as good practice?
> > 
> > It just says "Use const whenever it makes sense." Beyond that it gives no
> > explicit guidelines for local variables that I can find. Personally, I've
> found
> > it a good habit to make locals const whenever possible, which is why I
> recommend
> > it to others. The real payoff comes in places where the variable in question
> has
> > a scope of more than just three lines, though, so I don't feel strongly
about
> it
> > for this particular case.
> > 
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> > File webrtc/voice_engine/channel_proxy.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> > webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
> > On 2017/03/02 00:36:25, the sun wrote:
> > > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > > Why initialize?
> > > 
> > > Variables should always be initialized, unless there is a particular
> > performance
> > > reason why not to. In particular with an interface like the below
function,
> > it's
> > > not evident that it may always be set without looking closely at the
> > underlying
> > > code.
> > 
> > On the contrary, I'd argue that the call below is expected to *always* set
> level
> > unless it signals an error (this is the usual contract for output
parameters).
> > By not initializing level, you give MemorySanitizer a chance to verify this,
> and
> > you tell the reader of the code that you expect level to always be filled in
> > with a real value.
> >
> > Initializing to a default value you're sure won't be needed is akin to
having
> a
> > default case in a switch that you don't think you need. It can hide bugs,
and
> > readers may be fooled into thinking the default value is actually needed.
> 
> I agree with half of what you say. :) In this case I should have initialized
the
> variable to something which is *known* to be illegal, to make sure downstream
> tests/assertions fail consistently. Yes, tests *should* catch such a condition
> (not saying we have enough coverage). Relying on an external tool to find this
> class of bug, to me is less favorable than a programming style which prevents
> them.

Initializing to a known-bad value doesn't prevent the bug either---it's just
another way to enable tests and tooling to detect the bug. Or are you suggesting
there should be an error path after the call that tests for the illegal value
and deals with the problem?

[the sun, 2017-03-02 14:07:22]

On 2017/03/02 08:30:02, kwiberg-webrtc wrote:
> On 2017/03/02 04:57:16, the sun wrote:
> > On 2017/03/02 02:37:37, kwiberg-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> > > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/media/engine/webrtcvoi...
> > > webrtc/media/engine/webrtcvoiceengine.cc:965: int8_t currentLevel =
> > > transmit_mixer()->AudioLevel();
> > > On 2017/03/02 00:36:25, the sun wrote:
> > > > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > > > const? Also, why camelCase?
> > > > 
> > > > a) Do you mean currentLevel (sic) should be const? I don't usually do
that
> > > with
> > > > local variables that are set from a function call. Does the style guide
> > > mention
> > > > that as good practice?
> > > 
> > > It just says "Use const whenever it makes sense." Beyond that it gives no
> > > explicit guidelines for local variables that I can find. Personally, I've
> > found
> > > it a good habit to make locals const whenever possible, which is why I
> > recommend
> > > it to others. The real payoff comes in places where the variable in
question
> > has
> > > a scope of more than just three lines, though, so I don't feel strongly
> about
> > it
> > > for this particular case.
> > > 
> > >
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> > > File webrtc/voice_engine/channel_proxy.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2721003002/diff/1/webrtc/voice_engine/channel_p...
> > > webrtc/voice_engine/channel_proxy.cc:133: uint32_t level = 0;
> > > On 2017/03/02 00:36:25, the sun wrote:
> > > > On 2017/02/28 13:59:18, kwiberg-webrtc wrote:
> > > > > Why initialize?
> > > > 
> > > > Variables should always be initialized, unless there is a particular
> > > performance
> > > > reason why not to. In particular with an interface like the below
> function,
> > > it's
> > > > not evident that it may always be set without looking closely at the
> > > underlying
> > > > code.
> > > 
> > > On the contrary, I'd argue that the call below is expected to *always* set
> > level
> > > unless it signals an error (this is the usual contract for output
> parameters).
> > > By not initializing level, you give MemorySanitizer a chance to verify
this,
> > and
> > > you tell the reader of the code that you expect level to always be filled
in
> > > with a real value.
> > >
> > > Initializing to a default value you're sure won't be needed is akin to
> having
> > a
> > > default case in a switch that you don't think you need. It can hide bugs,
> and
> > > readers may be fooled into thinking the default value is actually needed.
> > 
> > I agree with half of what you say. :) In this case I should have initialized
> the
> > variable to something which is *known* to be illegal, to make sure
downstream
> > tests/assertions fail consistently. Yes, tests *should* catch such a
condition
> > (not saying we have enough coverage). Relying on an external tool to find
this
> > class of bug, to me is less favorable than a programming style which
prevents
> > them.
> 
> Initializing to a known-bad value doesn't prevent the bug either---it's just
> another way to enable tests and tooling to detect the bug. Or are you
suggesting
> there should be an error path after the call that tests for the illegal value
> and deals with the problem?

No, that's not what I suggest. Asserting valid ranges and having appropriate
tests should be enough. The point is to make it deterministically detectable.
Without relying on tools that may not be available on all platforms.