Now using rtc::Buffer in FineAudioBuffer

Now using rtc::Buffer in FineAudioBuffer

BUG=b/35589717

Review-Url: https://codereview.webrtc.org/2706923006
Cr-Commit-Position: refs/heads/master@{#16793}
Committed: https://chromium.googlesource.com/external/webrtc/+/f166e1bcaba8b873205b59633bbdc99efe293f1c

[henrika_webrtc, 2017-02-22 14:43:03]

Description was changed from

==========
Now using rtc::Buffer in FineAudioBuffer

Now using rtc::Buffer in FineAudioBuffer

BUG=
==========

to

==========
Now using rtc::Buffer in FineAudioBuffer

BUG=b/35589717
==========

[henrika_webrtc, 2017-02-22 14:43:04]

henrika@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[henrika_webrtc, 2017-02-22 14:44:42]

As discussed. Started out with replacing old unique_ptr<int8_t[]> to
rtc::Buffer.

Plan was to add CHECK once we are OK with first round.

Verified that audio works well as is using different WebRTC audio clients.

[kwiberg-webrtc, 2017-02-22 15:25:39]

https://codereview.webrtc.org/2706923006/diff/1/webrtc/modules/audio_device/f...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2706923006/diff/1/webrtc/modules/audio_device/f...
webrtc/modules/audio_device/fine_audio_buffer.cc:128:
record_buffer_.AppendData(buffer, size_in_bytes);
I don't think this is quite right. .capacity() is only useful if you want to
know when the Buffer will do internal reallocations.

What I think you should do when new data arrives is this:

  record_buffer_.AppendData(buffer, size_in_bytes);

Predictably, this will append new data to the Buffer, growing it if necessary.
And when consuming x bytes of data:

  DCHECK(x <= record_buffer_.size());
  DoSomething(record_buffer_.data(), x);
  memmove(record_buffer_.data(), record_buffer_.data() + x,
          record_buffer_.size() - x);
  record_buffer_.SetSize(record_buffer_.size() - x);

This will use the first x bytes of data, then move the remaining data to the
front and truncate the Buffer.

No separate read or write position is needed; you always read from the beginning
of the Buffer, and write at the end.

Of course, there are ways to optimize this so that you need to memmove less, but
this is the straightforward way of doing it.

[henrika_webrtc, 2017-02-22 16:30:25]

Rewrote as suggested. Agree that the code looks much nicer now.

But...it feels as if the loopback latency increases if I do more than one call
now. I am almost certain that it is the case. I do call Buffer::Clear() in
between.

Have added logging but can't find that and initial delay should be built up but
it does feel like it to me.

Any ideas if something else should be flushed between calls now with the new
approach?

https://codereview.webrtc.org/2706923006/diff/1/webrtc/modules/audio_device/f...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2706923006/diff/1/webrtc/modules/audio_device/f...
webrtc/modules/audio_device/fine_audio_buffer.cc:128:
record_buffer_.AppendData(buffer, size_in_bytes);
Great comments. Will fix!

[kwiberg-webrtc, 2017-02-23 08:08:24]

Much easier to understand than the old code! lgtm, provided the optimization I
mentioned isn't necessary.

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:113:
RTC_DCHECK_LE(bytes_per_10_ms_, record_buffer_.size());
You just checked this on the line before. :-)

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:121: }
Hmm. You repeatedly take a bite from the front, then move the rest of the queue
forward. A simple optimization---in case this loop runs more than once some of
the time---would be to do just one memmove+SetSize after the loop.

[kwiberg-webrtc, 2017-02-23 08:18:24]

On 2017/02/22 16:30:25, henrika_webrtc wrote:
> Rewrote as suggested. Agree that the code looks much nicer now.
> 
> But...it feels as if the loopback latency increases if I do more than one call
> now. I am almost certain that it is the case. I do call Buffer::Clear() in
> between.

.Clear() is just the same as .SetSize(0), so there should be no surprising
performance side effects. The first few times you call .AppendData() the buffer
will reallocate its heap block, each time to at least 1.5x the previous size;
the heap block is never made smaller by calls like .SetSize(), so after a short
warm-up phase you won't be making new allocations anymore. (If you have a good
guess of how large the buffer needs to be, you can ask the Buffer for that
amount of initial capacity, and skip the warm-up phase. But don't do this unless
you in fact have a good guess, since otherwise you'll either have to reallocate
anyway, or you'll waste memory.)

Does the optimization I suggested help? It might, if that loop typically runs
many iterations.

[henrika_webrtc, 2017-02-23 08:27:49]

A warm-up phase is OK. The problem I see is an increased latency during the
complete call. Only happens for call 2,3,.... Not the first call.

[henrika_webrtc, 2017-02-23 08:39:05]

Same happens without the change. Hence not related to this fix.
I am using an unofficial Android build. Does not happen on older devices.
Will land as is.

[henrika_webrtc, 2017-02-23 08:39:11]

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:113:
RTC_DCHECK_LE(bytes_per_10_ms_, record_buffer_.size());
Man ;-) So, I guess we are fine without a CHECK with the new design.

https://codereview.webrtc.org/2706923006/diff/20001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:121: }
Thanks but a double-loop is rare. I have not been able to trigger it on any of
my devices. I added it as a safety step.

[henrika_webrtc, 2017-02-23 09:23:57]

The CQ bit was checked by henrika@webrtc.org

[henrika_webrtc, 2017-02-23 09:23:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2706923006/#ps60001 (title:
"nit")

[commit-bot: I haz the power, 2017-02-23 09:24:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 10:44:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487841837457840,
"parent_rev": "02f994b4e9ffdf6fddb0e52818785a1f28c4db26", "commit_rev":
"f166e1bcaba8b873205b59633bbdc99efe293f1c"}

[commit-bot: I haz the power, 2017-02-23 10:44:58]

Description was changed from

==========
Now using rtc::Buffer in FineAudioBuffer

BUG=b/35589717
==========

to

==========
Now using rtc::Buffer in FineAudioBuffer

BUG=b/35589717

Review-Url: https://codereview.webrtc.org/2706923006
Cr-Commit-Position: refs/heads/master@{#16793}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f166e1bcaba8b873205b59633...
==========

[commit-bot: I haz the power, 2017-02-23 10:44:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/f166e1bcaba8b873205b59633...