Add mute state field to AudioFrame.

Add mute state field to AudioFrame and switch some callers to use it. Also make AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field, i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial to do so. Other improvements requiring more significant structural changes will come later.

BUG=webrtc:7343
TBR=henrika

Review-Url: https://codereview.webrtc.org/2750783004
Cr-Commit-Position: refs/heads/master@{#18543}
Committed: https://chromium.googlesource.com/external/webrtc/+/36b1a5fcec6270ec4a5bea87b33a49b418a3cb29

[yujo, 2017-03-15 01:22:52]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 01:23:00]

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, 2017-03-15 01:24:49]

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

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

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18572)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23856)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22640)

[yujo, 2017-03-15 01:29:44]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 01:29:49]

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, 2017-03-15 01:33:17]

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

[commit-bot: I haz the power, 2017-03-15 01:33:18]

Dry run: Try jobs failed on following builders:
  android_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  linux_arm on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/6763)
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/24197)

[yujo, 2017-03-16 02:44:30]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 02:44:35]

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

[yujo, 2017-03-16 02:45:47]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 02:45:55]

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, 2017-03-16 02:55:25]

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

[commit-bot: I haz the power, 2017-03-16 02:55:26]

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

[yujo, 2017-03-16 06:47:05]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 06:47:10]

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, 2017-03-16 06:51:21]

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

[commit-bot: I haz the power, 2017-03-16 06:51:22]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/17345)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/24359)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/23778)

[yujo, 2017-03-16 08:10:57]

Description was changed from

==========
Add mute state field to AudioFrame.

BUG=webrtc:7343
==========

to

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.

mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently muted,
sets muted=false, and returns AudioFrame::data_.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
==========

[yujo, 2017-03-16 08:10:58]

yujo@chromium.org changed reviewers:
+ henrik.lundin@webrtc.org, solenberg@webrtc.org

[yujo, 2017-03-16 08:25:13]

Hi - this is the change we were discussing last week. Not fully finished, but
wanted to get some comments first.

Regarding data() and mutable_data() - how do you feel about this? The purpose is
to "force" callers to be aware of the muted field. It is a little intrusive
though.

If it's ok - how should we go about deprecating direct usage of data_? (I saw a
warning in the presubmit.)

Also, not building on windows yet.

[yujo, 2017-03-16 08:28:35]

Description was changed from

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.

mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently muted,
sets muted=false, and returns AudioFrame::data_.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
==========

to

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently
muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field,
i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
==========

[hlundin-webrtc, 2017-03-16 14:47:48]

First of all, thanks for your effort!

I started reviewing from the top, but half-way through the NetEq changes, I
realized that we should maybe discuss the central changes to
module_common_types.h first. See my comments inline.

