Make more media APIs aware of |delay| and |delay_timestamp|

Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation [1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped) -> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) -> AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533
[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

Committed: https://crrev.com/df872a842609926a62e0211868c886d0ec96cff7
Committed: https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Cr-Original-Commit-Position: refs/heads/master@{#427996}
Cr-Commit-Position: refs/heads/master@{#428953}

[Mikhail, 2016-10-20 18:07:15]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/
==========

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/
==========

[Mikhail, 2016-10-20 18:07:15]

mikhail.pozdnyakov@intel.com changed reviewers:
+ chcunningham@chromium.org, jameswest@google.com

[Mikhail, 2016-10-20 18:12:09]

Please take a look.
Unfortunately the modified APIs are used in many places so the patch is quite
massive.

[DaleCurtis, 2016-10-20 18:35:29]

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_sync_reader.cc:159: Packet packet =
{delay.ToInternalValue(), delay_timestamp.ToInternalValue()};
In the past, there were concerns that sending more than 4 bytes of data might
cause descheduling of the real-time audio thread. Probably this needs to be
watched closely to see if we see an increase in AudioRendererMissedDeadline,
AudioRendererAudioGlitches, etc.

[chcunningham, 2016-10-21 18:19:47]

Nice change :)

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:72: if (bytes_read != sizeof(packet))
Not sure if this is part of normal operation. Should we log an error for this?

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:45: virtual void Process(int64_t pending_data,
Can you use a TimeDelta in this signature (and calling it delay and
delay_timestamp like we do browser side).

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:81: int64_t pending_data;
Can you name these like you do browser side? 

delay;
delay_timestamp;

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.cc (left):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:315: total_bytes_delay + frames *
params_.GetBytesPerFrame(),
Why aren't you adding in the delay for the dest buffer we just read? I think you
still need to factor that in here.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:314: sync_reader_->PrepareNextData(delay,
delay_timestamp, prior_frames_skipped);
Nit: What do you think about "RequestMoreData"? Its probably not obvious that
UpdatePendingBytes is also a request for the next buffer... PrepareNextData is
an improvement, but "Request" reads more clearly IMHO

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.h:91: // Notify the synchronous reader the
number of bytes in the
Replace mentions of bytes with time delayed.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:482: const int frames =
output_bus_->frames();
I'm confused. Why are we doing this here instead of audio_output_controller
where it was previously done?

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
File media/base/fake_audio_render_callback.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.cc:29: uint32_t frames_skipped) {
ditto

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
File media/base/fake_audio_render_callback.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.h:35: uint32_t frames_skipped) override;
nit: do you mean to rename this to prior_frames_skipped everywhere?

[James West, 2016-10-21 19:40:16]

jameswest@chromium.org changed reviewers:
+ jameswest@chromium.org

[James West, 2016-10-21 19:40:17]

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:413: // TODO(grunell): Using of
milliseconds will potentially lose
"Using of milliseconds" => "Using milliseconds"

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.h:99: uint32_t frames_skipped) = 0;
Change "uint32_t frames_skipped" to "int prior_frames_skipped".

https://codereview.chromium.org/2437863004/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:39: uint32_t prior_frames_skipped) = 0;
Please use the same signature as
AudioOutputStream::AudioSourceCallback::OnMoreData to be consistent and bring
this in line with Chromium style.

[miu, 2016-10-23 01:19:15]

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

[miu, 2016-10-23 01:19:16]

Nice work! Great to see more of the "legacy audio API" being fixed. This will
help a lot of projects resolve some long-standing audio timing issues (such as
A/V sync concerns).

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:331:
base::TimeTicks::Now().ToInternalValue()};
Please don't use ToInternalValue() (per http://crbug.com/634507). Use
InMicroseconds() instead.

