Move functionality out from AudioFrame and into AudioFrameOperations.

Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations,
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.

TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True

Committed: https://crrev.com/6321b49a0da3ebdeede1430aa9bc825855646b79
Cr-Commit-Position: refs/heads/master@{#15413}

[aleloi, 2016-10-18 10:54:01]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute, Reset, UpdateFrame are
moved into audio_frame_operations. A new GN target
webrtc/modules/utility:audio_frame_operations is created in order to
avoid a circular dependency between modules/utility and
modules/audio_coding.

BUG=webrtc:6548
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute, Reset, UpdateFrame are
moved into audio_frame_operations. A new GN target
webrtc/modules/utility:audio_frame_operations is created in order to
avoid a circular dependency between modules/utility and
modules/audio_coding.

BUG=webrtc:6548
==========

[aleloi, 2016-10-18 10:54:01]

aleloi@webrtc.org changed reviewers:
+ kjellander@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org

[aleloi, 2016-10-18 11:36:38]

Patchset #1 (id:1) has been deleted

[aleloi, 2016-10-18 11:38:04]

Can you please take a look?

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (left):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:520: *   deciding the resulting
state. To do this use the -operator.
I think this comment is not valid: there is no -operator and operator+ just adds
samples together. Opposite frames add to zero.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:30: // |result_frame| is
empty.
I'll try to improve this comment somewhat.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
File webrtc/modules/utility/source/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:25: 
All implementations are copied from AudioFrame with the following exceptions:

All asserts are replaced with RTC_DCHECKs.
memset and memcpy are changed into std::fill and std::copy.
NULL is changed into nullptr.

operator>>=(int) is renamed into ShiftDown(). We only downshifts by 1, so
ShiftDown() right-shifts by 1 and doesn't take parameters.

operator+=(frame) is renamed into AddFrames(frame_to_add, result_frame).

[kwiberg-webrtc, 2016-10-19 09:22:30]

Why do you want to change these from methods to static methods of another class?
I would have thought that the opposite (turning the other static methods in
AudioFrameOperations into regular AudioFrame methods) would be the way to go.

[the sun, 2016-10-19 09:23:17]

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/audio_conf...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:43:
AudioFrameOperations::ShiftDown(frame);
Note, if this function was called DivideBy2(), ApplyHalfGain() or similar, the
comment could be removed.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:21: // than a class.
Perhaps address that comment in a follow-up? Would indeed be nice to remove the
class and make this namespace audio_frame_operations { instead.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:73: static void
ShiftDown(AudioFrame* frame);
Consider renaming to "HalfGain()", "ApplyHalfGain()", "ScaleToHalf()" or perhaps
"ScaledDown6dB()"

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:90: int16_t
ClampToInt16(int32_t input);
Add TODO to move this into a separate file (we probably have similar utilities
which could benefit from being in the same place).

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
File webrtc/modules/utility/source/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:25: 
On 2016/10/18 11:38:04, aleloi wrote:
> All implementations are copied from AudioFrame with the following exceptions:
> 
> All asserts are replaced with RTC_DCHECKs.
> memset and memcpy are changed into std::fill and std::copy.
> NULL is changed into nullptr.
> 
> operator>>=(int) is renamed into ShiftDown(). We only downshifts by 1, so
> ShiftDown() right-shifts by 1 and doesn't take parameters.
> 
> operator+=(frame) is renamed into AddFrames(frame_to_add, result_frame).

Can we just make it "Add()" or "AddTo()"? Or you could add a "Frames" suffix for
all of the functions...

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:64: // IMPROVEMENT this
can be done very fast in assembly
I think you can remove the comment

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:185: frame->id_ = -1;
It is dangerous to have the default values in multiple places. Is this
performance critical? (and that is why we're not zeroing data_)? If not,
default-construct an AudioFrame on the stack and assign the values from there.

[the sun, 2016-10-19 09:37:32]

It is great that you're taking the time to set up build dependencies properly by
the way!

I think we could even take the opportunity to move audio_frame_operations* to a
new home in webrtc/audio/utility/ (folder is not there yet).

The reason for the webrtc/modules/ folder is historical and evident when looking
in webrtc/modules/include/ - it was a system for various parts of the code to
hook in and be periodically called. Among the audio modules, only the conference
mixer you're replacing uses this anymore, so we could just as well untangle the
code by moving all the audio "modules" somewhere else, e.g. to webrtc/audio/

[the sun, 2016-10-19 10:20:05]

On 2016/10/19 09:22:30, kwiberg-webrtc wrote:
> Why do you want to change these from methods to static methods of another
class?
> I would have thought that the opposite (turning the other static methods in
> AudioFrameOperations into regular AudioFrame methods) would be the way to go.

We want to move AudioFrame into api/ and therefore want to make it as POD-like
as possible.

[kwiberg-webrtc, 2016-10-19 10:49:37]

On 2016/10/19 10:20:05, the sun wrote:
> On 2016/10/19 09:22:30, kwiberg-webrtc wrote:
> > Why do you want to change these from methods to static methods of another
> class?
> > I would have thought that the opposite (turning the other static methods in
> > AudioFrameOperations into regular AudioFrame methods) would be the way to
go.
> 
> We want to move AudioFrame into api/ and therefore want to make it as POD-like
> as possible.

Oh. I don't think that's what we should be wanting. But let's have that
discussion in person...

[kwiberg-webrtc, 2016-10-19 12:19:47]

OK, took a closer look, and (depending on what we intend users to do with the
type) it seems far from clear that UpdateFrame() and Reset() shouldn't be
methods. The shift and plus operators can probably go, though; as for Mute(), se
earlier point about what we intend users to do with the type.

As for the data members, there's a bunch of details that should be fixed (off
the top of my head: trailing underscores, using rtc::Optional, heap allocation
of the data array), but they seem basically sane.

[aleloi, 2016-10-20 08:27:06]

I tried to set up a trybot compilation alias. That's why the trybot result list
is so colorful...

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/audio_conf...
File webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc:43:
AudioFrameOperations::ShiftDown(frame);
On 2016/10/19 09:23:16, the sun wrote:
> Note, if this function was called DivideBy2(), ApplyHalfGain() or similar, the
> comment could be removed.

Done.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:21: // than a class.
On 2016/10/19 09:23:17, the sun wrote:
> Perhaps address that comment in a follow-up? Would indeed be nice to remove
the
> class and make this namespace audio_frame_operations { instead.

Acknowledged.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:73: static void
ShiftDown(AudioFrame* frame);
On 2016/10/19 09:23:17, the sun wrote:
> Consider renaming to "HalfGain()", "ApplyHalfGain()", "ScaleToHalf()" or
perhaps
> "ScaledDown6dB()"

Done.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:90: int16_t
ClampToInt16(int32_t input);
On 2016/10/19 09:23:17, the sun wrote:
> Add TODO to move this into a separate file (we probably have similar utilities
> which could benefit from being in the same place).

Done.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
File webrtc/modules/utility/source/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:25: 
On 2016/10/19 09:23:17, the sun wrote:
> On 2016/10/18 11:38:04, aleloi wrote:
> > All implementations are copied from AudioFrame with the following
exceptions:
> > 
> > All asserts are replaced with RTC_DCHECKs.
> > memset and memcpy are changed into std::fill and std::copy.
> > NULL is changed into nullptr.
> > 
> > operator>>=(int) is renamed into ShiftDown(). We only downshifts by 1, so
> > ShiftDown() right-shifts by 1 and doesn't take parameters.
> > 
> > operator+=(frame) is renamed into AddFrames(frame_to_add, result_frame).
> 
> Can we just make it "Add()" or "AddTo()"? Or you could add a "Frames" suffix
for
> all of the functions...

Done.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:64: // IMPROVEMENT this
can be done very fast in assembly
On 2016/10/19 09:23:17, the sun wrote:
> I think you can remove the comment

Done.

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:185: frame->id_ = -1;
On 2016/10/19 09:23:17, the sun wrote:
> It is dangerous to have the default values in multiple places. Is this
> performance critical? (and that is why we're not zeroing data_)? If not,
> default-construct an AudioFrame on the stack and assign the values from there.

I think it might lead to a (barely noticeable) performance regression. Reset is
called in SyncBuffer::GetNextAudioInterleaved (the only non-test code that uses
::Reset), which runs every 10ms for every input channel. The values seems to
never be read anywhere, just updated with new ones. Probably everything should
still work if that Reset call is gone. I don't think it should be removed, since
we'd get a frame full of unknown values passed through a long chain of methods.

I moved Reset back to AudioFrame. Now at least the two sets of default values
are in the same file.

[the sun, 2016-10-20 08:37:57]

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
File webrtc/modules/utility/source/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/40001/webrtc/modules/utility/so...
webrtc/modules/utility/source/audio_frame_operations.cc:185: frame->id_ = -1;
On 2016/10/20 08:27:06, aleloi wrote:
> On 2016/10/19 09:23:17, the sun wrote:
> > It is dangerous to have the default values in multiple places. Is this
> > performance critical? (and that is why we're not zeroing data_)? If not,
> > default-construct an AudioFrame on the stack and assign the values from
there.
> 
> I think it might lead to a (barely noticeable) performance regression. Reset
is
> called in SyncBuffer::GetNextAudioInterleaved (the only non-test code that
uses
> ::Reset), which runs every 10ms for every input channel. The values seems to
> never be read anywhere, just updated with new ones. Probably everything should
> still work if that Reset call is gone. I don't think it should be removed,
since
> we'd get a frame full of unknown values passed through a long chain of
methods.
> 
> I moved Reset back to AudioFrame. Now at least the two sets of default values
> are in the same file.

Acknowledged.

https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:67: // Halve samples in
|frame| by applying a right shift.
Leave out the implementation detail.

[aleloi, 2016-10-20 08:51:20]

https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
File webrtc/modules/utility/include/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
webrtc/modules/utility/include/audio_frame_operations.h:67: // Halve samples in
|frame| by applying a right shift.
On 2016/10/20 08:37:57, the sun wrote:
> Leave out the implementation detail.

Done.

[the sun, 2016-10-24 07:50:26]

On 2016/10/20 08:51:20, aleloi wrote:
>
https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
> File webrtc/modules/utility/include/audio_frame_operations.h (right):
> 
>
https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
> webrtc/modules/utility/include/audio_frame_operations.h:67: // Halve samples
in
> |frame| by applying a right shift.
> On 2016/10/20 08:37:57, the sun wrote:
> > Leave out the implementation detail.
> 
> Done.

I'd still like to move audio_frame_operations* to webrtc/audio/utility/

[kjellander_webrtc, 2016-10-24 08:20:07]

On 2016/10/24 07:50:26, the sun wrote:
> On 2016/10/20 08:51:20, aleloi wrote:
> >
>
https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
> > File webrtc/modules/utility/include/audio_frame_operations.h (right):
> > 
> >
>
https://codereview.webrtc.org/2424173003/diff/60001/webrtc/modules/utility/in...
> > webrtc/modules/utility/include/audio_frame_operations.h:67: // Halve samples
> in
> > |frame| by applying a right shift.
> > On 2016/10/20 08:37:57, the sun wrote:
> > > Leave out the implementation detail.
> > 
> > Done.
> 
> I'd still like to move audio_frame_operations* to webrtc/audio/utility/

+1

[aleloi, 2016-11-21 13:18:47]

Patchset #4 (id:80001) has been deleted

[aleloi, 2016-11-21 14:30:34]

Patchset #5 (id:120001) has been deleted

[aleloi, 2016-11-21 14:31:00]

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

[commit-bot: I haz the power, 2016-11-21 14:31:08]

Dry run: 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, 2016-11-21 14:34:15]

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

[commit-bot: I haz the power, 2016-11-21 14:34:16]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10453)

[aleloi, 2016-11-21 15:36:43]

Patchset #4 (id:100001) has been deleted

[the sun, 2016-11-25 12:08:16]

Can I help you un-stuck this CL somehow?

[aleloi, 2016-11-28 13:21:20]

Patchset #6 (id:180001) has been deleted

[aleloi, 2016-11-28 13:23:35]

Patchset #5 (id:160001) has been deleted

[aleloi, 2016-11-28 15:34:21]

Patchset #6 (id:220001) has been deleted

[aleloi, 2016-11-29 10:09:28]

On 2016/11/25 12:08:16, the sun wrote:
> Can I help you un-stuck this CL somehow?

I've figured out what has been blocking me! The linux_internal test has been
failing, and that is because I've added a new folder with publicly visible
headers. I have to make an internal CL to append add the path to the new header
to a list.

[aleloi, 2016-11-29 13:24:20]

Patchset #6 (id:240001) has been deleted

[aleloi, 2016-11-29 13:24:38]

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

[commit-bot: I haz the power, 2016-11-29 13:24:43]

Dry run: 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, 2016-11-29 13:25:55]

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

[commit-bot: I haz the power, 2016-11-29 13:25:57]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15269)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15129)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19201)