Thanks!

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/api/call/audio_sink.h
File webrtc/api/call/audio_sink.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/api/call/audio_sin...
webrtc/api/call/audio_sink.h:30: Data(const int16_t* data,
This file is in our set of public API files, which usually requires a PSA, a
waiting period, and then doing the change.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
File webrtc/audio/audio_transport_proxy.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
webrtc/audio/audio_transport_proxy.cc:28: // TODO(yujo): for muted input frames,
don't resample. Either 1) allow
I would suggest that this function simply writes the correct number of zeros to
destination if frame is muted.

EDIT: This might not work, since you would then not be priming the resampler
filters with the correct data (i.e., it will be missing out on the zero
samples). I will have to think about his.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:520: }
It would be great if you could add some tests for the operations on muted
frames, too.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:349: int UpMix(const
AudioFrame& frame, size_t length_out_buff, int16_t* out_buff) {
Add special case for frame.muted() here too, or drop it in DownMix. I think in
practice we will never have a muted frame in the send chain, so optimization
here is not likely to be worth the extra code.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:747: // TODO(yujo): Skip
encode of muted frames.
See above: I don't think we will have muted input frames, right?

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:312: const int16_t* data() const;
This is probably going to break all the work you've done so far, but I wonder if
we actually would like to make this return an rtc::ArrayView<const int16_t> (and
maybe an rtc::ArrayView<int16_t> below, but that poses even more problems).
Pros:
+ The actual length of the data can be included in the type. No need to look at
samples per channel.
+ Modern C++ things like range-based for loops work out of the box.
Cons:
- Breaks even more code.
- Might result in asymmetric APIs if both of these cannot be made to return
ArrayViews.

The problem with mutable_data is that the caller might want to change the number
of samples (like when upsampling). This does not work with the ArrayView design.


Another alternative is to change the member data_ to be an rtc::Buffer<int16_t>.
Then data() can return a const ref/ptr (or just as easily an rtc::ArrayView),
and mutable_data() can return a pointer to the Buffer object. The caller can
then change the size when needed.

I realize that this really increases the scope of this change, and I'd rather
not force this on you.

WDYT?

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:347: static const int16_t
kEmptyData[kMaxDataSizeSamples] = {};
Nit: I think an explicit {0} would be more obvious to the casual reader (unless
there is a performance penalty to it).

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:354: public:
Hmmm. Two public sections...
I think you can safely declare this along with kMaxDataSizeSamples, and let it
be kMaxDataSizeSamples * 2.

Also, this thing with an enum for constants. We tend not to do this anymore. Use
a constexpr instead.

[hlundin-webrtc, 2017-03-16 14:58:23]

I'll come back to reviewing the rest of the files tomorrow.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:312: const int16_t* data() const;
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> This is probably going to break all the work you've done so far, but I wonder
if
> we actually would like to make this return an rtc::ArrayView<const int16_t>
(and
> maybe an rtc::ArrayView<int16_t> below, but that poses even more problems).
> Pros:
> + The actual length of the data can be included in the type. No need to look
at
> samples per channel.
> + Modern C++ things like range-based for loops work out of the box.
> Cons:
> - Breaks even more code.
> - Might result in asymmetric APIs if both of these cannot be made to return
> ArrayViews.
> 
> The problem with mutable_data is that the caller might want to change the
number
> of samples (like when upsampling). This does not work with the ArrayView
design.
> 
> 
> Another alternative is to change the member data_ to be an
rtc::Buffer<int16_t>.
> Then data() can return a const ref/ptr (or just as easily an rtc::ArrayView),
> and mutable_data() can return a pointer to the Buffer object. The caller can
> then change the size when needed.
> 
> I realize that this really increases the scope of this change, and I'd rather
> not force this on you.
> 
> WDYT?

Actually, after discussions with Solenberg, I changed my mind. Let's not bother
with this now. Keep the changes pragmatic for now, and we will revisit the
design questions at a later stage. Feel free to disregard from my comment above.

[yujo, 2017-03-16 23:37:21]

Thanks for the quick review! Still need to do the tests, decide what we want to
do with the AudioFrame API, and figure out how to do deprecation.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/api/call/audio_sink.h
File webrtc/api/call/audio_sink.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/api/call/audio_sin...
webrtc/api/call/audio_sink.h:30: Data(const int16_t* data,
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> This file is in our set of public API files, which usually requires a PSA, a
> waiting period, and then doing the change.

Done.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
File webrtc/audio/audio_transport_proxy.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
webrtc/audio/audio_transport_proxy.cc:28: // TODO(yujo): for muted input frames,
don't resample. Either 1) allow
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> I would suggest that this function simply writes the correct number of zeros
to
> destination if frame is muted.
> 
> EDIT: This might not work, since you would then not be priming the resampler
> filters with the correct data (i.e., it will be missing out on the zero
> samples). I will have to think about his.

I think the correct answer is to push AudioFrame deeper, what do you think?
(Also sorry, the TODO made no sense. I didn't see that destination_sample_rate
was available as a local.)

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:520: }
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> It would be great if you could add some tests for the operations on muted
> frames, too.

Yep, planning to. Let me know whether you want to try the ArrayView approach
first?

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:349: int UpMix(const
AudioFrame& frame, size_t length_out_buff, int16_t* out_buff) {
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> Add special case for frame.muted() here too, or drop it in DownMix. I think in
> practice we will never have a muted frame in the send chain, so optimization
> here is not likely to be worth the extra code.

Done. Also noticed my bug above...

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:747: // TODO(yujo): Skip
encode of muted frames.
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> See above: I don't think we will have muted input frames, right?

Muted input is actually my motivation for this whole change:
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:312: const int16_t* data() const;
On 2017/03/16 14:58:23, hlundin-webrtc wrote:
> On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > This is probably going to break all the work you've done so far, but I
wonder
> if
> > we actually would like to make this return an rtc::ArrayView<const int16_t>
> (and
> > maybe an rtc::ArrayView<int16_t> below, but that poses even more problems).
> > Pros:
> > + The actual length of the data can be included in the type. No need to look
> at
> > samples per channel.
> > + Modern C++ things like range-based for loops work out of the box.
> > Cons:
> > - Breaks even more code.
> > - Might result in asymmetric APIs if both of these cannot be made to return
> > ArrayViews.
> > 
> > The problem with mutable_data is that the caller might want to change the
> number
> > of samples (like when upsampling). This does not work with the ArrayView
> design.
> > 
> > 
> > Another alternative is to change the member data_ to be an
> rtc::Buffer<int16_t>.
> > Then data() can return a const ref/ptr (or just as easily an
rtc::ArrayView),
> > and mutable_data() can return a pointer to the Buffer object. The caller can
> > then change the size when needed.
> > 
> > I realize that this really increases the scope of this change, and I'd
rather
> > not force this on you.
> > 
> > WDYT?
> 
> Actually, after discussions with Solenberg, I changed my mind. Let's not
bother
> with this now. Keep the changes pragmatic for now, and we will revisit the
> design questions at a later stage. Feel free to disregard from my comment
above.

I considered the ArrayView approach, but ran into the same issue with changing
the length. And using Buffer means allocations on resizes, which perhaps is best
avoided.

Maybe introduce a version of mutable_frame() that takes num_channels,
samples_per_channel, and sample_rate_hz? If we add the corresponding version of
Mute(), then we can hide those fields entirely. That would be manageable to do
in this change, if you think it's an acceptable solution.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:342: private:
There's a presubmit warning about this being a public header file. Is there any
clean way to do the deprecation here?

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:347: static const int16_t
kEmptyData[kMaxDataSizeSamples] = {};
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> Nit: I think an explicit {0} would be more obvious to the casual reader
(unless
> there is a performance penalty to it).

Done.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:354: public:
On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> Hmmm. Two public sections...
> I think you can safely declare this along with kMaxDataSizeSamples, and let it
> be kMaxDataSizeSamples * 2.

Yeah, made it a static_assert instead and moved the constant up top.

> Also, this thing with an enum for constants. We tend not to do this anymore.
Use
> a constexpr instead.

I did try, but for some reason the compiler treats it as a static variable
rather than a compile-time constant, which means linking fails because the
symbol is missing. It works with an out-of-class definition, but because this is
a header-only class and we don't get inline variables until C++17, that would
waste a few KB of binary size and runtime memory.

[hlundin-webrtc, 2017-03-17 14:29:39]

This is a very good change. Thanks!

All I have is a few minor comments.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
File webrtc/audio/audio_transport_proxy.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/audio_transp...
webrtc/audio/audio_transport_proxy.cc:28: // TODO(yujo): for muted input frames,
don't resample. Either 1) allow
On 2017/03/16 23:37:21, yujo wrote:
> On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > I would suggest that this function simply writes the correct number of zeros
> to
> > destination if frame is muted.
> > 
> > EDIT: This might not work, since you would then not be priming the resampler
> > filters with the correct data (i.e., it will be missing out on the zero
> > samples). I will have to think about his.
> 
> I think the correct answer is to push AudioFrame deeper, what do you think?
> (Also sorry, the TODO made no sense. I didn't see that destination_sample_rate
> was available as a local.)

I think the TODO now is good enough for this CL. I agree that we should push
AudioFrame deeper later on.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:747: // TODO(yujo): Skip
encode of muted frames.
On 2017/03/16 23:37:21, yujo wrote:
> On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > See above: I don't think we will have muted input frames, right?
> 
> Muted input is actually my motivation for this whole change:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....

Oh! Then I see. I though you were only in it to shave off complexity for muted
output channels.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:312: const int16_t* data() const;
On 2017/03/16 23:37:21, yujo wrote:
> On 2017/03/16 14:58:23, hlundin-webrtc wrote:
> > On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > > This is probably going to break all the work you've done so far, but I
> wonder
> > if
> > > we actually would like to make this return an rtc::ArrayView<const
int16_t>
> > (and
> > > maybe an rtc::ArrayView<int16_t> below, but that poses even more
problems).
> > > Pros:
> > > + The actual length of the data can be included in the type. No need to
look
> > at
> > > samples per channel.
> > > + Modern C++ things like range-based for loops work out of the box.
> > > Cons:
> > > - Breaks even more code.
> > > - Might result in asymmetric APIs if both of these cannot be made to
return
> > > ArrayViews.
> > > 
> > > The problem with mutable_data is that the caller might want to change the
> > number
> > > of samples (like when upsampling). This does not work with the ArrayView
> > design.
> > > 
> > > 
> > > Another alternative is to change the member data_ to be an
> > rtc::Buffer<int16_t>.
> > > Then data() can return a const ref/ptr (or just as easily an
> rtc::ArrayView),
> > > and mutable_data() can return a pointer to the Buffer object. The caller
can
> > > then change the size when needed.
> > > 
> > > I realize that this really increases the scope of this change, and I'd
> rather
> > > not force this on you.
> > > 
> > > WDYT?
> > 
> > Actually, after discussions with Solenberg, I changed my mind. Let's not
> bother
> > with this now. Keep the changes pragmatic for now, and we will revisit the
> > design questions at a later stage. Feel free to disregard from my comment
> above.
> 
> I considered the ArrayView approach, but ran into the same issue with changing
> the length. And using Buffer means allocations on resizes, which perhaps is
best
> avoided.
> 
> Maybe introduce a version of mutable_frame() that takes num_channels,
> samples_per_channel, and sample_rate_hz? If we add the corresponding version
of
> Mute(), then we can hide those fields entirely. That would be manageable to do
> in this change, if you think it's an acceptable solution.

Let's take this one step at a time, and skip the ArrayView for now. The
AudioFrame is slated for more refactoring in the future, so we'll have plenty of
opportunities to figure out great designs then.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:342: private:
On 2017/03/16 23:37:21, yujo wrote:
> There's a presubmit warning about this being a public header file. Is there
any
> clean way to do the deprecation here?

Yeah, this is going to be a bit of a pain. Let's finalize the CL first, and then
we can discuss offline how to go about landing this.

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:354: public:
On 2017/03/16 23:37:21, yujo wrote:
> On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > Hmmm. Two public sections...
> > I think you can safely declare this along with kMaxDataSizeSamples, and let
it
> > be kMaxDataSizeSamples * 2.
> 
> Yeah, made it a static_assert instead and moved the constant up top.
> 
> > Also, this thing with an enum for constants. We tend not to do this anymore.
> Use
> > a constexpr instead.
> 
> I did try, but for some reason the compiler treats it as a static variable
> rather than a compile-time constant, which means linking fails because the
> symbol is missing. It works with an out-of-class definition, but because this
is
> a header-only class and we don't get inline variables until C++17, that would
> waste a few KB of binary size and runtime memory.

Excellent!

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:885: EXPECT_EQ(0,
const_out_frame_data[i]);
Why did this change?

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
File webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc
(right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:45: if
(!audioFrame.muted())
I think I would prefer an early return when muted.
if (audioFrame.muted()) {
  return 0;
}

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:61: if
(!audioFrame.muted())
Early return.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:74: if
(!audioFrame.muted())
Early return.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:20: if
(!audio_frame.muted()) {
Early return 0 when muted.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/voice_engine/file_...
File webrtc/voice_engine/file_recorder.cc (left):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/voice_engine/file_...
webrtc/voice_engine/file_recorder.cc:166: // closest integer value. Note samples
can be either 1 or 2 byte.
This comment must be off, right? The code could not have worked for 1-byte
samples.

[yujo, 2017-03-17 23:55:55]

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
File webrtc/audio/utility/audio_frame_operations_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/audio/utility/audi...
webrtc/audio/utility/audio_frame_operations_unittest.cc:520: }
On 2017/03/16 23:37:21, yujo wrote:
> On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > It would be great if you could add some tests for the operations on muted
> > frames, too.
> 
> Yep, planning to. Let me know whether you want to try the ArrayView approach
> first?

Done.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:885: EXPECT_EQ(0,
const_out_frame_data[i]);
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> Why did this change?

I misread this (confused it for the block above), but it's because NetEqImpl now
mutes on sample underrun, which has the observable effect of zeroing the entire
frame. Previously, it only zeroed the first 160 samples.

I just deleted this entire loop - if the frame is 160 samples, then we shouldn't
care what's in the rest of the buffer.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
File webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc
(right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:45: if
(!audioFrame.muted())
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> I think I would prefer an early return when muted.
> if (audioFrame.muted()) {
>   return 0;
> }

Done.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:61: if
(!audioFrame.muted())
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> Early return.

Done.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_conf...
webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc:74: if
(!audioFrame.muted())
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> Early return.

Done.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:20: if
(!audio_frame.muted()) {
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> Early return 0 when muted.

Done.

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/voice_engine/file_...
File webrtc/voice_engine/file_recorder.cc (left):

https://codereview.webrtc.org/2750783004/diff/80001/webrtc/voice_engine/file_...
webrtc/voice_engine/file_recorder.cc:166: // closest integer value. Note samples
can be either 1 or 2 byte.
On 2017/03/17 14:29:39, hlundin-webrtc wrote:
> This comment must be off, right? The code could not have worked for 1-byte
> samples.

Yeah, I thought so too.

[yujo, 2017-03-17 23:56:12]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 23:56:18]

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, 2017-03-18 00:03:25]

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

[commit-bot: I haz the power, 2017-03-18 00:03:26]

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

[yujo, 2017-03-18 00:09:48]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 00:09:56]

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, 2017-03-18 00:19:45]

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

[commit-bot: I haz the power, 2017-03-18 00:19:46]

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

[yujo, 2017-03-19 19:07:10]

There is still a bug being caught by tests; I'll take a look on Monday.

[yujo, 2017-03-20 23:45:22]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 23:45:27]

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, 2017-03-21 00:13:21]

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

