Moved the AECM render sample queue into the audio processing module

Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
Committed: https://crrev.com/a062460a68489fd973167ec041b1883631d41371
Cr-Commit-Position: refs/heads/master@{#14768}

[peah-webrtc, 2016-10-24 21:27:56]

Description was changed from

==========
Moved the AEC render sample queue into the audio processing module

Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
==========

to

==========
Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
==========

[peah-webrtc, 2016-10-24 21:34:57]

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

[ivoc, 2016-10-25 08:57:00]

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:810: const size_t
new_int16_render_queue_element_max_size =
Would this ever be different from the size of the float buffer in practice?

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:866: while
(int16_render_signal_queue_->Remove(&int16_capture_queue_buffer_)) {
Shouldn't this be split into separate functions? This function is called when
the float buffer is full or when the int16 buffer is full. If the sizes of these
buffers are potentially different (and it looks from the code that they could
be), then the buffers can be full at different times. Shouldn't we have a
separate EmptyQueuedFloatRenderAudio() and EmptyQueuedInt16RenderAudio()?

[peah-webrtc, 2016-10-25 09:48:33]

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:810: const size_t
new_int16_render_queue_element_max_size =
On 2016/10/25 08:57:00, ivoc wrote:
> Would this ever be different from the size of the float buffer in practice?

In practice it should not. However, currently these lengths implicitly depend on
how the submodules are initialized. In practice, it should not be a problem
since they are not used at the same time but until we do further refactoring I'd
prefer to have these lengths separate.

WDYT?

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:866: while
(int16_render_signal_queue_->Remove(&int16_capture_queue_buffer_)) {
On 2016/10/25 08:57:00, ivoc wrote:
> Shouldn't this be split into separate functions? This function is called when
> the float buffer is full or when the int16 buffer is full. If the sizes of
these
> buffers are potentially different (and it looks from the code that they could
> be), then the buffers can be full at different times. Shouldn't we have a
> separate EmptyQueuedFloatRenderAudio() and EmptyQueuedInt16RenderAudio()?

Yes, that could make sense.
This method is, however, called in two situations
1) Before a capture call to ensure that all render data has been processed.
2) When the render buffer becomes full to ensure that there is space in the
render buffer for more data.

Bullet 1) is part of the standard operation and for that I think that it is not
necessary to split this method.

Bullet 2) is very costly as it forces the APM to run single-threadedly. This we
want to do as seldom as possible and therefore I think it is good to take the
opportunity to empty all the render buffers to avoid that happening soon again.

Finally, note that all the render buffers store kMaxNumFramesToBuffer so they
should have the same lengths all the time.

WDYT? Can we keep it as it is?

[ivoc, 2016-10-25 10:39:46]

LGTM.

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:810: const size_t
new_int16_render_queue_element_max_size =
On 2016/10/25 09:48:33, peah-webrtc wrote:
> On 2016/10/25 08:57:00, ivoc wrote:
> > Would this ever be different from the size of the float buffer in practice?
> 
> In practice it should not. However, currently these lengths implicitly depend
on
> how the submodules are initialized. In practice, it should not be a problem
> since they are not used at the same time but until we do further refactoring
I'd
> prefer to have these lengths separate.
> 
> WDYT?

Ok, that sounds fine.

https://codereview.webrtc.org/2444793005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:866: while
(int16_render_signal_queue_->Remove(&int16_capture_queue_buffer_)) {
On 2016/10/25 09:48:33, peah-webrtc wrote:
> On 2016/10/25 08:57:00, ivoc wrote:
> > Shouldn't this be split into separate functions? This function is called
when
> > the float buffer is full or when the int16 buffer is full. If the sizes of
> these
> > buffers are potentially different (and it looks from the code that they
could
> > be), then the buffers can be full at different times. Shouldn't we have a
> > separate EmptyQueuedFloatRenderAudio() and EmptyQueuedInt16RenderAudio()?
> 
> Yes, that could make sense.
> This method is, however, called in two situations
> 1) Before a capture call to ensure that all render data has been processed.
> 2) When the render buffer becomes full to ensure that there is space in the
> render buffer for more data.
> 
> Bullet 1) is part of the standard operation and for that I think that it is
not
> necessary to split this method.
> 
> Bullet 2) is very costly as it forces the APM to run single-threadedly. This
we
> want to do as seldom as possible and therefore I think it is good to take the
> opportunity to empty all the render buffers to avoid that happening soon
again.
> 
> Finally, note that all the render buffers store kMaxNumFramesToBuffer so they
> should have the same lengths all the time.
> 
> WDYT? Can we keep it as it is?

Right, that does make sense, let's keep it like this.

[peah-webrtc, 2016-10-25 10:40:37]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2016-10-25 10:40:46]

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-25 10:51:55]

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

[commit-bot: I haz the power, 2016-10-25 10:51:56]

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

[peah-webrtc, 2016-10-25 11:01:13]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-10-25 11:01:14]

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/2444793005/#ps20001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-10-25 11:01:16]

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-25 11:45:26]

Description was changed from

==========
Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
==========

to

==========
Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
==========

[commit-bot: I haz the power, 2016-10-25 11:45:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-25 11:45:37]

Description was changed from

==========
Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540
==========

to

==========
Several subcomponents inside APM copy render audio from
the render side to the capture side using the same
pattern. Currently this is done independently for the
submodules.

This CL moves the the AECM functionality for this into
APM.

BUG=webrtc:5298, webrtc:6540

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

[commit-bot: I haz the power, 2016-10-25 11:45:38]

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