[aleloi, 2016-11-29 13:34:54]

Patchset #6 (id:260001) has been deleted

[aleloi, 2016-11-29 13:35:05]

Patchset #6 (id:280001) has been deleted

[aleloi, 2016-11-29 13:35:17]

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

[commit-bot: I haz the power, 2016-11-29 13:35:20]

Dry run: 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, 2016-11-29 13:38:31]

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

[commit-bot: I haz the power, 2016-11-29 13:38:32]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10774)

[aleloi, 2016-11-29 13:52:40]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute, Reset, UpdateFrame are
moved into audio_frame_operations. A new GN target
webrtc/modules/utility:audio_frame_operations is created in order to
avoid a circular dependency between modules/utility and
modules/audio_coding.

BUG=webrtc:6548
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.

BUG=webrtc:6548
==========

[aleloi, 2016-11-29 13:53:10]

Patchset #5 (id:200001) has been deleted

[aleloi, 2016-11-29 13:53:21]

Patchset #4 (id:140001) has been deleted

[aleloi, 2016-11-29 13:54:07]

Patchset #1 (id:20001) has been deleted

[aleloi, 2016-11-29 13:54:33]

Patchset #2 (id:60001) has been deleted

[aleloi, 2016-11-29 14:01:35]

Hi! Could you take a look at this old CL again? The difference from last time (I
know it's been a month...) is that I moved the audio frame operations to
webrtc/audio/utility and put the original implementations back in AudioFrame
(not doing that will break internal projects).

[kwiberg-webrtc, 2016-11-30 09:38:34]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:30:
RTC_DCHECK_LT(result_frame->num_channels_, 3u);
You don't need the "u"s anymore!

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:48: frame_to_add.vad_activity_ ==
result_frame->kVadActive) {
Remove one layer of parentheses, since the relative precedence of == and || is
well-known.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:67: int32_t wrap_guard =
static_cast<int32_t>(result_frame->data_[i]) +
const

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:187:
RTC_DCHECK_LT(frame->num_channels_, 3u);
Remove "u"s.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:188: if ((frame->num_channels_ >
2) || (frame->num_channels_ < 1))
Remove the extra parentheses.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:236: }
Instead of defining this function, use rtc::saturated_cast<int16_t>.