[commit-bot: I haz the power, 2017-03-21 00:13:22]

Dry run: This issue passed the CQ dry run.

[yujo, 2017-03-21 00:15:02]

On 2017/03/19 19:07:10, yujo wrote:
> There is still a bug being caught by tests; I'll take a look on Monday.

Tests are green now, it was a trivial mistake. The linux_tsan2 test is flaky but
not related.

[hlundin-webrtc, 2017-03-22 12:07:26]

Very nice! All I have now is a few minor comments and nits.

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

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations_unittest.cc:603:
ASSERT_TRUE(frame_.muted());
Nit: Move this ASSERT to just before the Add operation. Makes it easier to see
that frame_ is the muted one.

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

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:360: #if !defined(WEBRTC_WIN)
That's weird. What if you put the static_assert in the ctor body instead?

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
File webrtc/modules/module_common_types_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:47: EXPECT_TRUE(AllSamplesAre(0,
frame));
I think you can replace this with
EXPECT_THAT(frame.data(), ::testing::Each(0));

You will have to #include "webrtc/test/gtest.h"

This would make AllSamplesAre unused, making the AudioFrameTest class collapse
into just a collection of constants, which could just as well be in the unnamed
namespace. So you can turn your TEST_Fs into TESTs.

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:84: EXPECT_EQ(0,
std::memcmp(samples, frame.data(), sizeof(samples)));
Can you use
EXPECT_THAT(frame.data(), ::testing::ElementsAreArray(samples));
?

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:112: EXPECT_EQ(0,
std::memcmp(frame2.data(), frame1.data(), sizeof(samples)));
EXPECT_THAT(...)

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:120: EXPECT_EQ(0,
std::memcmp(frame2.data(), frame1.data(), sizeof(samples)));
EXPECT_THAT(...)

