Style changes in Audio Mixer

This removes forward declarations, changes include order, changes integers to plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True

Committed: https://crrev.com/dc7669a8a62c0fe3d5ddb7069be637dadf6a3740
Cr-Commit-Position: refs/heads/master@{#14494}

[aleloi, 2016-08-31 15:44:38]

Patchset #3 (id:40001) has been deleted

[aleloi, 2016-09-01 09:28:09]

Patchset #3 (id:60001) has been deleted

[aleloi, 2016-09-01 09:28:22]

Patchset #2 (id:20001) has been deleted

[aleloi, 2016-09-01 09:28:37]

Patchset #1 (id:1) has been deleted

[aleloi, 2016-09-01 13:23:12]

Patchset #1 (id:80001) has been deleted

[aleloi, 2016-09-01 13:23:21]

Patchset #1 (id:100001) has been deleted

[aleloi, 2016-09-01 13:56:48]

Patchset #1 (id:120001) has been deleted

[aleloi, 2016-09-01 13:56:55]

Patchset #1 (id:140001) has been deleted

[aleloi, 2016-09-01 14:05:21]

Patchset #4 (id:220001) has been deleted

[aleloi, 2016-09-01 14:12:20]

aleloi@webrtc.org changed reviewers:
+ ossu@webrtc.org

[aleloi, 2016-09-01 14:12:22]

A few relatively simple changes to make the audio mixer follow the style guide:

Removed forward declarations, changed a static member into a non-member (it
wasn't closely related to specific instances), changed integer variables to
plain 'int', and changed the includes and their order.

I hope I didn't miss anything from the style guide!

Please take a look!

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.h (left):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.h:17: class AudioFrame;
Import instead of forward declaration.

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer.h (left):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:21: class MixerAudioSource;
Was not needed, since we include audio_mixer_defines anyway.

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:46: }  // namespace webrtc
Implementation moved here from audio_mixer_impl.cc

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.h (right):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.h:41: };
Moved here from audio_mixer_impl.h

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.h:71:
std::unique_ptr<NewMixHistory> mix_history_;
Simplifying change suggested by kwiberg@

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:21: #include
"webrtc/system_wrappers/include/critical_section_wrapper.h"
Shouldn't include headers in .cc file that are duplicated in accompanying .h
file

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:18: #include
"webrtc/modules/audio_mixer/audio_mixer_defines.h"
Duplicate between .cc and .h

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (left):

https://codereview.webrtc.org/2302483002/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.h:26: class CriticalSectionWrapper;
Forward declaration replaced by imports.

https://codereview.webrtc.org/2302483002/diff/180001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (right):

https://codereview.webrtc.org/2302483002/diff/180001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.h:22: #include
"webrtc/modules/audio_mixer/audio_mixer_defines.h"
Should import things we are using and not rely on them being imported by other
headers

https://codereview.webrtc.org/2302483002/diff/180001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.h:88: 
Static member changed to non-member in .cc file (it was not clearly connected to
specific instance).

https://codereview.webrtc.org/2302483002/diff/180001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (left):

https://codereview.webrtc.org/2302483002/diff/180001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:18: #include
"webrtc/modules/audio_mixer/audio_mixer.h"
Include order.

[aleloi, 2016-09-01 14:14:16]

Description was changed from

==========
Tried to make mixer conform to style guide.
==========

to

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.
==========

[ossu, 2016-09-01 15:42:50]

Had a quick glance over it. Not sure if changing size_t -> int is strictly
necessary everywhere. The style guide seems a bit split on the issue: It says to
not use unsigned just because there won't be any negative values, but it also
says size_t is fine, but it also says to prefer int.

