Remove AudioBuffersState usage in Chromium

Remove AudioBuffersState class

The AudioBuffersState object doesn't appear to be necessary anymore.
Most code either completely ignores this information or doesn't actually
care about the difference between pending_bytes and hardware_delay_bytes.
Also usually only one of the 2 fields was actually being used at a time.
This change removes the class and simply uses an int that represent the
total number of delay bytes.

BUG=125685
Committed: https://crrev.com/c0a9787ba21a78aff69d863fb545e85981ac4dc1
Cr-Commit-Position: refs/heads/master@{#297220}

[acolwell GONE FROM CHROMIUM, 2014-09-17 20:48:26]

acolwell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, palmer@chromium.org, rkc@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-09-17 20:48:27]

dalecurtis: Please review all of media/
rkc : Please OWNERS review components/copresence changes
palmer: Please OWNERS review content/common/media/audio_messages.h

[palmer, 2014-09-17 21:08:44]

If I'm skimming this right, and I'm not sure I am, it seems like
|total_bytes_delay| is never used, either?

Also, in 1 place where you calculate it, there could be integer overflow. Is it
ever going to be used as an array index or pointer offset?

[acolwell GONE FROM CHROMIUM, 2014-09-17 22:03:13]

On 2014/09/17 21:08:44, Chromium Palmer wrote:
> If I'm skimming this right, and I'm not sure I am, it seems like
> |total_bytes_delay| is never used, either?
It is in media/audio/audio_output_controller.cc and
media/audio/audio_output_resampler.cc . This value is basically used to
determine how many bytes are in the audio device's hardware buffer. The number
of bytes is typically converted into frames and then into a duration based on
the number of channels, bit depth, and sample rate.

> 
> Also, in 1 place where you calculate it, there could be integer overflow. Is
it
> ever going to be used as an array index or pointer offset?

palmer: It won't ever be used as an array index or pointer offset. This value is
mainly used to compute a timestamp/duration.

[rkc, 2014-09-17 22:41:36]

//compoents/copresence lgtm

https://codereview.chromium.org/481193003/diff/60001/components/copresence/me...
File components/copresence/mediums/audio/audio_player.h (right):

https://codereview.chromium.org/481193003/diff/60001/components/copresence/me...
components/copresence/mediums/audio/audio_player.h:71: virtual void
OnError(media::AudioOutputStream* /* stream */) OVERRIDE;
Nit: Change the /* stream */ to stream to keep things consistent.

[DaleCurtis, 2014-09-17 22:47:46]

lgtm - thanks a lot!

[acolwell GONE FROM CHROMIUM, 2014-09-18 00:25:06]

https://codereview.chromium.org/481193003/diff/60001/components/copresence/me...
File components/copresence/mediums/audio/audio_player.h (right):

https://codereview.chromium.org/481193003/diff/60001/components/copresence/me...
components/copresence/mediums/audio/audio_player.h:71: virtual void
OnError(media::AudioOutputStream* /* stream */) OVERRIDE;
On 2014/09/17 22:41:36, Rahul Chaturvedi wrote:
> Nit: Change the /* stream */ to stream to keep things consistent.

Done.

[rkc, 2014-09-18 13:41:00]

FYI: ledger no longer builds with Chrome. So you should be fine committing the
patch completely removing AudioSBufferState.

[acolwell GONE FROM CHROMIUM, 2014-09-18 16:00:09]

On 2014/09/18 13:41:00, Rahul Chaturvedi wrote:
> FYI: ledger no longer builds with Chrome. So you should be fine committing the
> patch completely removing AudioSBufferState.

Did that happen in the last month or so? The original CL got reverted
(https://codereview.chromium.org/487543002) because the ledger code broke the
ChromeOS build.

[rkc, 2014-09-18 16:31:04]

Yep. It got fixed about a week ago with
https://codereview.chromium.org/510893003/

[acolwell GONE FROM CHROMIUM, 2014-09-18 16:41:49]

On 2014/09/18 16:31:04, Rahul Chaturvedi wrote:
> Yep. It got fixed about a week ago with
> https://codereview.chromium.org/510893003/

Wonderful! I'm so glad I won't have to do a 3-sided change now. \o/ 

Can the src-internal DEPS entries be removed at this point too? It looks like
the code still gets checked out.

[rkc, 2014-09-18 16:43:17]

Ah yes, that's been on my P3 list for a while :)
I'll land a CL removing the DEPS.

[acolwell GONE FROM CHROMIUM, 2014-09-18 17:02:33]

palmer: ping

[palmer, 2014-09-18 18:39:40]

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:283: int total_bytes_delay) {
void AudioSyncReader::UpdatePendingBytes(uint32 bytes)

int != uint32 in signedness and possibly even in width (ILP64).

(UpdatePendingBytes is correct to be defined as taking uint32; thus, change
|total_bytes_delay| to be uint32.)

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:297: total_bytes_delay + frames *
params_.GetBytesPerFrame());
I see this:

// media::AudioOutputController::SyncReader implementations.
void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
  // Zero out the entire output buffer to avoid stuttering/repeating-buffers
  // in the anomalous case if the renderer is unable to keep up with real-time.
  output_bus_->Zero();
  socket_->Send(&bytes, sizeof(bytes));
  ++buffer_index_;
}

But, after we send |bytes| on the socket, what does the receiver do with the
value? (I'm sorry, I am not familiar with the audio code, so I don't know
off-hand.) Can you show me how integer overflow doesn't matter for how the
receiver uses the value?

[miu, 2014-09-18 21:55:48]

Sorry if I'm too late to the discussion, but can we plumb in
"total_sample_delay" rather than "total_bytes_delay"?  The former would be much
more useful throughout the code base.  I'm also mentioning this because I'll
probably have to change it this way myself when I attack the A/V sync issues
with tab capture and streaming.

IMO, total_sample_delay should mean: Number of one channels' samples buffered
upstream.

[acolwell GONE FROM CHROMIUM, 2014-09-19 00:46:38]

On 2014/09/18 21:55:48, miu wrote:
> Sorry if I'm too late to the discussion, but can we plumb in
> "total_sample_delay" rather than "total_bytes_delay"?  The former would be
much
> more useful throughout the code base.  I'm also mentioning this because I'll
> probably have to change it this way myself when I attack the A/V sync issues
> with tab capture and streaming.

I'd like to defer this behavior change to a follow up CL so changes related to
using different units don't get mixed up with the struct removal stuff. I
realize it touches a bunch of the same files, but I'd like to keep these 2
changes separate.

> 
> IMO, total_sample_delay should mean: Number of one channels' samples buffered
> upstream.

Yes. I believe a better unit would be total_delay_in_frames. In a lot of the
audio code the term frame is used for what you describe.

[acolwell GONE FROM CHROMIUM, 2014-09-19 00:46:41]

On 2014/09/18 21:55:48, miu wrote:
> Sorry if I'm too late to the discussion, but can we plumb in
> "total_sample_delay" rather than "total_bytes_delay"?  The former would be
much
> more useful throughout the code base.  I'm also mentioning this because I'll
> probably have to change it this way myself when I attack the A/V sync issues
> with tab capture and streaming.

I'd like to defer this behavior change to a follow up CL so changes related to
using different units don't get mixed up with the struct removal stuff. I
realize it touches a bunch of the same files, but I'd like to keep these 2
changes separate.

> 
> IMO, total_sample_delay should mean: Number of one channels' samples buffered
> upstream.

Yes. I believe a better unit would be total_delay_in_frames. In a lot of the
audio code the term frame is used for what you describe.

[acolwell GONE FROM CHROMIUM, 2014-09-19 17:16:46]

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:283: int total_bytes_delay) {
On 2014/09/18 18:39:40, Chromium Palmer wrote:
> void AudioSyncReader::UpdatePendingBytes(uint32 bytes)
> 
> int != uint32 in signedness and possibly even in width (ILP64).
> 
> (UpdatePendingBytes is correct to be defined as taking uint32; thus, change
> |total_bytes_delay| to be uint32.)

Done.

https://codereview.chromium.org/481193003/diff/120001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:297: total_bytes_delay + frames *
params_.GetBytesPerFrame());
On 2014/09/18 18:39:40, Chromium Palmer wrote:
> I see this:
> 
> // media::AudioOutputController::SyncReader implementations.
> void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
>   // Zero out the entire output buffer to avoid stuttering/repeating-buffers
>   // in the anomalous case if the renderer is unable to keep up with
real-time.
>   output_bus_->Zero();
>   socket_->Send(&bytes, sizeof(bytes));
>   ++buffer_index_;
> }
> 
> But, after we send |bytes| on the socket, what does the receiver do with the
> value? (I'm sorry, I am not familiar with the audio code, so I don't know
> off-hand.) Can you show me how integer overflow doesn't matter for how the
> receiver uses the value?

The receiver of this value is in AudioDeviceThread::Thread::Run(). The Receive()
call was actually using an int instead of a uint32, which I have fixed in the
latest patch. Thanks for being persistant on this point. :)

[acolwell GONE FROM CHROMIUM, 2014-09-19 20:55:31]

palmer: ping. I believe I've addressed your concerns and I'd like to be able to
land this today.

[palmer, 2014-09-23 21:28:18]

Ooops, sorry; I got inundated in reviews. LGTM.

[acolwell GONE FROM CHROMIUM, 2014-09-23 21:47:35]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/481193003/140001

[commit-bot: I haz the power, 2014-09-23 22:23:45]

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

[commit-bot: I haz the power, 2014-09-23 22:23:46]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[acolwell GONE FROM CHROMIUM, 2014-09-29 18:47:04]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:48:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/481193003/180001

[commit-bot: I haz the power, 2014-09-29 18:54:07]

Committed patchset #10 (id:180001) as 0f5946add444ab1ba30e5d6b6e3e5388db4c7980

[commit-bot: I haz the power, 2014-09-29 18:55:10]

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