Removed TimeScheduler, Process() and TimeToNextProcess() from mixer

Removed Module inheritance and unused methods and members TimeScheduler, Process() and TimeToNextProcess() from mixer.

Committed: https://crrev.com/8b2233f5a6394461845310ca14c918b3ce9eb5ef
Cr-Commit-Position: refs/heads/master@{#13552}

[aleloi, 2016-07-01 09:25:05]

Description was changed from

==========
Removed TimeScheduler, Process() and TimeToNextProcess() from mixer

NOTRY=True
==========

to

==========
Removed Module inheritance and unused methods and members TimeScheduler,
Process() and TimeToNextProcess() from mixer. 

See the design doc:

https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2khw/

NOTRY=True
==========

[aleloi, 2016-07-01 09:54:56]

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

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

This is the least complex and most uncontroversial change to the mixer. I
removed some unused functionality.

[aleloi, 2016-07-01 11:32:50]

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

[commit-bot: I haz the power, 2016-07-01 11:32:52]

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

[ivoc, 2016-07-01 11:35:20]

Looks good, see some minor comments below.

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
File webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h (right):

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h:21: #include
"webrtc/modules/audio_conference_mixer/source/time_scheduler.h"
Is this include still needed?

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h:71: void
Mix(AudioFrame*) override;
Please add a name for the input variable (i.e. audio_frame_for_mixing)

[aleloi, 2016-07-01 11:40:49]

Description was changed from

==========
Removed Module inheritance and unused methods and members TimeScheduler,
Process() and TimeToNextProcess() from mixer. 

See the design doc:

https://docs.google.com/document/d/13WbsqJJXCOvS4Pq8lr2BmdQGtFWnvbVpvipOHVH2khw/

NOTRY=True
==========

to

==========
Removed Module inheritance and unused methods and members TimeScheduler,
Process() and TimeToNextProcess() from mixer.
==========

[aleloi, 2016-07-01 11:42:13]

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
File webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h (right):

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h:21: #include
"webrtc/modules/audio_conference_mixer/source/time_scheduler.h"
On 2016/07/01 11:35:20, ivoc wrote:
> Is this include still needed?

No, it's not. Thank you!

https://codereview.webrtc.org/2109333006/diff/1/webrtc/modules/audio_mixer/so...
webrtc/modules/audio_mixer/source/audio_conference_mixer_impl.h:71: void
Mix(AudioFrame*) override;
On 2016/07/01 11:35:20, ivoc wrote:
> Please add a name for the input variable (i.e. audio_frame_for_mixing)

Done.

[ivoc, 2016-07-04 13:11:52]

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
(left):

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:195:
assert(_processCalls == 0);
It seems like this code checks that this function is not called from multiple
threads, do you think it's useful to keep a check for that in the new code?

[aleloi, 2016-07-06 10:04:09]

Patchset #7 (id:120001) has been deleted

[aleloi, 2016-07-06 10:17:27]

Patchset #8 (id:160001) has been deleted

[aleloi, 2016-07-06 10:18:05]

Patchset #8 (id:180001) has been deleted

[aleloi, 2016-07-06 10:20:32]

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
(left):

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:195:
assert(_processCalls == 0);
On 2016/07/04 13:11:52, ivoc wrote:
> It seems like this code checks that this function is not called from multiple
> threads, do you think it's useful to keep a check for that in the new code?

Wow! This was important and I missed it. I thought for some reason that the mix
call counter was used to let the time scheduler know how many more calls should
be done.

I added it back in next the next patch sets.

[aleloi, 2016-07-06 11:03:51]

Patchset #8 (id:200001) has been deleted

[aleloi, 2016-07-06 11:27:22]

Patchset #8 (id:220001) has been deleted

[aleloi, 2016-07-06 11:27:34]

Patchset #7 (id:140001) has been deleted

[ivoc, 2016-07-06 14:43:26]

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
(left):

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:195:
assert(_processCalls == 0);
On 2016/07/06 10:20:32, aleloi wrote:
> On 2016/07/04 13:11:52, ivoc wrote:
> > It seems like this code checks that this function is not called from
multiple
> > threads, do you think it's useful to keep a check for that in the new code?
> 
> Wow! This was important and I missed it. I thought for some reason that the
mix
> call counter was used to let the time scheduler know how many more calls
should
> be done.
> 
> I added it back in next the next patch sets.

Although I agree that it's good to check for this, I'm not sure that the old
code is the best way to do this. In much of the rest of WebRTC code a
ThreadChecker is used for it, usually with a call to CalledOnValidThread()
inside a DCHECK (so it's only actually checked in debug builds). I don't really
see any reason not to use the same pattern here.

[aleloi, 2016-07-06 15:17:19]

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
(left):

https://codereview.webrtc.org/2109333006/diff/80001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:195:
assert(_processCalls == 0);
On 2016/07/06 14:43:26, ivoc wrote:
> On 2016/07/06 10:20:32, aleloi wrote:
> > On 2016/07/04 13:11:52, ivoc wrote:
> > > It seems like this code checks that this function is not called from
> multiple
> > > threads, do you think it's useful to keep a check for that in the new
code?
> > 
> > Wow! This was important and I missed it. I thought for some reason that the
> mix
> > call counter was used to let the time scheduler know how many more calls
> should
> > be done.
> > 
> > I added it back in next the next patch sets.
> 
> Although I agree that it's good to check for this, I'm not sure that the old
> code is the best way to do this. In much of the rest of WebRTC code a
> ThreadChecker is used for it, usually with a call to CalledOnValidThread()
> inside a DCHECK (so it's only actually checked in debug builds). I don't
really
> see any reason not to use the same pattern here.

Good idea! Done.

[aleloi, 2016-07-07 15:37:25]

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

[commit-bot: I haz the power, 2016-07-07 15:37: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, 2016-07-07 15:39:40]

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

[commit-bot: I haz the power, 2016-07-07 15:39:41]

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

[aleloi, 2016-07-08 08:38:09]

Patchset #9 (id:280001) has been deleted

[aleloi, 2016-07-08 08:38:14]

Patchset #8 (id:260001) has been deleted

[aleloi, 2016-07-08 08:38:41]

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

[commit-bot: I haz the power, 2016-07-08 08:38:49]

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

[aleloi, 2016-07-08 08:39:58]

Patchset #8 (id:300001) has been deleted

[aleloi, 2016-07-08 08:40:53]

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

[commit-bot: I haz the power, 2016-07-08 08:41:04]

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-07-08 08:53:14]

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

[commit-bot: I haz the power, 2016-07-08 08:53:15]

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

[aleloi, 2016-07-08 09:16:14]

Patchset #8 (id:320001) has been deleted

[aleloi, 2016-07-08 09:26:13]

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

[commit-bot: I haz the power, 2016-07-08 09:26:22]

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-07-08 11:26:53]

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

[commit-bot: I haz the power, 2016-07-08 11:26:54]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ivoc, 2016-07-08 12:19:28]