[yujo, 2017-03-22 19:45:45]

We're kind of back to the ArrayView discussion. As an in-between measure, would
you want to change data() and mutable_data() to return std::array&?

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

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/audio/utility/aud...
webrtc/audio/utility/audio_frame_operations_unittest.cc:603:
ASSERT_TRUE(frame_.muted());
On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> Nit: Move this ASSERT to just before the Add operation. Makes it easier to see
> that frame_ is the muted one.

Done.

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

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:360: #if !defined(WEBRTC_WIN)
On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> That's weird. What if you put the static_assert in the ctor body instead?

Done.

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
File webrtc/modules/module_common_types_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:47: EXPECT_TRUE(AllSamplesAre(0,
frame));
On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> I think you can replace this with
> EXPECT_THAT(frame.data(), ::testing::Each(0));
> 
> You will have to #include "webrtc/test/gtest.h"
> 
> This would make AllSamplesAre unused, making the AudioFrameTest class collapse
> into just a collection of constants, which could just as well be in the
unnamed
> namespace. So you can turn your TEST_Fs into TESTs.

Deleted the unnecessary test fixture.

data() returns int16_t* so we don't get any length information. Do you want to
change it to return std::array<int16_t, kMaxDataSizeSamples>&?

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:84: EXPECT_EQ(0,
std::memcmp(samples, frame.data(), sizeof(samples)));
On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> Can you use
> EXPECT_THAT(frame.data(), ::testing::ElementsAreArray(samples));
> ?

