Check buffer index in shared memory for input audio.

Check buffer sequence and ring buffer index in shared memory at browser/renderer boundary for input audio.

Also moves sending hardware delay over socket to store it in buffer meta data.

BUG=517871, 519336
Committed: https://crrev.com/bca680e362046b84a5742e0b7239104714141dc1
Cr-Commit-Position: refs/heads/master@{#344537}

[Henrik Grunell, 2015-08-13 15:17:01]

grunell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tommi@chromium.org

[Henrik Grunell, 2015-08-13 15:17:41]

Another shot at verifying sequence of audio input buffers over b/r boundary.

[DaleCurtis, 2015-08-13 22:00:49]

lgtm % some nits.

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

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:90:
next_buffer_id_ = 0;
Rollover is defined for unsigned values, so no need for this?

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
media/audio/audio_input_device.cc:317: if (last_buffer_id_ == UINT32_MAX)
Again, shouldn't be necessary since overflow is defined, UINT32_MAX + 1 == 0

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
media/audio/audio_parameters.h:24: uint8_t pad8[3];
?

[tommi (sloooow) - chröme, 2015-08-14 09:56:21]

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

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:87:
buffer->params.id = next_buffer_id_;
Can we have a thread check in this method? (and where we touch these member
variables)

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

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.h:41: uint32
hardware_delay_bytes) override;
good to have this provided now with the data

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
media/audio/audio_input_device.cc:313: uint32 hardware_delay_bytes =
buffer->params.hardware_delay_bytes;
nit: are these temporaries necessary?

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
media/audio/audio_parameters.h:18: struct MEDIA_EXPORT
AudioInputBufferParameters {
use #pragma pack if explicit packing is always necessary

[Henrik Grunell, 2015-08-14 12:38:32]

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

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:87:
buffer->params.id = next_buffer_id_;
On 2015/08/14 09:56:21, tommi wrote:
> Can we have a thread check in this method? (and where we touch these member
> variables)

Hmm, this is called by AIC::OnData, and it seems as the thread it is called on
varies, see
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

Currently, |socket_| is the only member accessed in different functions (expect
ctor).

How about posting to another function that does socket_->Send, add one thread
check in Write, and another for the rest of the functions? Plus comment on the
members which thread they can be accessed on.

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:90:
next_buffer_id_ = 0;
On 2015/08/13 22:00:49, DaleCurtis wrote:
> Rollover is defined for unsigned values, so no need for this?

Right, removed. (I used int first, but changed it.)

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

https://codereview.chromium.org/1293503002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.h:41: uint32
hardware_delay_bytes) override;
On 2015/08/14 09:56:21, tommi wrote:
> good to have this provided now with the data

Acknowledged.

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
media/audio/audio_input_device.cc:313: uint32 hardware_delay_bytes =
buffer->params.hardware_delay_bytes;
On 2015/08/14 09:56:21, tommi wrote:
> nit: are these temporaries necessary?

Good point, removed all of them.

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_input...
media/audio/audio_input_device.cc:317: if (last_buffer_id_ == UINT32_MAX)
On 2015/08/13 22:00:49, DaleCurtis wrote:
> Again, shouldn't be necessary since overflow is defined, UINT32_MAX + 1 == 0

Done.

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/40001/media/audio/audio_param...
media/audio/audio_parameters.h:18: struct MEDIA_EXPORT
AudioInputBufferParameters {
On 2015/08/14 09:56:21, tommi wrote:
> use #pragma pack if explicit packing is always necessary

As discussed offline, that doesn't work (for gcc at least) since it specifies
maximum alignment. Using to ALIGNAS(), thanks for the tip.

[Henrik Grunell, 2015-08-18 08:15:42]

Ping for review.

[tommi (sloooow) - chröme, 2015-08-18 12:44:37]

https://codereview.chromium.org/1293503002/diff/80001/media/audio/audio_param...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/80001/media/audio/audio_param...
media/audio/audio_parameters.h:31: AudioInputBufferParameters_not_aligned);
I think we should apply the alignas attribute on the variable that actually
needs it, which is the 'audio' member variable of AudioInputBuffer.  Then we can
have a compile assert (and I think we use static_assert now), that the offset of
the audio member within AudioInputBuffer, is == kChannelAlignment.

The compiler should inject whatever padding is required before the member
variable, to get that alignment done right.

[Henrik Grunell, 2015-08-19 14:28:38]

ptal

https://codereview.chromium.org/1293503002/diff/80001/media/audio/audio_param...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/80001/media/audio/audio_param...
media/audio/audio_parameters.h:31: AudioInputBufferParameters_not_aligned);
On 2015/08/18 12:44:37, tommi wrote:
> I think we should apply the alignas attribute on the variable that actually
> needs it, which is the 'audio' member variable of AudioInputBuffer.  Then we
can
> have a compile assert (and I think we use static_assert now), that the offset
of
> the audio member within AudioInputBuffer, is == kChannelAlignment.
> 
> The compiler should inject whatever padding is required before the member
> variable, to get that alignment done right.

Did that and hit dcheck in AudioBus::WrapMemory. As discussing and debugging
offline the problem is we rely on calculating required size by using
sizeof(AudioInputBufferParameters) + size for AudioBus data. This is done in
several places. So sticking with this seems like the best way.

[tommi (sloooow) - chröme, 2015-08-19 14:45:38]

lgtm

[Henrik Grunell, 2015-08-19 18:57:29]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-08-19 18:57:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1293503002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-19 18:58:13]

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

[commit-bot: I haz the power, 2015-08-19 19:51:41]

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

[commit-bot: I haz the power, 2015-08-19 19:51:42]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Henrik Grunell, 2015-08-20 09:38:27]

Fixed Win compile error, I hope.

[Henrik Grunell, 2015-08-20 09:38:36]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-08-20 09:38:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1293503002/#ps160001
(title: "Fix compile error on Windows.")

[commit-bot: I haz the power, 2015-08-20 09:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293503002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293503002/160001

[commit-bot: I haz the power, 2015-08-20 10:13:14]

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

[commit-bot: I haz the power, 2015-08-20 10:13:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Henrik Grunell, 2015-08-20 13:42:17]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-08-20 13:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1293503002/#ps180001
(title: "Now the Win compile error should really be fixed.")

[commit-bot: I haz the power, 2015-08-20 13:43:04]

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

[Henrik Grunell, 2015-08-20 14:13:48]

Now really fixed the compile error on Win. (Patch set 10.)

[tommi (sloooow) - chröme, 2015-08-20 14:59:58]

lgtm

https://codereview.chromium.org/1293503002/diff/180001/media/audio/audio_para...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/180001/media/audio/audio_para...
media/audio/audio_parameters.h:28: struct MEDIA_EXPORT ALIGNAS(16)
AudioInputBufferParameters {
maybe add a comment here that 16 == kChannelAlignment

[Henrik Grunell, 2015-08-20 16:59:37]

https://codereview.chromium.org/1293503002/diff/180001/media/audio/audio_para...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1293503002/diff/180001/media/audio/audio_para...
media/audio/audio_parameters.h:28: struct MEDIA_EXPORT ALIGNAS(16)
AudioInputBufferParameters {
On 2015/08/20 14:59:58, tommi wrote:
> maybe add a comment here that 16 == kChannelAlignment

Follow-up: https://codereview.chromium.org/1302953002/

[commit-bot: I haz the power, 2015-08-20 18:05:20]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-20 18:06:28]

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