Note: base::TimeTicks::Now().ToInternalValue() can be written equivalently as:

  (base::TimeTicks::Now() - base::TimeTicks()).InMicroseconds()

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc
(right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc:51:
const size_t kWriterPacketSize = sizeof(media::AudioDeviceThread::Packet);
nit: You could probably eliminate this, and just use the sizeof(Packet) on line
68. Otherwise, please use constexpr instead of const.

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_sync_reader.cc:159: Packet packet =
{delay.ToInternalValue(), delay_timestamp.ToInternalValue()};
ditto here: Please use InMicroseconds() instead of ToInternalValue().

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:417: int64_t
audio_delay_milliseconds = delay.InMilliseconds();
No need to create a local variable here. Just assign directly to the class
member |audio_delay_milliseconds_| (and delete line 423 below).

Bonus points (because of the TODO comment) if you change:

  int audio_delay_milliseconds_;  -->  base::TimeDelta audio_delay_;

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:86:
base::TimeTicks::FromInternalValue(packet.data_timestamp_us));
ditto: Instead of FromInternalValue(), please use:

  base::TimeTicks() +
base::TimeDelta::FromMicroseconds(packet.data_timestamp_us)

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:458: base::TimeDelta delay =
base::TimeDelta::FromInternalValue(pending_data);
Instead of passing pending_data as an int64_t, could you pass it as a
base::TimeDelta? IMHO, we should try to keep all the (de)serialization logic
where the shared memory is owned/read.

https://codereview.chromium.org/2437863004/diff/1/media/renderers/audio_rende...
File media/renderers/audio_renderer_impl.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/renderers/audio_rende...
media/renderers/audio_renderer_impl.h:153: // not called at the same rate as
audio samples are played, then the reported
This part of the comment needs to be updated to refer to time delays rather than
frame delays.

[Mikhail, 2016-10-24 05:58:48]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 05:59:02]

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, 2016-10-24 06:16:37]

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

[commit-bot: I haz the power, 2016-10-24 06:16:37]

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

[Mikhail, 2016-10-24 09:25:58]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 09:26:10]

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, 2016-10-24 09:41:32]

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

[commit-bot: I haz the power, 2016-10-24 09:41:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mikhail, 2016-10-24 17:07:09]

Patchset #2 (id:20001) has been deleted

[Mikhail, 2016-10-24 19:47:17]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 19:48:06]

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

[Mikhail, 2016-10-24 19:50:24]

Thank you for review!

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:331:
base::TimeTicks::Now().ToInternalValue()};
On 2016/10/23 01:19:15, miu wrote:
> Please don't use ToInternalValue() (per http://crbug.com/634507). Use
> InMicroseconds() instead.
> 
> Note: base::TimeTicks::Now().ToInternalValue() can be written equivalently as:
> 
>   (base::TimeTicks::Now() - base::TimeTicks()).InMicroseconds()

Done. Even though IMO in this particular case using of 'ToInternalValue' is
quite appropriate as it is used for serialization.

off topic: Might be advisable to rename 'FromInternalValue/ToInternalValue' to
'Serialize/Deserialize'

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc
(right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc:51:
const size_t kWriterPacketSize = sizeof(media::AudioDeviceThread::Packet);
On 2016/10/23 01:19:15, miu wrote:
> nit: You could probably eliminate this, and just use the sizeof(Packet) on
line
> 68. Otherwise, please use constexpr instead of const.

Done.

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_sync_reader.cc:159: Packet packet =
{delay.ToInternalValue(), delay_timestamp.ToInternalValue()};
On 2016/10/23 01:19:15, miu wrote:
> ditto here: Please use InMicroseconds() instead of ToInternalValue().

Done.

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:417: int64_t
audio_delay_milliseconds = delay.InMilliseconds();
On 2016/10/23 01:19:15, miu wrote:
> No need to create a local variable here. Just assign directly to the class
> member |audio_delay_milliseconds_| (and delete line 423 below).
> 
> Bonus points (because of the TODO comment) if you change:
> 
>   int audio_delay_milliseconds_;  -->  base::TimeDelta audio_delay_;

Done.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:72: if (bytes_read != sizeof(packet))
On 2016/10/21 18:19:47, chcunningham wrote:
> Not sure if this is part of normal operation. Should we log an error for this?

Done.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:86:
base::TimeTicks::FromInternalValue(packet.data_timestamp_us));
On 2016/10/23 01:19:15, miu wrote:
> ditto: Instead of FromInternalValue(), please use:
> 
>   base::TimeTicks() +
> base::TimeDelta::FromMicroseconds(packet.data_timestamp_us)