[aleloi, 2016-11-30 10:11:48]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:30:
RTC_DCHECK_LT(result_frame->num_channels_, 3u);
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> You don't need the "u"s anymore!

Done. Has the implementation of the DCHECK macros changed? (Hurray for that in
that case!)

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:48: frame_to_add.vad_activity_ ==
result_frame->kVadActive) {
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> Remove one layer of parentheses, since the relative precedence of == and || is
> well-known.

Done.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:67: int32_t wrap_guard =
static_cast<int32_t>(result_frame->data_[i]) +
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:187:
RTC_DCHECK_LT(frame->num_channels_, 3u);
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> Remove "u"s.

Done.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:188: if ((frame->num_channels_ >
2) || (frame->num_channels_ < 1))
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> Remove the extra parentheses.

Done.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:236: }
On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> Instead of defining this function, use rtc::saturated_cast<int16_t>.

Done. Great that we have saturated_cast! I missed that.

I checked that it does the same thing as ClampToInt16 by running 
ClampToInt16.TestCases from audio_frame_operations_unittest.cc on it. I also
replaced the ClampToInt16 copy in module_common_types with saturated_cast. That
way we can remove an anonymous namespace in a header, and delete the method
right away instead of deprecating it.

[hlundin-webrtc, 2016-11-30 11:24:35]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-11-30 11:24:36]