Yes here, but not in the other two cases below. Leaving as is unless you feel
strongly.

[yujo, 2017-03-22 20:01:03]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 20:01:11]

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, 2017-03-22 20:43:15]

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

[commit-bot: I haz the power, 2017-03-22 20:43:16]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-03-23 07:04:03]

LGTM from me!
Sterling work! Thank you.

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
File webrtc/modules/module_common_types_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:47: EXPECT_TRUE(AllSamplesAre(0,
frame));
On 2017/03/22 19:45:45, yujo wrote:
> On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> > I think you can replace this with
> > EXPECT_THAT(frame.data(), ::testing::Each(0));
> > 
> > You will have to #include "webrtc/test/gtest.h"
> > 
> > This would make AllSamplesAre unused, making the AudioFrameTest class
collapse
> > into just a collection of constants, which could just as well be in the
> unnamed
> > namespace. So you can turn your TEST_Fs into TESTs.
> 
> Deleted the unnecessary test fixture.
> 
> data() returns int16_t* so we don't get any length information. Do you want to
> change it to return std::array<int16_t, kMaxDataSizeSamples>&?

Don't change any more. This is good enough. I did not think straight about the
lack of length information.

https://codereview.webrtc.org/2750783004/diff/160001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:84: EXPECT_EQ(0,
std::memcmp(samples, frame.data(), sizeof(samples)));
On 2017/03/22 19:45:45, yujo wrote:
> On 2017/03/22 12:07:26, hlundin-webrtc wrote:
> > Can you use
> > EXPECT_THAT(frame.data(), ::testing::ElementsAreArray(samples));
> > ?
> 
> Yes here, but not in the other two cases below. Leaving as is unless you feel
> strongly.