Done.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:45: virtual void Process(int64_t pending_data,
On 2016/10/21 18:19:47, chcunningham wrote:
> Can you use a TimeDelta in this signature (and calling it delay and
> delay_timestamp like we do browser side).

The reason for the current naming is that AudioInputDevice is using it in a
different way (pls see AudioInputDevice::AudioThreadCallback::Process):
|pending_data| contains shmem segment there, so I had to keep naming generic.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:81: int64_t pending_data;
On 2016/10/21 18:19:47, chcunningham wrote:
> Can you name these like you do browser side? 
> 
> delay;
> delay_timestamp;

Same motivation as in the comment above.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:314: sync_reader_->PrepareNextData(delay,
delay_timestamp, prior_frames_skipped);
On 2016/10/21 18:19:47, chcunningham wrote:
> Nit: What do you think about "RequestMoreData"? Its probably not obvious that
> UpdatePendingBytes is also a request for the next buffer... PrepareNextData is
> an improvement, but "Request" reads more clearly IMHO
Agree, "RequestMoreData" is better. thanks!

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.h:91: // Notify the synchronous reader the
number of bytes in the
On 2016/10/21 18:19:47, chcunningham wrote:
> Replace mentions of bytes with time delayed.

Done.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.h:99: uint32_t frames_skipped) = 0;
On 2016/10/21 19:40:17, James West wrote:
> Change "uint32_t frames_skipped" to "int prior_frames_skipped".

Done.

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:458: base::TimeDelta delay =
base::TimeDelta::FromInternalValue(pending_data);
On 2016/10/23 01:19:16, miu wrote:
> Instead of passing pending_data as an int64_t, could you pass it as a
> base::TimeDelta? IMHO, we should try to keep all the (de)serialization logic
> where the shared memory is owned/read.
The reason for keeping this is 'AudioInputDevice::AudioThreadCallback::Process'
(pls see comments above).

https://codereview.chromium.org/2437863004/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:482: const int frames =
output_bus_->frames();
On 2016/10/21 18:19:47, chcunningham wrote:
> I'm confused. Why are we doing this here instead of audio_output_controller
> where it was previously done?
Indeed. Thanks for noticing!

https://codereview.chromium.org/2437863004/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:39: uint32_t prior_frames_skipped) = 0;
On 2016/10/21 19:40:17, James West wrote:
> Please use the same signature as
> AudioOutputStream::AudioSourceCallback::OnMoreData to be consistent and bring
> this in line with Chromium style.

Sure, I missed somehow that the arg order is different :(

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
File media/base/fake_audio_render_callback.cc (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.cc:29: uint32_t frames_skipped) {
On 2016/10/21 18:19:47, chcunningham wrote:
> ditto

Done.

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
File media/base/fake_audio_render_callback.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.h:35: uint32_t frames_skipped) override;
On 2016/10/21 18:19:47, chcunningham wrote:
> nit: do you mean to rename this to prior_frames_skipped everywhere?

Done.

https://codereview.chromium.org/2437863004/diff/1/media/renderers/audio_rende...
File media/renderers/audio_renderer_impl.h (right):

https://codereview.chromium.org/2437863004/diff/1/media/renderers/audio_rende...
media/renderers/audio_renderer_impl.h:153: // not called at the same rate as
audio samples are played, then the reported
On 2016/10/23 01:19:16, miu wrote:
> This part of the comment needs to be updated to refer to time delays rather
than
> frame delays.

Done.

[commit-bot: I haz the power, 2016-10-24 20:44:04]

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

[commit-bot: I haz the power, 2016-10-24 20:44:06]

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

[James West, 2016-10-24 21:05:45]

https://codereview.chromium.org/2437863004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://codereview.chromium.org/2437863004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.h:51: uint32_t
frames_skipped) override;
Change to "int prior_frames_skipped" for consistency with other APIs and because
Chromium style discourages unsigned integers unless they're necessary for some
reason.