LG. I have a few comments.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
I know we usually don't allow other than plain mono and stereo, but is there
anything particular to _this_ method that requires this limitation. Can we not
add two 5-channel frames?

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:36: bool noPrevData = false;
no_previous_data

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:39: // special case we have no
data to start with
Capital start and end with '.'

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:188: if ((frame->num_channels_ >
2) || (frame->num_channels_ < 1))
Again, does this function really need the restriction to one or two channels?

[kwiberg-webrtc, 2016-11-30 11:32:44]

lgtm

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:30:
RTC_DCHECK_LT(result_frame->num_channels_, 3u);
On 2016/11/30 10:11:48, aleloi wrote:
> On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> > You don't need the "u"s anymore!
> 
> Done. Has the implementation of the DCHECK macros changed? (Hurray for that in
> that case!)

Yes. (I sent a mail about it yesterday to discuss-webrtc.)

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:236: }
On 2016/11/30 10:11:48, aleloi wrote:
> On 2016/11/30 09:38:34, kwiberg-webrtc wrote:
> > Instead of defining this function, use rtc::saturated_cast<int16_t>.
> 
> Done. Great that we have saturated_cast! I missed that.

Except for the name---it should have been "saturating_cast"...

> I checked that it does the same thing as ClampToInt16 by running 
> ClampToInt16.TestCases from audio_frame_operations_unittest.cc on it. I also
> replaced the ClampToInt16 copy in module_common_types with saturated_cast.
That
> way we can remove an anonymous namespace in a header, and delete the method
> right away instead of deprecating it.

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:674: 
Theoretically, this is part of the public API, but we can always hope that no
one was using it...

