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

Reland: 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/

Initially committed at: https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/a6a331af3e247c40a7aa360c0422db4ec4e5b275
Cr-Commit-Position: refs/heads/master@{#435597}

[Mikhail, 2016-11-18 16:31:40]

Description was changed from

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
==========

to

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Mikhail, 2016-11-18 16:35:18]

Patchset #1 (id:1) has been deleted

[Mikhail, 2016-11-21 10:51:17]

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

[commit-bot: I haz the power, 2016-11-21 10:51: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-11-21 13:40:29]

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

[commit-bot: I haz the power, 2016-11-21 13:40:30]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-11-21 14:09:47]

Description was changed from

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Mikhail, 2016-11-21 14:09:47]

mikhail.pozdnyakov@intel.com changed reviewers:
+ chcunningham@chromium.org, dtseng@chromium.org, jameswest@chromium.org,
miu@chromium.org, tommi@chromium.org

[Mikhail, 2016-11-21 14:10:17]

PTAL

[Mikhail, 2016-11-21 14:12:37]

On 2016/11/21 14:10:17, Mikhail wrote:
> PTAL

The problem with initial patch was so that socket at content::AudioSyncReader at
some point could not cope with an increased amount data send through it, in the
updated patch both delay and timestamp are passed via shared buffer.

[miu, 2016-11-21 20:30:53]

PS2 lgtm % a couple concerns:

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

https://codereview.chromium.org/2517503003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:178: return;
Is this early return safe? In the old code |buffer_index_| was being incremented
unconditionally. Is that okay, or could this break things?

