Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder

Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder

This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead
of an AudioFifo, to avoid copying data needlessly.

BUG=None

Review-Url: https://codereview.chromium.org/2854363002
Cr-Commit-Position: refs/heads/master@{#471039}
Committed: https://chromium.googlesource.com/chromium/src/+/678c74cf3e17aaef9562ee13df8c803e1d014fc6

[Chandan, 2017-05-03 11:36:43]

c.padhi@samsung.com changed reviewers:
+ mcasas@chromium.org

[Chandan, 2017-05-03 11:36:44]

mcasas@, I have made an attempt to remove AudioFifo from
AudioTrackRecorder::AudioEncoder. Please let me know if my approach to it is
correct. Thank you.

[Chandan, 2017-05-05 09:23:49]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 09:24:03]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-05 10:19:13]

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

[commit-bot: I haz the power, 2017-05-05 10:19:14]

Dry run: This issue passed the CQ dry run.

[Chandan, 2017-05-08 15:36:14]

On 2017/05/03 11:36:44, Chandan wrote:
> mcasas@, I have made an attempt to remove AudioFifo from
> AudioTrackRecorder::AudioEncoder. Please let me know if my approach to it is
> correct. Thank you.

PTAL.

[mcasas, 2017-05-09 17:32:33]

mcasas@chromium.org changed reviewers:
+ miu@chromium.org

[mcasas, 2017-05-09 17:32:34]

overall it doesn't look bad and the tests are
passing, which is a very good sign, but I'd
like miu@ to take a look since he's more 
versed in the audio intricacies.

[miu, 2017-05-09 19:53:19]

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/audio_track_recorder.cc (left):

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:48:
kMaxNumberOfFifoBuffers = 2,
Keep this. (See comment below.)

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/audio_track_recorder.cc (right):

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:133: int frames_in_;
Since these values always increase, they are at risk of overflowing after
several hours of recording.

Instead, how about a |frames_queued_|, which is increased when adding to the
|audio_bus_queue_| and decreased when removing from the |audio_bus_queue_|?

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:243:
audio_bus_queue_.push_back(std::move(input_bus));
The old code had a limit on the maximum amount of audio that could be buffered,
which is important (to make sure we don't grow without bound). I'd suggest
checking for this, and dropping old audio. Something like:

  frames_queued_ += input_bus->frames();
  audio_bus_queue_.push_back(std::move(input_bus));
  const int max_frame_limit = kMaxNumberOfFifoBuffers *
input_params_.frames_per_buffer();
  while (frames_queued_ > max_frame_limit) {
    frames_queued_ -= audio_bus_queue_.front()->frames();
    audio_bus_queue_.pop_front();
  }

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:275: return 1.0;  //
Return volume greater than zero to indicate we have more data.
It seems that if |audio_bus_queue_| is empty, we should return 0.0 here.

[Chandan, 2017-05-10 06:01:42]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

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

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-10 07:34:50]

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

[commit-bot: I haz the power, 2017-05-10 07:34:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chandan, 2017-05-10 08:38:23]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 08:38:48]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-10 09:35:07]

miu@, thanks for the review. PTAL.

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/audio_track_recorder.cc (left):

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:48:
kMaxNumberOfFifoBuffers = 2,
On 2017/05/09 19:53:19, miu wrote:
> Keep this. (See comment below.)

Done.

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
File content/renderer/media_recorder/audio_track_recorder.cc (right):

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:133: int frames_in_;
On 2017/05/09 19:53:19, miu wrote:
> Since these values always increase, they are at risk of overflowing after
> several hours of recording.
> 
> Instead, how about a |frames_queued_|, which is increased when adding to the
> |audio_bus_queue_| and decreased when removing from the |audio_bus_queue_|?

Acknowledged.

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:243:
audio_bus_queue_.push_back(std::move(input_bus));
On 2017/05/09 19:53:19, miu wrote:
> The old code had a limit on the maximum amount of audio that could be
buffered,
> which is important (to make sure we don't grow without bound). I'd suggest
> checking for this, and dropping old audio. Something like:
> 
>   frames_queued_ += input_bus->frames();
>   audio_bus_queue_.push_back(std::move(input_bus));
>   const int max_frame_limit = kMaxNumberOfFifoBuffers *
> input_params_.frames_per_buffer();
>   while (frames_queued_ > max_frame_limit) {
>     frames_queued_ -= audio_bus_queue_.front()->frames();
>     audio_bus_queue_.pop_front();
>   }

Done.

https://codereview.chromium.org/2854363002/diff/1/content/renderer/media_reco...
content/renderer/media_recorder/audio_track_recorder.cc:275: return 1.0;  //
Return volume greater than zero to indicate we have more data.
On 2017/05/09 19:53:19, miu wrote:
> It seems that if |audio_bus_queue_| is empty, we should return 0.0 here.

Done.

[commit-bot: I haz the power, 2017-05-10 11:12:56]

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

[commit-bot: I haz the power, 2017-05-10 11:12:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2017-05-10 20:47:03]

PS2 lgtm. Thanks! :)

[Chandan, 2017-05-11 03:36:04]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-11 03:37:03]

CQ is trying da patch.

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

[Chandan, 2017-05-11 03:47:34]

On 2017/05/11 03:37:03, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Thanks miu@.
mcasas@, PTAL.

[commit-bot: I haz the power, 2017-05-11 03:50:14]

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

[commit-bot: I haz the power, 2017-05-11 03:50:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mcasas, 2017-05-11 16:17:40]

On 2017/05/11 03:50:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

RS lgtm

[Chandan, 2017-05-11 16:23:16]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-11 16:24:12]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-11 18:18:26]

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

[commit-bot: I haz the power, 2017-05-11 18:18:28]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Chandan, 2017-05-11 18:36:39]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-11 18:37:26]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-11 19:25:04]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494527799891150,
"parent_rev": "60e4c6d33956a93ca6dcb7c78c913e84445dcfce", "commit_rev":
"678c74cf3e17aaef9562ee13df8c803e1d014fc6"}

[commit-bot: I haz the power, 2017-05-11 19:25:14]

Description was changed from

==========
Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder

This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead
of an AudioFifo, to avoid copying data needlessly.

BUG=None
==========

to

==========
Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder

This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead
of an AudioFifo, to avoid copying data needlessly.

BUG=None

Review-Url: https://codereview.chromium.org/2854363002
Cr-Commit-Position: refs/heads/master@{#471039}
Committed:
https://chromium.googlesource.com/chromium/src/+/678c74cf3e17aaef9562ee13df8c...
==========

[commit-bot: I haz the power, 2017-05-11 19:25:15]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/678c74cf3e17aaef9562ee13df8c...

[apacible, 2017-05-11 22:23:31]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2873123005/ by apacible@chromium.org.

The reason for reverting is: Builder failing:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

[findit-for-me, 2017-05-12 01:19:34]

Findit (https://goo.gl/kROfz5) identified this CL at revision 471039 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Chandan, 2017-05-12 13:27:50]

On 2017/05/12 01:19:34, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 471039 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

I am not able to relate the failures in the build cycles with this CL.
Try jobs had earlier succeeded for this CL.
I have created a reland CL(https://codereview.chromium.org/2881713002/) that
also has passed the dry run.
@mcasas, @miu: Your insights, please?