[aleloi, 2016-11-30 11:33:39]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
On 2016/11/30 11:24:36, hlundin-webrtc wrote:
> I know we usually don't allow other than plain mono and stereo, but is there
> anything particular to _this_ method that requires this limitation. Can we not
> add two 5-channel frames?

Not really. If there is a chance that poly-channel frames will become a reality,
I think we should remove the checks from functions that don't require it. There
are lots places where it is an assumption (e.g. the mixer).

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:188: if ((frame->num_channels_ >
2) || (frame->num_channels_ < 1))
On 2016/11/30 11:24:36, hlundin-webrtc wrote:
> Again, does this function really need the restriction to one or two channels?

No, there is no such assumption here either.

[kwiberg-webrtc, 2016-11-30 11:36:04]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
On 2016/11/30 11:33:39, aleloi wrote:
> On 2016/11/30 11:24:36, hlundin-webrtc wrote:
> > I know we usually don't allow other than plain mono and stereo, but is there
> > anything particular to _this_ method that requires this limitation. Can we
not
> > add two 5-channel frames?
> 
> Not really. If there is a chance that poly-channel frames will become a
reality,
> I think we should remove the checks from functions that don't require it.
There
> are lots places where it is an assumption (e.g. the mixer).

But there's no good reason to not remove the unnecessary checks right away, is
there?

[aleloi, 2016-11-30 11:48:34]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
I think methods should in general only check their preconditions. If they also
check all the things that are true for the system at the point of writing them,
there will be more to keep track of when functionality changes (more work to
make a change), and some old checks will probably be left when they shouldn't. I
don't think the benefit of tests like this is high enough.

[hlundin-webrtc, 2016-11-30 11:51:06]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
On 2016/11/30 11:36:04, kwiberg-webrtc wrote:
> On 2016/11/30 11:33:39, aleloi wrote:
> > On 2016/11/30 11:24:36, hlundin-webrtc wrote:
> > > I know we usually don't allow other than plain mono and stereo, but is
there
> > > anything particular to _this_ method that requires this limitation. Can we
> not
> > > add two 5-channel frames?
> > 
> > Not really. If there is a chance that poly-channel frames will become a
> reality,
> > I think we should remove the checks from functions that don't require it.
> There
> > are lots places where it is an assumption (e.g. the mixer).
> 
> But there's no good reason to not remove the unnecessary checks right away, is
> there?