Maybe using size_t in parameters and return values is useful to indicate that
"this is a count of some sort, it cannot be negative" but that one shouldn't use
unsigned int in actual code just because one expects the value to never be
negative.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer.gypi (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.gypi:26: 'audio_mixer_defines.cc',
A nitpick: put the .cc file above the .h file to retain alphabetic ordering,
like with the other files.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.h (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.h:22: class NewMixHistory {
Why isn't these methods directly in MixerAudioSource?
It seems they were PIMPL'd before, which seems a bit odd to me for such simple
functionality. I think you should probably just move them into MixerAudioSource
unless there's a great need to hide them; in which case NewMixHistory shouldn't
be exposed like this anyways.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:138: AudioMixerImpl* mixer = new
AudioMixerImpl(id);
This is a bit strange. Not sure it's 100% related to this CL, but you should be
able to do:

std::unique_ptr<AudioMixerImpl> mixer(new AudioMixerImpl(id));
if (!mixer->Init()) {
  return nullptr;
}
return mixer;

and avoid manual memory management through delete.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:301: int num_mixed_non_anonymous
= static_cast<int>(audio_source_list_.size());
Why not just keep this as size_t? According to the google style guide, size_t is
fine. Maybe the chromium one overrides that?

[aleloi, 2016-09-02 11:52:34]

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer.gypi (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.gypi:26: 'audio_mixer_defines.cc',
On 2016/09/01 15:42:50, ossu wrote:
> A nitpick: put the .cc file above the .h file to retain alphabetic ordering,
> like with the other files.

Done.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.h (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.h:22: class NewMixHistory {
On 2016/09/01 15:42:50, ossu wrote:
> Why isn't these methods directly in MixerAudioSource?
> It seems they were PIMPL'd before, which seems a bit odd to me for such simple
> functionality. I think you should probably just move them into
MixerAudioSource
> unless there's a great need to hide them; in which case NewMixHistory
shouldn't
> be exposed like this anyways.

Agree. Moved to MixerAudioSource.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:138: AudioMixerImpl* mixer = new
AudioMixerImpl(id);
On 2016/09/01 15:42:50, ossu wrote:
> This is a bit strange. Not sure it's 100% related to this CL, but you should
be
> able to do:
> 
> std::unique_ptr<AudioMixerImpl> mixer(new AudioMixerImpl(id));
> if (!mixer->Init()) {
>   return nullptr;
> }
> return mixer;
> 
> and avoid manual memory management through delete.

Changed in upstream CL.

https://codereview.webrtc.org/2302483002/diff/240001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:301: int num_mixed_non_anonymous
= static_cast<int>(audio_source_list_.size());
On 2016/09/01 15:42:50, ossu wrote:
> Why not just keep this as size_t? According to the google style guide, size_t
is
> fine. Maybe the chromium one overrides that?

Thanks! I didn't read it carefully enough. Changing num_mixed_audio_sources_
back to size_t seems more reasonable. It's a count and we avoid two explicit
casts.

[ossu, 2016-09-05 12:02:54]

https://codereview.webrtc.org/2302483002/diff/260001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc (right):

https://codereview.webrtc.org/2302483002/diff/260001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/test/audio_mixer_unittest.cc:294:
EXPECT_EQ(static_cast<size_t>(number_of_channels),
I believe you should be able to use one of int or size_t throughout AudioMixer.
There should be no reason to cast between them unless you need to interface with
something external that is outside of your control (at least right now, in these
CLs). I believe I prefer size_t throughout for channel counts.

[aleloi, 2016-10-03 09:38:39]

Patchset #6 (id:280001) has been deleted

[aleloi, 2016-10-03 09:38:48]

Patchset #6 (id:300001) has been deleted

[aleloi, 2016-10-03 09:38:56]

Patchset #6 (id:320001) has been deleted

[aleloi, 2016-10-03 09:39:06]

Patchset #6 (id:340001) has been deleted

[aleloi, 2016-10-03 09:44:44]

Patchset #6 (id:360001) has been deleted

[aleloi, 2016-10-03 09:53:39]

Hi! Sorry for waiting this long with this. This CL contained a few style changes
in the mixer. I have gone through your comments and made small changes. The last
main change is to keep channel counts 'size_t'. Again, sorry for waiting a month
with this :)

[ossu, 2016-10-03 12:49:01]

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:23: bool
MixerAudioSource::WasMixed() const {
If WasMixed is the same as IsMixed, then why is there a WasMixed?
Or is this not part of this CL, but rather pulled in through a rebase? Still,
the question stands.

[aleloi, 2016-10-03 13:23:38]

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:23: bool
MixerAudioSource::WasMixed() const {
On 2016/10/03 12:49:01, ossu wrote:
> If WasMixed is the same as IsMixed, then why is there a WasMixed?
> Or is this not part of this CL, but rather pulled in through a rebase? Still,
> the question stands.

There were two functions that did the same thing since the old mixer. It's for
abstraction and readability. IsMixed() is used after the mixing status is
updated in a certain iteration. WasMixed() is used before the update.

I noticed the comment is old. I've changed it to 'AudioMixerImpl'.

[aleloi, 2016-10-03 13:24:31]

Patchset #7 (id:400001) has been deleted

[ossu, 2016-10-04 09:00:06]

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:23: bool
MixerAudioSource::WasMixed() const {
On 2016/10/03 13:23:38, aleloi wrote:
> On 2016/10/03 12:49:01, ossu wrote:
> > If WasMixed is the same as IsMixed, then why is there a WasMixed?
> > Or is this not part of this CL, but rather pulled in through a rebase?
Still,
> > the question stands.
> 
> There were two functions that did the same thing since the old mixer. It's for
> abstraction and readability. IsMixed() is used after the mixing status is
> updated in a certain iteration. WasMixed() is used before the update.
> 
> I noticed the comment is old. I've changed it to 'AudioMixerImpl'.

This doesn't make sense. IIUC, they will always return the same thing. Will this
change? Do they have different names only to sound right grammatically? Or is
this just how it's going to be for now, and this will change in a later CL?

[aleloi, 2016-10-04 09:15:18]

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:23: bool
MixerAudioSource::WasMixed() const {
On 2016/10/04 09:00:05, ossu wrote:
> On 2016/10/03 13:23:38, aleloi wrote:
> > On 2016/10/03 12:49:01, ossu wrote:
> > > If WasMixed is the same as IsMixed, then why is there a WasMixed?
> > > Or is this not part of this CL, but rather pulled in through a rebase?
> Still,
> > > the question stands.
> > 
> > There were two functions that did the same thing since the old mixer. It's
for
> > abstraction and readability. IsMixed() is used after the mixing status is
> > updated in a certain iteration. WasMixed() is used before the update.
> > 
> > I noticed the comment is old. I've changed it to 'AudioMixerImpl'.
> 
> This doesn't make sense. IIUC, they will always return the same thing. Will
this
> change? Do they have different names only to sound right grammatically? Or is
> this just how it's going to be for now, and this will change in a later CL?

Yes, the functions will always return the same thing. There hasn't been any
plans to change it. Now that you pointed it out I also find it a bit confusing.

[ossu, 2016-10-04 09:28:44]

lgtm

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_defines.cc (right):

https://codereview.webrtc.org/2302483002/diff/380001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_defines.cc:23: bool
MixerAudioSource::WasMixed() const {
On 2016/10/04 09:15:18, aleloi wrote:
> On 2016/10/04 09:00:05, ossu wrote:
> > On 2016/10/03 13:23:38, aleloi wrote:
> > > On 2016/10/03 12:49:01, ossu wrote:
> > > > If WasMixed is the same as IsMixed, then why is there a WasMixed?
> > > > Or is this not part of this CL, but rather pulled in through a rebase?
> > Still,
> > > > the question stands.
> > > 
> > > There were two functions that did the same thing since the old mixer. It's
> for
> > > abstraction and readability. IsMixed() is used after the mixing status is
> > > updated in a certain iteration. WasMixed() is used before the update.
> > > 
> > > I noticed the comment is old. I've changed it to 'AudioMixerImpl'.
> > 
> > This doesn't make sense. IIUC, they will always return the same thing. Will
> this
> > change? Do they have different names only to sound right grammatically? Or
is
> > this just how it's going to be for now, and this will change in a later CL?
> 
> Yes, the functions will always return the same thing. There hasn't been any
> plans to change it. Now that you pointed it out I also find it a bit
confusing.

Since this is only a style related CL, I'm not going to press it further, but
something seems very strange if the exact same functionality needs two
different, but similar, names.

[aleloi, 2016-10-04 09:38:58]

Description was changed from

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.
==========

to

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True
==========

[aleloi, 2016-10-04 10:57:54]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-10-04 10:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2302483002/#ps440001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-10-04 10:57:59]

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-10-04 11:06:23]

Description was changed from

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True
==========

to

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True
==========

[commit-bot: I haz the power, 2016-10-04 11:06:24]

Committed patchset #8 (id:440001)

[commit-bot: I haz the power, 2016-10-04 11:06:35]

Description was changed from

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True
==========

to

==========
This removes forward declarations, changes include order, changes integers to
plain 'int', and changes static methods to non-members.

BUG=6346
NOTRY=True

Committed: https://crrev.com/dc7669a8a62c0fe3d5ddb7069be637dadf6a3740
Cr-Commit-Position: refs/heads/master@{#14494}
==========

[commit-bot: I haz the power, 2016-10-04 11:06:37]

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