Acknowledged.

[the sun, 2017-03-23 19:34:38]

Great job! A few comments, one for Henrik. :)

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/audio_trans...
File webrtc/audio/audio_transport_proxy.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/audio_trans...
webrtc/audio/audio_transport_proxy.cc:29: // handling of muted frames.
Note: That would mean the resampler needs to maintain the muted state also, and
possibly run for a while on zeroed input, to allow IR and filter state to
converge...

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:335: if
(frame.num_channels_ != 2 ||
Make this a DCHECK instead - it doesn't look like the return value from this
function is acted upon when tracing the calls up the tree.

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:342: out_buff[n] =
(frame_data[2 * n] + frame_data[2 * n + 1]) >> 1;
out of scope for this CL, but IIUC this risks wrapping?!

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:351: if
(frame.num_channels_ != 1 ||
DCHECK

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

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:286: // Stereo, 32 kHz, 60 ms (2 *
32 * 60)
hlundin@: Does this need to be changed with the move to 120ms packet sizes,
and/or 48kHz support?

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:469: if (rhs.muted()) return *this;
This is different from
https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/utility/aud...
- which doesn't early out but takes care to transfer vad_activity and update
speech type.

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/module_co...
File webrtc/modules/module_common_types_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:13: #include <cstring>  // memcmp
Use <string.h>

[yujo, 2017-03-24 07:30:15]

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/audio_trans...
File webrtc/audio/audio_transport_proxy.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/audio_trans...
webrtc/audio/audio_transport_proxy.cc:29: // handling of muted frames.
On 2017/03/23 19:34:38, the sun wrote:
> Note: That would mean the resampler needs to maintain the muted state also,
and
> possibly run for a while on zeroed input, to allow IR and filter state to
> converge...

I'm not clear on how the resampler works, but maybe it's best to let it work
through the muted frames normally, then.

I think the important thing is actually to have it take an audio frame and write
an audio frame, and to make sure that muted input always gives muted output.
Does that sound right?

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:335: if
(frame.num_channels_ != 2 ||
On 2017/03/23 19:34:38, the sun wrote:
> Make this a DCHECK instead - it doesn't look like the return value from this
> function is acted upon when tracing the calls up the tree.

Here's one major place where the result is used:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:342: out_buff[n] =
(frame_data[2 * n] + frame_data[2 * n + 1]) >> 1;
On 2017/03/23 19:34:38, the sun wrote:
> out of scope for this CL, but IIUC this risks wrapping?!

Fixed.

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

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:469: if (rhs.muted()) return *this;
On 2017/03/23 19:34:38, the sun wrote:
> This is different from
>
https://codereview.webrtc.org/2750783004/diff/180001/webrtc/audio/utility/aud...
> - which doesn't early out but takes care to transfer vad_activity and update
> speech type.

Done.

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/module_co...
File webrtc/modules/module_common_types_unittest.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/module_co...
webrtc/modules/module_common_types_unittest.cc:13: #include <cstring>  // memcmp
On 2017/03/23 19:34:38, the sun wrote:
> Use <string.h>

Done.

[yujo, 2017-03-24 07:30:19]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 07:30:21]

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-03-24 08:05:58]

LGTM % those two DCHECKs

Again, great job on this, thanks for jumping in!

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:335: if
(frame.num_channels_ != 2 ||
On 2017/03/24 07:30:14, yujo wrote:
> On 2017/03/23 19:34:38, the sun wrote:
> > Make this a DCHECK instead - it doesn't look like the return value from this
> > function is acted upon when tracing the calls up the tree.
> 
> Here's one major place where the result is used:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...

Well, tracing that upwards:
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....
then
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/voe_base...
or
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/transmit...
or
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/transmit...

Please make it a DCHECK(). This is a logic error in the code.

I honestly don't know what the return code would ever be used for. Would we try
to recover by down mixing some other way? Log that we couldn't encode because
our code is broken and makes bad decisions? This is a typical example of when it
helps to aggressively state preconditions, and when defensively returning an
error (which ultimately goes unhandled), may potentially hide bugs.

[hlundin-webrtc, 2017-03-24 08:12:07]

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

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:286: // Stereo, 32 kHz, 60 ms (2 *
32 * 60)
On 2017/03/23 19:34:38, the sun wrote:
> hlundin@: Does this need to be changed with the move to 120ms packet sizes,
> and/or 48kHz support?