Just one comment left, almost there.

https://codereview.webrtc.org/2109333006/diff/340001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
(right):

https://codereview.webrtc.org/2109333006/diff/340001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:178:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
Do you know if the NewAudioConferenceMixerImpl is constructed on the same thread
that Mix is called on? If not, you should add a call to
thread_checker_.DetachFromThread() in the constructor.

[aleloi, 2016-07-08 12:24:34]

Patchset #8 (id:340001) has been deleted

[aleloi, 2016-07-08 12:25:07]

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

[commit-bot: I haz the power, 2016-07-08 12:25:19]

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

[aleloi, 2016-07-08 12:27:54]

Patchset #8 (id:360001) has been deleted

[aleloi, 2016-07-08 12:28:25]

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

[commit-bot: I haz the power, 2016-07-08 12:28:28]

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

[ivoc, 2016-07-08 12:29:48]

On 2016/07/08 12:19:28, ivoc wrote:
> Just one comment left, almost there.
> 
>
https://codereview.webrtc.org/2109333006/diff/340001/webrtc/modules/audio_mix...
> File webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc
> (right):
> 
>
https://codereview.webrtc.org/2109333006/diff/340001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc:178:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());
> Do you know if the NewAudioConferenceMixerImpl is constructed on the same
thread
> that Mix is called on? If not, you should add a call to
> thread_checker_.DetachFromThread() in the constructor.

LGTM.

[aleloi, 2016-07-08 13:02:09]

Patchset #8 (id:380001) has been deleted

[aleloi, 2016-07-08 13:46:52]

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

[commit-bot: I haz the power, 2016-07-08 13:47:05]

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-07-08 15:47:45]

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

[commit-bot: I haz the power, 2016-07-08 15:47:46]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[aleloi, 2016-07-28 09:23:42]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-07-28 09:23:43]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2109333006/#ps400001 (title:
"Added thread checker.")

[commit-bot: I haz the power, 2016-07-28 09:23:51]

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

[commit-bot: I haz the power, 2016-07-28 09:23:52]

This CL has an open dependency (Issue 2111293003 Patch 380001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[aleloi, 2016-07-28 11:06:41]

The CQ bit was checked by aleloi@webrtc.org

[commit-bot: I haz the power, 2016-07-28 11:06: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, 2016-07-28 13:24:18]

Committed patchset #8 (id:400001)

[commit-bot: I haz the power, 2016-07-28 13:24:32]

Description was changed from

==========
Removed Module inheritance and unused methods and members TimeScheduler,
Process() and TimeToNextProcess() from mixer.
==========

to

==========
Removed Module inheritance and unused methods and members TimeScheduler,
Process() and TimeToNextProcess() from mixer.

Committed: https://crrev.com/8b2233f5a6394461845310ca14c918b3ce9eb5ef
Cr-Commit-Position: refs/heads/master@{#13552}
==========

[commit-bot: I haz the power, 2016-07-28 13:24:35]

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