[Logic evaluation stalled due to double negation]
[Attempting simplification]
["no good reason to not" -> "good reason to"]
[Substitute into original expression]
[-> "But there's good reason to remove the unnecessary checks right away, is
there?"]
[Ignoring positive question tag]
[-> "But there's good reason to remove the unnecessary checks right away"]
[Computer says: yes!]

[kwiberg-webrtc, 2016-11-30 11:55:04]

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/320001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29:
RTC_DCHECK_GT(result_frame->num_channels_, 0u);
On 2016/11/30 11:51:05, hlundin-webrtc wrote:
> On 2016/11/30 11:36:04, kwiberg-webrtc wrote:
> > On 2016/11/30 11:33:39, aleloi wrote:
> > > On 2016/11/30 11:24:36, hlundin-webrtc wrote:
> > > > I know we usually don't allow other than plain mono and stereo, but is
> there
> > > > anything particular to _this_ method that requires this limitation. Can
we
> > not
> > > > add two 5-channel frames?
> > > 
> > > Not really. If there is a chance that poly-channel frames will become a
> > reality,
> > > I think we should remove the checks from functions that don't require it.
> > There
> > > are lots places where it is an assumption (e.g. the mixer).
> > 
> > But there's no good reason to not remove the unnecessary checks right away,
is
> > there?
> 
> [Logic evaluation stalled due to double negation]
> [Attempting simplification]
> ["no good reason to not" -> "good reason to"]
> [Substitute into original expression]
> [-> "But there's good reason to remove the unnecessary checks right away, is
> there?"]
> [Ignoring positive question tag]
> [-> "But there's good reason to remove the unnecessary checks right away"]
> [Computer says: yes!]

Yes, that's what I meant. I (mis-)interpreted Alex's original reply as saying
that we should remove the checks later. But it seems like we all agree that the
checks should (at least in principle) be removed immediately.

[aleloi, 2016-11-30 19:05:06]

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

[commit-bot: I haz the power, 2016-11-30 19:05:14]

Dry run: 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, 2016-11-30 19:11:04]

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

[commit-bot: I haz the power, 2016-11-30 19:11:05]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10898)

[aleloi, 2016-12-01 09:10:41]

Hi! The build issues are fixed and linux-internal is green. Can we proceed to
land this in a few days? There are lots of files affected and I get merge
conflicts trying to keep it on top all the time </complaint>

[kwiberg-webrtc, 2016-12-01 09:22:57]

On 2016/12/01 09:10:41, aleloi wrote:
> Hi! The build issues are fixed and linux-internal is green. Can we proceed to
> land this in a few days? There are lots of files affected and I get merge
> conflicts trying to keep it on top all the time </complaint>

You already have my lgtm. Henrik?

[hlundin-webrtc, 2016-12-01 09:44:51]

lgtm with one nit

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:14: #include
"webrtc/modules/include/module_common_types.h"
Order of includes is wrong.