This is only used on 10ms frames, so the allocated size is enough also for 48
kHz stereo. But I do agree that the comment is misleading. This is a mess...

[commit-bot: I haz the power, 2017-03-24 08:42:06]

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

[commit-bot: I haz the power, 2017-03-24 08:42:07]

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

[yujo, 2017-03-24 17:00:27]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 17:00:34]

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

[yujo, 2017-03-24 17:10:32]

Thanks to both of you for the feedback! Let's talk offline about next steps.

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:335: if
(frame.num_channels_ != 2 ||
On 2017/03/24 08:05:58, the sun wrote:
> On 2017/03/24 07:30:14, yujo wrote:
> > On 2017/03/23 19:34:38, the sun wrote:
> > > Make this a DCHECK instead - it doesn't look like the return value from
this
> > > function is acted upon when tracing the calls up the tree.
> > 
> > Here's one major place where the result is used:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...
> 
> Well, tracing that upwards:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel....
> then
>
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/voe_base...
> or
>
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/transmit...
> or
>
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/transmit...
> 
> Please make it a DCHECK(). This is a logic error in the code.
> 
> I honestly don't know what the return code would ever be used for. Would we
try
> to recover by down mixing some other way? Log that we couldn't encode because
> our code is broken and makes bad decisions? This is a typical example of when
it
> helps to aggressively state preconditions, and when defensively returning an
> error (which ultimately goes unhandled), may potentially hide bugs.

Done. I meant that there was a difference in behavior as a result - in one case,
we would encode, and in the other, we wouldn't. But I see your point about the
layers above that.

https://codereview.webrtc.org/2750783004/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:351: if
(frame.num_channels_ != 1 ||
On 2017/03/23 19:34:38, the sun wrote:
> DCHECK

Done.

[commit-bot: I haz the power, 2017-03-24 17:21:38]

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

[commit-bot: I haz the power, 2017-03-24 17:21:40]

Dry run: This issue passed the CQ dry run.

[yujo, 2017-04-14 02:46:48]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 02:46:54]

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, 2017-04-14 03:40:09]

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

[commit-bot: I haz the power, 2017-04-14 03:40:10]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-05-10 08:38:28]

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2750783004/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:312: const int16_t* data() const;
On 2017/03/17 14:29:38, hlundin-webrtc wrote:
> On 2017/03/16 23:37:21, yujo wrote:
> > On 2017/03/16 14:58:23, hlundin-webrtc wrote:
> > > On 2017/03/16 14:47:48, hlundin-webrtc wrote:
> > > > This is probably going to break all the work you've done so far, but I
> > wonder
> > > if
> > > > we actually would like to make this return an rtc::ArrayView<const
> int16_t>
> > > (and
> > > > maybe an rtc::ArrayView<int16_t> below, but that poses even more
> problems).
> > > > Pros:
> > > > + The actual length of the data can be included in the type. No need to
> look
> > > at
> > > > samples per channel.
> > > > + Modern C++ things like range-based for loops work out of the box.
> > > > Cons:
> > > > - Breaks even more code.
> > > > - Might result in asymmetric APIs if both of these cannot be made to
> return
> > > > ArrayViews.
> > > > 
> > > > The problem with mutable_data is that the caller might want to change
the
> > > number
> > > > of samples (like when upsampling). This does not work with the ArrayView
> > > design.
> > > > 
> > > > 
> > > > Another alternative is to change the member data_ to be an
> > > rtc::Buffer<int16_t>.
> > > > Then data() can return a const ref/ptr (or just as easily an
> > rtc::ArrayView),
> > > > and mutable_data() can return a pointer to the Buffer object. The caller
> can
> > > > then change the size when needed.
> > > > 
> > > > I realize that this really increases the scope of this change, and I'd
> > rather
> > > > not force this on you.
> > > > 
> > > > WDYT?
> > > 
> > > Actually, after discussions with Solenberg, I changed my mind. Let's not
> > bother
> > > with this now. Keep the changes pragmatic for now, and we will revisit the
> > > design questions at a later stage. Feel free to disregard from my comment
> > above.
> > 
> > I considered the ArrayView approach, but ran into the same issue with
changing
> > the length. And using Buffer means allocations on resizes, which perhaps is
> best
> > avoided.
> > 
> > Maybe introduce a version of mutable_frame() that takes num_channels,
> > samples_per_channel, and sample_rate_hz? If we add the corresponding version
> of
> > Mute(), then we can hide those fields entirely. That would be manageable to
do
> > in this change, if you think it's an acceptable solution.
> 
> Let's take this one step at a time, and skip the ArrayView for now. The
> AudioFrame is slated for more refactoring in the future, so we'll have plenty
of
> opportunities to figure out great designs then.