https://codereview.chromium.org/2437863004/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/2437863004/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:93: // if any frames has been skipped by
the renderer (typically the OS). The
"has been" => "have been"

https://codereview.chromium.org/2437863004/diff/40001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2437863004/diff/40001/media/base/audio_render...
media/base/audio_renderer_sink.h:38: uint32_t prior_frames_skipped,
Use int instead of uint32_t.

[miu, 2016-10-24 22:45:52]

lgtm (assuming James' comments are addressed before commit)

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:331:
base::TimeTicks::Now().ToInternalValue()};
On 2016/10/24 19:50:23, Mikhail wrote:
> off topic: Might be advisable to rename 'FromInternalValue/ToInternalValue' to
> 'Serialize/Deserialize'

Your use was appropriate here. :)

As the base/time OWNER, I'm trying to eliminate these methods because developers
are repeatedly using them inappropriately to side-step the compile-time checking
of math operations. This has already led to user-facing bugs. Instead,
developers should use In/FromMicroseconds() since the naming makes it clear that
a data type conversion is occurring (with clear semantics), and also would tend
to allow code reviewers to catch problems more easily.

I could go into more detail, but I've probably already said too much here. ;)

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc_audio_renderer.cc:417: int64_t
audio_delay_milliseconds = delay.InMilliseconds();
On 2016/10/24 19:50:23, Mikhail wrote:
> > Bonus points (because of the TODO comment) if you change:
> > 
> >   int audio_delay_milliseconds_;  -->  base::TimeDelta audio_delay_;
> 
> Done.

Thanks for the "bonus" change! :)

[Mikhail, 2016-10-25 06:50:48]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

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

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

[Mikhail, 2016-10-25 07:46:10]

https://codereview.chromium.org/2437863004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.h (right):

https://codereview.chromium.org/2437863004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.h:51: uint32_t
frames_skipped) override;
On 2016/10/24 21:05:45, James West wrote:
> Change to "int prior_frames_skipped" for consistency with other APIs and
because
> Chromium style discourages unsigned integers unless they're necessary for some
> reason.

Done.

https://codereview.chromium.org/2437863004/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/2437863004/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:93: // if any frames has been skipped by
the renderer (typically the OS). The
On 2016/10/24 21:05:45, James West wrote:
> "has been" => "have been"

Done.

https://codereview.chromium.org/2437863004/diff/40001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2437863004/diff/40001/media/base/audio_render...
media/base/audio_renderer_sink.h:38: uint32_t prior_frames_skipped,
On 2016/10/24 21:05:45, James West wrote:
> Use int instead of uint32_t.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[James West, 2016-10-25 18:22:17]

lgtm except for a few minor things.

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:106: int frames_skipped,
prior_frames_skipped

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:46: int frames_skipped,
prior_frames_skipped

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:422:
static_cast<int>(sink_params_.sample_rate() / 100);
static_cast is unnecessary because sink_params_.sample_rate() returns an int.

[Mikhail, 2016-10-25 20:57:30]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 20:57:55]

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

[Mikhail, 2016-10-25 20:58:10]

Thanks for review!

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:106: int frames_skipped,
On 2016/10/25 18:22:17, James West wrote:
> prior_frames_skipped

Done.

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:46: int frames_skipped,
On 2016/10/25 18:22:17, James West wrote:
> prior_frames_skipped