https://codereview.chromium.org/2517503003/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:454: void
AudioOutputDevice::AudioThreadCallback::Process(uint32_t pending_data) {
Did you mean to check |pending_data| here and return early if it's uint32 max?

[Mikhail, 2016-11-22 12:10:40]

Thanks for review!

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

https://codereview.chromium.org/2517503003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:178: return;
On 2016/11/21 20:30:53, miu wrote:
> Is this early return safe? In the old code |buffer_index_| was being
incremented
> unconditionally. Is that okay, or could this break things?
The old code just did not check result of 'socket_->Send' and apparently it
never failed on 4 bytes package send, I thought it's fine to return earlier as
buffer is not actually send, but on the other hand we can keep increment
unconditionally and rely on existing error-catch mechanism (counting missed
callbacks at AudioSyncReader::Read). So I'll change the patch.

https://codereview.chromium.org/2517503003/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:454: void
AudioOutputDevice::AudioThreadCallback::Process(uint32_t pending_data) {
On 2016/11/21 20:30:53, miu wrote:
> Did you mean to check |pending_data| here and return early if it's uint32 max?

That is done at 'AudioDeviceThread::ThreadMain()' so 'Process()' will not be
called.

[chcunningham, 2016-11-22 17:36:58]

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:452: void
AudioOutputDevice::AudioThreadCallback::Process(uint32_t pending_data) {
Can you rename this to control_signal to match the sender side?

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device_unittest.cc (right):

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device_unittest.cc:83: ACTION_P2(SendPendingBytes,
socket, pending_bytes) {
Should this be named RequestMoreData to match AudioSyncReader?

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device_unittest.cc:233:
.WillOnce(SendPendingBytes(&browser_socket_, kMemorySize));
I might be missing something... this seems like it doesn't match the new
convention of just sending a control signal (typically zero) and sending pending
delay separately via shared memory. If so, this test is probably not making
strong enough verifications that control signal and delay values are reasonable.

[Mikhail, 2016-11-23 15:25:31]

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

[commit-bot: I haz the power, 2016-11-23 15:25:58]

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

[Mikhail, 2016-11-23 15:26:01]

please take another look

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:452: void
AudioOutputDevice::AudioThreadCallback::Process(uint32_t pending_data) {
On 2016/11/22 17:36:58, chcunningham wrote:
> Can you rename this to control_signal to match the sender side?

Done.

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device_unittest.cc (right):

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device_unittest.cc:83: ACTION_P2(SendPendingBytes,
socket, pending_bytes) {
On 2016/11/22 17:36:58, chcunningham wrote:
> Should this be named RequestMoreData to match AudioSyncReader?

Done.

https://codereview.chromium.org/2517503003/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device_unittest.cc:233:
.WillOnce(SendPendingBytes(&browser_socket_, kMemorySize));
On 2016/11/22 17:36:58, chcunningham wrote:
> I might be missing something... this seems like it doesn't match the new
> convention of just sending a control signal (typically zero) and sending
pending
> delay separately via shared memory. If so, this test is probably not making
> strong enough verifications that control signal and delay values are
reasonable.
> 

The existing test did not check 'Render' arguments, this check is added in the
latest patch set.

[commit-bot: I haz the power, 2016-11-23 16:57:27]

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

[commit-bot: I haz the power, 2016-11-23 16:57:28]

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

[chcunningham, 2016-11-23 19:26:05]

lgtm

[Mikhail, 2016-11-23 19:48:56]

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

[tommi (sloooow) - chröme, 2016-11-23 19:52:32]

lgtm for content/

[David Tseng, 2016-11-23 23:49:33]

Not lg for tts. There's noticeable clipping when using tts through Pepper audio.
Better than before, but definitely a regression.

[Mikhail, 2016-11-25 10:56:25]

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

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

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

[Mikhail, 2016-11-25 10:59:25]

On 2016/11/23 23:49:33, David Tseng wrote:
> Not lg for tts. There's noticeable clipping when using tts through Pepper
audio.
> Better than before, but definitely a regression.

Indeed, thanks for commenting on this! my ears would never feel it..
Speach synthesis output signal stats:

Original amplitude values:
Maximum amplitude:     0.854675
Minimum amplitude:    -0.654175
Midline amplitude:     0.100250

Regression amplitude values:
Maximum amplitude:     0.550323
Minimum amplitude:    -0.510986
Midline amplitude:     0.019669


The clipping was caused by the fact that I failed to update
'PPB_Audio_Shared::Run', so it still relied on getting latency data from socket
instead of reading it from shared buffer.

The latest patch fixes the problem (amplitude values recovered)

[Mikhail, 2016-11-25 11:01:36]

mikhail.pozdnyakov@intel.com changed reviewers:
+ bbudge@chromium.org, raymes@chromium.org

[Mikhail, 2016-11-25 11:01:37]

bbudge@, raymes@, could you take a look at ppapi/ changes?

[commit-bot: I haz the power, 2016-11-25 12:54:16]

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

[commit-bot: I haz the power, 2016-11-25 12:54:16]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-11-28 02:19:55]

raymes@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[raymes, 2016-11-28 02:19:56]

On 2016/11/25 11:01:37, Mikhail wrote:
> bbudge@, raymes@, could you take a look at ppapi/ changes?

I don't think we know that code very well - could you get someone from the media
side to review and we can rubberstamp? +dalecurtis who might be a little
familiar with this. Thanks!

[chcunningham, 2016-11-28 22:22:50]

On 2016/11/28 02:19:56, raymes wrote:
> On 2016/11/25 11:01:37, Mikhail wrote:
> > bbudge@, raymes@, could you take a look at ppapi/ changes?
> 
> I don't think we know that code very well - could you get someone from the
media
> side to review and we can rubberstamp? +dalecurtis who might be a little
> familiar with this. Thanks!

Whoops! I'm from media, but I'm not familiar with all the sync socket usage.

These places look suspicious too
-
https://cs.chromium.org/chromium/src/ppapi/proxy/audio_input_resource.cc?rcl=...
-
https://cs.chromium.org/chromium/src/media/audio/win/audio_output_win_unittes...
(both sender and receiver side)

The unit test looks self contained, but its probably good to stay consistent.
Not sure what others I may be missing. Not sure who is on the other side of
audio_input_resource socket. Dale :)?

[DaleCurtis, 2016-11-28 22:55:46]

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:155:
buffer->params.delay_timestamp
Formatting is incorrect here; you're also not supposed to convert TimeTicks to a
TimeDelta like this. Why not just pass an actual TimeTicks value here? Or if you
don't want to pass a complex object, use the internal value converters on
base::TimeTicks.

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:176: "AudioSyncWriter:
No room in socket buffer",
This is AudioSyncReader; this should never happen here, but I guess it's fine to
log.

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
media/audio/audio_output_device.cc:472: buffer->params.delay = 0;
Why?

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
media/audio/audio_output_device.cc:477: buffer->params.delay_timestamp = 0;
Ditto?

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
ppapi/shared_impl/ppb_audio_shared.cc:235: buffer->params.delay = 0;
Again why?

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
ppapi/shared_impl/ppb_audio_shared.cc:237: PP_TimeDelta latency =
delay.InSecondsF();
Just inline into callback.

[Mikhail, 2016-11-29 14:54:50]

Thanks for your comments!

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:155:
buffer->params.delay_timestamp
On 2016/11/28 22:55:46, DaleCurtis wrote:
> Formatting is incorrect here; you're also not supposed to convert TimeTicks to
a
> TimeDelta like this. Why not just pass an actual TimeTicks value here? Or if
you
> don't want to pass a complex object, use the internal value converters on
> base::TimeTicks.

miu@ recommended to avoid using of 'ToInternalValue()' as it is often being
misused. Please see

https://codereview.chromium.org/2437863004/diff/1/content/browser/renderer_ho...

https://codereview.chromium.org/2517503003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:176: "AudioSyncWriter:
No room in socket buffer",
On 2016/11/28 22:55:46, DaleCurtis wrote:
> This is AudioSyncReader; this should never happen here, but I guess it's fine
to
> log.

In practice it never happens. I put it in case someone (like me) decides to pass
more data through the socket, it can lead to flaky bugs which will take time to
find out the route cause.

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
media/audio/audio_output_device.cc:472: buffer->params.delay = 0;
On 2016/11/28 22:55:46, DaleCurtis wrote:
> Why?
took after 'frames_skipped' pattern, but now I see assigning delay and timestamp
to zero does not make much sense. I'll remove it. Thanks for
noticing!(alternatively syncreader could check that these params are zeroed i.e.
have been read, but not sure if it brings any benefit..)

https://codereview.chromium.org/2517503003/diff/100001/media/audio/audio_outp...
media/audio/audio_output_device.cc:477: buffer->params.delay_timestamp = 0;
On 2016/11/28 22:55:46, DaleCurtis wrote:
> Ditto?

Done.

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
ppapi/shared_impl/ppb_audio_shared.cc:235: buffer->params.delay = 0;
On 2016/11/28 22:55:46, DaleCurtis wrote:
> Again why?

Done.

https://codereview.chromium.org/2517503003/diff/100001/ppapi/shared_impl/ppb_...
ppapi/shared_impl/ppb_audio_shared.cc:237: PP_TimeDelta latency =
delay.InSecondsF();
On 2016/11/28 22:55:46, DaleCurtis wrote:
> Just inline into callback.

Done.

[Mikhail, 2016-11-29 15:03:20]

On 2016/11/28 22:22:50, chcunningham wrote:
> On 2016/11/28 02:19:56, raymes wrote:
> > On 2016/11/25 11:01:37, Mikhail wrote:
> > > bbudge@, raymes@, could you take a look at ppapi/ changes?
> > 
> > I don't think we know that code very well - could you get someone from the
> media
> > side to review and we can rubberstamp? +dalecurtis who might be a little
> > familiar with this. Thanks!
> 
> Whoops! I'm from media, but I'm not familiar with all the sync socket usage.
> 
> These places look suspicious too
> -
>
https://cs.chromium.org/chromium/src/ppapi/proxy/audio_input_resource.cc?rcl=...
> -
>
https://cs.chromium.org/chromium/src/media/audio/win/audio_output_win_unittes...
> (both sender and receiver side)
> 
> The unit test looks self contained, but its probably good to stay consistent.
> Not sure what others I may be missing. Not sure who is on the other side of
> audio_input_resource socket. Dale :)?

Apparently audio_input_resource does not have AudioSyncReader on other side (as
it uses AudioInputBuffer, not AudioOutputBuffer), so should not be affected by
this change.

As for the test, it indeed needs to be modified accordingly to the new API
contract, I'll do it asap.

I wish all clients used a dedicated wrapper class that would encapsulate the
socket rather than worked directly with row socket.. That way all problems would
be detected already on compilation stage.

[Mikhail, 2016-11-30 13:47:21]

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

[commit-bot: I haz the power, 2016-11-30 13:47:41]

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

[Mikhail, 2016-11-30 14:51:21]

On 2016/11/29 15:03:20, Mikhail wrote:
> As for the test, it indeed needs to be modified accordingly to the new API
> contract, I'll do it asap.
> 
Done. PTAL.

[commit-bot: I haz the power, 2016-11-30 15:11:54]

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

[commit-bot: I haz the power, 2016-11-30 15:11:56]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2016-11-30 20:54:28]

lgtm

https://codereview.chromium.org/2517503003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://codereview.chromium.org/2517503003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:607: sine.OnMoreData(delay,
base::TimeTicks::Now(), 0, audio_bus.get());
I'd send in the delay_timestamp as well.

[DaleCurtis, 2016-11-30 22:19:05]

Just checked over the ASR stuff, defer to chcunningham@ for rest of review. lgtm

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:172: const std::string
error_message = "ASW: No room in socket buffer.";
ASR instead of ASW.

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:176: "AudioSyncWriter:
No room in socket buffer",
This still says AudioSyncWriter instead of AudioSyncReader.

[raymes, 2016-11-30 22:45:07]

rs lgtm for ppai

[Mikhail, 2016-12-01 10:34:18]

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

[commit-bot: I haz the power, 2016-12-01 10:34: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-12-01 11:08:39]

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

[commit-bot: I haz the power, 2016-12-01 11:08:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mikhail, 2016-12-01 11:12:34]

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

[commit-bot: I haz the power, 2016-12-01 11:12:49]

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

[Mikhail, 2016-12-01 11:12:55]

Patchset #8 (id:160001) has been deleted

[Mikhail, 2016-12-01 12:22:30]

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:172: const std::string
error_message = "ASW: No room in socket buffer.";
On 2016/11/30 22:19:05, DaleCurtis wrote:
> ASR instead of ASW.

Done.

https://codereview.chromium.org/2517503003/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_sync_reader.cc:176: "AudioSyncWriter:
No room in socket buffer",
On 2016/11/30 22:19:04, DaleCurtis wrote:
> This still says AudioSyncWriter instead of AudioSyncReader.

Done.

https://codereview.chromium.org/2517503003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://codereview.chromium.org/2517503003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:607: sine.OnMoreData(delay,
base::TimeTicks::Now(), 0, audio_bus.get());
On 2016/11/30 20:54:28, chcunningham wrote:
> I'd send in the delay_timestamp as well. 

Done.

[commit-bot: I haz the power, 2016-12-01 12:33:33]

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

[commit-bot: I haz the power, 2016-12-01 12:33:34]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-12-01 12:36:34]

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

[Mikhail, 2016-12-01 12:36:35]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
tommi@chromium.org, chcunningham@chromium.org, raymes@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2517503003/#ps180001
(title: "Comments from chcunningham@ and Dale")

[commit-bot: I haz the power, 2016-12-01 12:36:44]

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-12-01 12:40:58]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480595794965340,
"parent_rev": "ff53e6b4d8da5b637ff25c6f9ed3c50ba48d0b21", "commit_rev":
"322f84420f970f05ebaf53b06bde22fdb8e78785"}

[commit-bot: I haz the power, 2016-12-01 12:41:25]

Description was changed from

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-12-01 12:41:26]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-12-01 12:43:21]

Description was changed from

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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/

Initially committed at:
https://crrev.com/06d9d80a203b9c329fc6d53fde44bae66645a389
Reverted at: https://crrev.com/2665e952502835c6c37d51153bfa180343635a41
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-12-01 12:43:23]

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