[the sun, 2016-12-01 20:32:46]

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:29: // Sanity check.
RTC_DCHECK(result_frame);

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:37: if
(result_frame->samples_per_channel_ != frame_to_add.samples_per_channel_) {
Could this be a DCHECK instead?

It seems what we allow is that result_frame->samples_per_channel is 0 or that
the frames are equal. If not, we silently return, which will allow potential
problems to go unnoticed.

The simplest API would be to only allow frames of identical size to be added,
but I'd be ok with the above DCHECK.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:48: frame_to_add.vad_activity_ ==
result_frame->kVadActive) {
result_frame->kVadActive should be AudioFrame::kVadActive.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:121: void
AudioFrameOperations::SwapStereoChannels(AudioFrame* frame) {
RTC_DCHECK(frame);

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:122: if (frame->num_channels_ !=
2)
nit: it looks like code previously in this file uses {} for one-liners - make
sure the new code does too, for consistency

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:192: frame->data_[i] =
static_cast<int16_t>(frame->data_[i] >> 1);
Why the need to cast an int16_t to int16_t?

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:14: #include
"webrtc/modules/include/module_common_types.h"
Please put back the forward declaration of AudioFrame instead. 

See: 
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:78: int16_t ClampToInt16(int32_t
input);
Is this unused? I don't find the definition.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations_unittest.cc:15: 
Are there really no tests for the "new" functions that we can move over here?

Since you change the impl of Add() quite a bit, adding a few test cases would be
rather handsome.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:645: "../audio/utility:audio_frame_operations",
isn't this pulled in as a transitive dep, via audio_mixer and
audio_conference_mixer?

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (left):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:588: Reset();
Why isn't this needed anymore?

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/utility/DEPS
File webrtc/modules/utility/DEPS (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/utility/D...
webrtc/modules/utility/DEPS:2: "+webrtc/audio/utility/audio_frame_operations.h",
 # TODO(aleloi):
Make the comment a single line above instead?

[aleloi, 2016-12-02 12:13:36]

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:37: if
(result_frame->samples_per_channel_ != frame_to_add.samples_per_channel_) {
On 2016/12/01 20:32:45, the sun wrote:
> Could this be a DCHECK instead?
> 
> It seems what we allow is that result_frame->samples_per_channel is 0 or that
> the frames are equal. If not, we silently return, which will allow potential
> problems to go unnoticed.
> 
> The simplest API would be to only allow frames of identical size to be added,
> but I'd be ok with the above DCHECK.

This function is called with samples_per_channel_==0 from the mixers (both new
and old). I've added DCHECKs.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:48: frame_to_add.vad_activity_ ==
result_frame->kVadActive) {
On 2016/12/01 20:32:46, the sun wrote:
> result_frame->kVadActive should be AudioFrame::kVadActive.

Thanks! Done.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:122: if (frame->num_channels_ !=
2)
On 2016/12/01 20:32:46, the sun wrote:
> nit: it looks like code previously in this file uses {} for one-liners - make
> sure the new code does too, for consistency

Done.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:192: frame->data_[i] =
static_cast<int16_t>(frame->data_[i] >> 1);
On 2016/12/01 20:32:46, the sun wrote:
> Why the need to cast an int16_t to int16_t?

That's strange. Removed. I've not removed this same cast in operator>>=: if I
fix this, I should fix everything else, and we're deprecating the methods
anyway.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:14: #include
"webrtc/modules/include/module_common_types.h"
On 2016/12/01 20:32:46, the sun wrote:
> Please put back the forward declaration of AudioFrame instead. 
> 
> See: 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Wow, didn't know that! Done.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:78: int16_t ClampToInt16(int32_t
input);
On 2016/12/01 20:32:46, the sun wrote:
> Is this unused? I don't find the definition.

I've replaced it with saturated_cast. Now removed.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations_unittest.cc:15: 
On 2016/12/01 20:32:46, the sun wrote:
> Are there really no tests for the "new" functions that we can move over here?
> 
> Since you change the impl of Add() quite a bit, adding a few test cases would
be
> rather handsome.

Nothing in module_common_types_unittests. Can't find anything anywhere else
either. I've written two simple tests for the addition.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:645: "../audio/utility:audio_frame_operations",
On 2016/12/01 20:32:46, the sun wrote:
> isn't this pulled in as a transitive dep, via audio_mixer and
> audio_conference_mixer?

Another miss by me: that's not actually a dependency anymore, since I moved the
unittest to the other audio tests. Now removed!

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (left):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:588: Reset();
On 2016/12/01 20:32:46, the sun wrote:
> Why isn't this needed anymore?

Oh! That's another issue you spotted! Originally, I added default member
initializers for all the members (which made Reset unnecessary). Then (one month
later) I changed module_common_types back to contain the deleted operators,
because it would break the internal build otherwise. The default member
initializers were changed too, because I didn't look closely enough on the diff.

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/utility/DEPS
File webrtc/modules/utility/DEPS (right):

https://codereview.webrtc.org/2424173003/diff/380001/webrtc/modules/utility/D...
webrtc/modules/utility/DEPS:2: "+webrtc/audio/utility/audio_frame_operations.h",
 # TODO(aleloi):
On 2016/12/01 20:32:46, the sun wrote:
> Make the comment a single line above instead?

Done.

[the sun, 2016-12-02 13:39:55]

lgtm % comments

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:11: #include <algorithm>
order!

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:44: if
(result_frame->samples_per_channel_ == 0) {
No need for this conditional handling now that you've asserted the precondition.
This is internal code - we don't need belt+suspenders - one will suffice. :)

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:193: void
AudioFrameOperations::ApplyHalfGain(AudioFrame* frame) {
DCHECK(frame) since you deref it, not just pass on

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:14: #include <stddef.h>
For size_t? But system include should still go after project includes.

[aleloi, 2016-12-05 09:18:06]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.

BUG=webrtc:6548
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
==========

[aleloi, 2016-12-05 09:18:37]

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.cc (right):

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:11: #include <algorithm>
On 2016/12/02 13:39:55, the sun wrote:
> order!

Done.

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:44: if
(result_frame->samples_per_channel_ == 0) {
On 2016/12/02 13:39:55, the sun wrote:
> No need for this conditional handling now that you've asserted the
precondition.
> This is internal code - we don't need belt+suspenders - one will suffice. :)

Done.

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.cc:193: void
AudioFrameOperations::ApplyHalfGain(AudioFrame* frame) {
On 2016/12/02 13:39:55, the sun wrote:
> DCHECK(frame) since you deref it, not just pass on

Done.

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
File webrtc/audio/utility/audio_frame_operations.h (right):

https://codereview.webrtc.org/2424173003/diff/400001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations.h:14: #include <stddef.h>
On 2016/12/02 13:39:55, the sun wrote:
> For size_t? But system include should still go after project includes.

Not according to the style guide...

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[aleloi, 2016-12-05 09:19:07]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-12-05 09:19:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2424173003/#ps420001 (title:
"Include order & DCHECKs.")

[commit-bot: I haz the power, 2016-12-05 09:19: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, 2016-12-05 09:22:41]

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

[commit-bot: I haz the power, 2016-12-05 09:22:42]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11057)

[aleloi, 2016-12-05 09:24:22]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True
==========

[aleloi, 2016-12-05 09:24:30]

The CQ bit was checked by aleloi@webrtc.org

[commit-bot: I haz the power, 2016-12-05 09:24:42]

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, 2016-12-05 09:46:11]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1480929870247000,
"parent_rev": "00e2f90e1e1bc10b81c0cfaf3c11ff65c8ea8eae", "commit_rev":
"a2b1c24054f996264972faa560e4f7f36dc61861"}

[commit-bot: I haz the power, 2016-12-05 09:46:15]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True
==========

[commit-bot: I haz the power, 2016-12-05 09:46:16]

Committed patchset #8 (id:420001)

[commit-bot: I haz the power, 2016-12-05 09:46:28]

Description was changed from

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations, 
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.


TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True
==========

to

==========
Move functionality out from AudioFrame and into AudioFrameOperations.

This CL is in preparation to move the AudioFrame into webrtc/api. The
AudioFrame is a POD type used for representing 10ms of audio. It
appears as a parameter and return value of interfaces being migrated
to webrtc/api, in particular AudioMixer.

Here, methods operator+=, operator>>=, Mute are
moved into a new target webrtc/audio/utility/audio_frame_operations,
and dependencies are changed to use
the new versions. The old AudioFrame methods are marked deprecated.

The audio frame utilities in webrtc/modules/utility:audio_frame_operations
are also moved to the new location.

TBR=kjellander@webrtc.org
BUG=webrtc:6548
NOPRESUBMIT=True

Committed: https://crrev.com/6321b49a0da3ebdeede1430aa9bc825855646b79
Cr-Commit-Position: refs/heads/master@{#15413}
==========

[commit-bot: I haz the power, 2016-12-05 09:46:29]

Patchset 8 (id:??) landed as
https://crrev.com/6321b49a0da3ebdeede1430aa9bc825855646b79
Cr-Commit-Position: refs/heads/master@{#15413}