(No need to change anything in this CL as a result of my comments, since I'm
very late to the party.)

I think data() should definitely return an ArrayView<const int16_t>. It's easier
to not make mistakes about the length of arrays if the pointer and the length
are always bundled in a standardized way. Callers that really want just the
pointer can say ".data().data()"...

For mutable data, using an rtc::Buffer wouldn't have to cause extra
allocations---just make sure to initialize it with a capacity of
kMaxDataSizeSamples and you're golden. So returning a Buffer<int16_t>* (or
possibly Buffer<int16_t>&) seems fine.

[yujo, 2017-05-11 21:08:23]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 21:08:33]

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, 2017-05-11 22:44:08]

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

[commit-bot: I haz the power, 2017-05-11 22:44:09]

Dry run: This issue passed the CQ dry run.

[yujo, 2017-06-09 23:42:53]

The CQ bit was checked by yujo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 23:42:56]

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, 2017-06-10 00:06:07]

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

[commit-bot: I haz the power, 2017-06-10 00:06:08]

Dry run: This issue passed the CQ dry run.

[yujo, 2017-06-12 19:07:45]

The CQ bit was checked by yujo@chromium.org

[yujo, 2017-06-12 19:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2750783004/#ps360001 (title:
"Update new usages of AudioFrame::data_")

[commit-bot: I haz the power, 2017-06-12 19:07:51]

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-06-12 19:12:31]

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

[commit-bot: I haz the power, 2017-06-12 19:12:35]

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

[yujo, 2017-06-12 19:16:32]

Description was changed from

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently
muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field,
i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
==========

to

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently
muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field,
i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
TBR=henrika
==========

[yujo, 2017-06-12 19:16:32]

yujo@chromium.org changed reviewers:
+ henrika@webrtc.org

[yujo, 2017-06-12 19:16:58]

The CQ bit was checked by yujo@chromium.org

[commit-bot: I haz the power, 2017-06-12 19:17:02]

CQ is trying da patch.

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

[yujo, 2017-06-12 19:19:20]

+henrika for the following files

Missing LGTM from an OWNER for these files:
    webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.cc
   
webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc
    webrtc/modules/include/module_common_types.h
    webrtc/modules/module_common_types_unittest.cc
    webrtc/tools/agc/activity_metric.cc

Thanks!

[commit-bot: I haz the power, 2017-06-12 19:45:33]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1497295018122430,
"parent_rev": "0703856b53e80159d666e289762406989f08ccfa", "commit_rev":
"36b1a5fcec6270ec4a5bea87b33a49b418a3cb29"}

[commit-bot: I haz the power, 2017-06-12 19:45:41]

Description was changed from

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently
muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field,
i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
TBR=henrika
==========

to

==========
Add mute state field to AudioFrame and switch some callers to use it. Also make
AudioFrame::data_ private and instead provide:

const int16_t* data() const;
int16_t* mutable_data();

- data() returns a zeroed static buffer on muted frames (to avoid unnecessary
zeroing of the member buffer) and directly returns AudioFrame::data_ on unmuted
frames.
- mutable_data(), lazily zeroes AudioFrame::data_ if the frame is currently
muted, sets muted=false, and returns AudioFrame::data_.

These accessors serve to "force" callers to be aware of the mute state field,
i.e. lazy zeroing is not the primary motivation.

This change only optimizes handling of muted frames where it is somewhat trivial
to do so. Other improvements requiring more significant structural changes will
come later.

BUG=webrtc:7343
TBR=henrika

Review-Url: https://codereview.webrtc.org/2750783004
Cr-Commit-Position: refs/heads/master@{#18543}
Committed:
https://chromium.googlesource.com/external/webrtc/+/36b1a5fcec6270ec4a5bea87b...
==========

[commit-bot: I haz the power, 2017-06-12 19:45:42]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/external/webrtc/+/36b1a5fcec6270ec4a5bea87b...