Done.

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/60001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:422:
static_cast<int>(sink_params_.sample_rate() / 100);
On 2016/10/25 18:22:17, James West wrote:
> static_cast is unnecessary because sink_params_.sample_rate() returns an int.

Done.

[Mikhail, 2016-10-25 20:58:51]

mikhail.pozdnyakov@intel.com changed reviewers:
+ jochen@chromium.org

[Mikhail, 2016-10-25 21:01:35]

jochen@, could you take a look at content/ changes?

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

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

[commit-bot: I haz the power, 2016-10-25 22:21:52]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2016-10-26 00:01:49]

lgtm

[jochen (gone - plz use gerrit), 2016-10-26 07:07:12]

please ask one of the /content/*/media OWNERS to review the content changes

[Mikhail, 2016-10-26 07:14:35]

mikhail.pozdnyakov@intel.com changed reviewers:
+ perkj@chromium.org, tommi@chromium.org

[Mikhail, 2016-10-26 07:18:10]

tommi@, perkj@, could you take a look at content/ changes?

[tommi (sloooow) - chröme, 2016-10-26 19:53:26]

lgtm % comments

https://codereview.chromium.org/2437863004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2437863004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:331:
base::TimeTicks::Now().ToInternalValue()};
using ToInternalValue() here but InMicroseconds elsewhere?

https://codereview.chromium.org/2437863004/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:85: if (packet.pending_data !=
std::numeric_limits<int64_t>::max())
{}

[Mikhail, 2016-10-27 06:14:14]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 06:14:29]

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, 2016-10-27 06:25:32]

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

[commit-bot: I haz the power, 2016-10-27 06:25:33]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Mikhail, 2016-10-27 06:28:20]

Patchset #5 (id:100001) has been deleted

[Mikhail, 2016-10-27 06:31:10]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 06:31:47]

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, 2016-10-27 10:47:43]

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

[commit-bot: I haz the power, 2016-10-27 10:47:44]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-10-27 10:56:27]

https://codereview.chromium.org/2437863004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2437863004/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:331:
base::TimeTicks::Now().ToInternalValue()};
On 2016/10/26 19:53:25, tommi (chrömium) wrote:
> using ToInternalValue() here but InMicroseconds elsewhere?

Done. Thanks for pointing out!

https://codereview.chromium.org/2437863004/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:85: if (packet.pending_data !=
std::numeric_limits<int64_t>::max())
On 2016/10/26 19:53:25, tommi (chrömium) wrote:
> {}

Done.

[Mikhail, 2016-10-27 10:56:48]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-10-27 10:56:48]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
jameswest@chromium.org, chcunningham@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2437863004/#ps120001
(title: "Comments from Tommi")

[commit-bot: I haz the power, 2016-10-27 10:57:09]

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, 2016-10-27 11:03:01]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/
==========

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/
==========

[commit-bot: I haz the power, 2016-10-27 11:03:02]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-27 11:05:25]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/
==========

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

[commit-bot: I haz the power, 2016-10-27 11:05:27]

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

[sakal-chromium, 2016-10-27 14:08:21]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.chromium.org/2452183003/ by sakal@chromium.org.

The reason for reverting is: Breaks chromium.webrtc bots due to excessive
logging.

https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/30786
https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/29755
https://build.chromium.org/p/chromium.webrtc/builders/Win10%20Tester/builds/1....

[Mikhail, 2016-10-31 12:25:41]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

[Mikhail, 2016-10-31 12:31:11]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 12:31:20]

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

[Mikhail, 2016-10-31 12:34:32]

Patchset #6 (id:140001) has been deleted

[Mikhail, 2016-10-31 12:34:52]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 12:35:01]

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

[Mikhail, 2016-10-31 12:36:15]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 12:36:21]

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

[Mikhail, 2016-10-31 12:36:44]

Patchset #6 (id:160001) has been deleted

[Mikhail, 2016-10-31 13:31:26]

On 2016/10/27 14:08:21, sakal-chromium wrote:
> A revert of this CL (patchset #5 id:120001) has been created in
> https://codereview.chromium.org/2452183003/ by mailto:sakal@chromium.org.
> 
> The reason for reverting is: Breaks chromium.webrtc bots due to excessive
> logging.
> 
>
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/30786
>
https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/29755
>
https://build.chromium.org/p/chromium.webrtc/builders/Win10%20Tester/builds/1....

miu@, the excessive logging was caused by a minor leftover which I failed to
exclude when uploaded the patch :( This is fixed now in the latest patch
https://codereview.chromium.org/2437863004/diff2/120001:180001/content/render....

Could you please take a look?

[commit-bot: I haz the power, 2016-10-31 13:57:25]

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

[commit-bot: I haz the power, 2016-10-31 13:57:27]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-10-31 14:23:13]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 14:23:30]

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, 2016-10-31 15:39:25]

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

[commit-bot: I haz the power, 2016-10-31 15:39:26]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2016-10-31 20:08:35]

https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
media/audio/audio_device_thread.cc:73: LOG(ERROR) << "Failed to receive.";
I'm surprised this ever happens. What am I missing?

[miu, 2016-10-31 20:59:04]

lgtm

https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
media/audio/audio_device_thread.cc:73: LOG(ERROR) << "Failed to receive.";
On 2016/10/31 20:08:35, chcunningham wrote:
> I'm surprised this ever happens. What am I missing?

It seems legitimate for Receive() to return zero when shutting down the socket
from the other end? If this happened during normal operation, the audio thread
would end; and we would hear no audio. While I agree we should make this change
(to remove this LOG(ERROR) statement), I don't think it's the cause of the
excessive logging on the bots.

https://codereview.chromium.org/2437863004/diff/200001/content/renderer/media...
File content/renderer/media/track_audio_renderer.cc (right):

https://codereview.chromium.org/2437863004/diff/200001/content/renderer/media...
content/renderer/media/track_audio_renderer.cc:59: DVLOG(2) << "Pulling audio
out of shifter to be played "
In the bots' logs, I see this is what's being repeated a lot:

  [924:5672:1027/051440:ERROR:track_audio_renderer.cc(59)] Pulling audio out of
shifter to be played 0 ms from now. 512

This was a problem with the bot configuration. DVLOG(2) *should* produce
excessive logging, by definition. So, there's nothing wrong with your change.
Actually, there are other problems with the bot configuration, like the use of
--gtest_also_run_disabled_tests (!); and so you can't even disable faulty
tests?!? (which is what I would recommend if the re-land bounces back out)

The strange thing is: As this is unconditional logging (both before and after
this CL), I don't see why your change should be blamed. Looking at recent builds
on the bots, I don't see this being logged at all. I wonder if something in your
change is allowing audio to flow through TrackAudioRenderer where there wasn't
any before? (Maybe your change even unintentionally fixed something that was
broken?)

Therefore, I think you should be fine to go ahead and re-land the change. If it
bounces out for the same reason (excessive logging), IMHO, you would be
justified in filing a P0 bug since the webrtc bots have been misconfigured to
the point of blocking the commit of valid changes to the tree.

[Mikhail, 2016-11-01 06:57:05]

On 2016/10/31 20:08:35, chcunningham wrote:
>
https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
> File media/audio/audio_device_thread.cc (right):
> 
>
https://codereview.chromium.org/2437863004/diff/180001/media/audio/audio_devi...
> media/audio/audio_device_thread.cc:73: LOG(ERROR) << "Failed to receive.";
> I'm surprised this ever happens. What am I missing?

This happens during the normal shutting down, apparently that's why logging did
not exist there before.

[Mikhail, 2016-11-01 07:02:40]

On 2016/10/31 20:59:04, miu wrote:
>
https://codereview.chromium.org/2437863004/diff/200001/content/renderer/media...
> File content/renderer/media/track_audio_renderer.cc (right):
> 
>
https://codereview.chromium.org/2437863004/diff/200001/content/renderer/media...
> content/renderer/media/track_audio_renderer.cc:59: DVLOG(2) << "Pulling audio
> out of shifter to be played "
> In the bots' logs, I see this is what's being repeated a lot:
> 
>   [924:5672:1027/051440:ERROR:track_audio_renderer.cc(59)] Pulling audio out
of
> shifter to be played 0 ms from now. 512
> 
> This was a problem with the bot configuration. DVLOG(2) *should* produce
> excessive logging, by definition. So, there's nothing wrong with your change.
> Actually, there are other problems with the bot configuration, like the use of
> --gtest_also_run_disabled_tests (!); and so you can't even disable faulty
> tests?!? (which is what I would recommend if the re-land bounces back out)
> 
> The strange thing is: As this is unconditional logging (both before and after
> this CL), I don't see why your change should be blamed. Looking at recent
builds
> on the bots, I don't see this being logged at all. I wonder if something in
your
> change is allowing audio to flow through TrackAudioRenderer where there wasn't
> any before? (Maybe your change even unintentionally fixed something that was
> broken?)
> 
> Therefore, I think you should be fine to go ahead and re-land the change. If
it
> bounces out for the same reason (excessive logging), IMHO, you would be
> justified in filing a P0 bug since the webrtc bots have been misconfigured to
> the point of blocking the commit of valid changes to the tree.

In the reverted version of the patch this DVLOG was accidentally replaced by me
with LOG(ERROR)  (which I used locally for debug purposes and failed to clean up
when uploaded the patch to CL :-( ), so that caused unneeded logging. This is
fixed in patch set #6. Thank you for review and sorry for the hassle.

[Mikhail, 2016-11-01 07:03:12]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-11-01 07:03:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, jameswest@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2437863004/#ps200001
(title: "Removed unneeded logging also from audio_device_thread.cc")

[commit-bot: I haz the power, 2016-11-01 07:03: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, 2016-11-01 07:11:18]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

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

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-11-01 07:13:31]

Description was changed from

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

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

to

==========
Make more media APIs aware of |delay| and |delay_timestamp|

This patch is a part of 'AudioContext.getOutputTimestamp' method implementation
[1] and it continues the changes introduced at [2].

The AudioRendererSink::RenderCallback::Render method has same semantics as
AudioSourceCallback::OnMoreData.
That caused other changes in the following interfaces:
- AudioSyncReader::UpdatePendingBytes(uint32_t bytes, uint32_t frames_skipped)
-> AudioSyncReader::PrepareNextData(base::TimeDelta delay, base::TimeTicks
delay_timestamp, uint32_t frames_skipped)
- AudioDeviceThread::Callback::Process(uint32_t pending_data) ->
AudioDeviceThread::Callback::Process(int64_t pending_data, base::TimeTicks
data_timestamp)
- AudioDeviceThread is receiving a "packet" of two int64_t values

BUG=619533

[1] https://codereview.chromium.org/2060833002/
[2] https://codereview.chromium.org/2101303004/

Committed: https://crrev.com/df872a842609926a62e0211868c886d0ec96cff7
Committed: https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Cr-Original-Commit-Position: refs/heads/master@{#427996}
Cr-Commit-Position: refs/heads/master@{#428953}
==========

[commit-bot: I haz the power, 2016-11-01 07:13:33]

Patchset 7 (id:??) landed as
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Cr-Commit-Position: refs/heads/master@{#428953}

[David Tseng, 2016-11-02 20:31:34]

A revert of this CL (patchset #7 id:200001) has been created in
https://codereview.chromium.org/2471733004/ by dtseng@chromium.org.

The reason for reverting is: This breaks built in speech synthesis on Chrome OS.
To reproduce, simply use web speech to speak something e.g.
speechSynthesis.speak(new SpeechSynthesisUtterance('test'));.