Pass audio to AudioEncoder::Encode() in an ArrayView

Pass audio to AudioEncoder::Encode() in an ArrayView

Instead of in separate pointer and size arguments.

Committed: https://crrev.com/288886b2ec9a2dac730f115e9c3079d8439efe60
Cr-Commit-Position: refs/heads/master@{#10535}

[kwiberg-webrtc, 2015-11-03 12:39:28]

Description was changed from

==========
Pass audio to AudioEncoder::Encode() in an ArrayView

Instead of in separate pointer and size arguments.
==========

to

==========
Pass audio to AudioEncoder::Encode() in an ArrayView

Instead of in separate pointer and size arguments.
==========

[kwiberg-webrtc, 2015-11-03 12:39:28]

kwiberg@webrtc.org changed reviewers:
+ andrew@webrtc.org, henrik.lundin@webrtc.org, solenberg@webrtc.org

[kwiberg-webrtc, 2015-11-03 12:41:10]

Please take a look.

Andrew, Fredrik: webrtc/base
Henrik: all of it, especially the rest, but webrtc/base too if you're interested

[the sun, 2015-11-04 12:32:18]

I only looked quickly at the files outside of webrtc/base, but I didn't see a
lot of use for the methods added on ArrayView? Nevertheless, I'm fine with the
operator==/!=, but don't think implicit conversion to bool should be there. The
client can check size().

For the rest of the files, many interfaces now take an "ArrayView<..>"; is there
any point for them to not being passed as "const ArrayView<..>&"s?

https://codereview.webrtc.org/1418423010/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1418423010/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:105:
std::copy(audio.cbegin(), audio.cend(),
What does this compile to? Is std::copy special cased for memcopies?

[kwiberg-webrtc, 2015-11-04 14:58:11]

https://codereview.webrtc.org/1418423010/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1418423010/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:105:
std::copy(audio.cbegin(), audio.cend(),
On 2015/11/04 12:32:18, the sun wrote:
> What does this compile to? Is std::copy special cased for memcopies?

I assume so. [...] OK, I checked. Clang compiles this

  void f(ArrayView<int> av, int* out) {
    std::copy(av.begin(), av.end(), out);
  }

into this

0000000000400660 <_Z1f9ArrayViewIiEPi>:
  400660:       50                      push   %rax
  400661:       48 89 f0                mov    %rsi,%rax
  400664:       48 89 f9                mov    %rdi,%rcx
  400667:       48 85 c0                test   %rax,%rax
  40066a:       74 12                   je     40067e <_Z1f9ArrayViewIiEPi+0x1e>
  40066c:       48 c1 e0 02             shl    $0x2,%rax
  400670:       48 89 d7                mov    %rdx,%rdi
  400673:       48 89 ce                mov    %rcx,%rsi
  400676:       48 89 c2                mov    %rax,%rdx
  400679:       e8 d2 fe ff ff          callq  400550 <memmove@plt>
  40067e:       58                      pop    %rax
  40067f:       c3                      retq   

It ends up calling memmove instead of memcpy, but I call that good enough.

[kwiberg-webrtc, 2015-11-04 15:05:49]

On 2015/11/04 12:32:18, the sun wrote:
> I only looked quickly at the files outside of webrtc/base, but I didn't see a
> lot of use for the methods added on ArrayView? Nevertheless, I'm fine with the
> operator==/!=, but don't think implicit conversion to bool should be there.
The
> client can check size().

Fair enough. In that case, I'll add an empty() method; that's how the STL does
it.

> For the rest of the files, many interfaces now take an "ArrayView<..>"; is
there
> any point for them to not being passed as "const ArrayView<..>&"s?

The same reason why we pass other small and trivial types such as int by value
instead of by reference: easier-to-read code, and not forcing the compiler to
spill them to the stack just so they can have an address.

These two

  void f1(ArrayView<int> av, int* out) {
    std::copy(av.begin(), av.end(), out);
  }

  void f2(const ArrayView<int>& av, int* out) {
    std::copy(av.begin(), av.end(), out);
  }

compile to this

0000000000400660 <_Z2f19ArrayViewIiEPi>:
  400660:       50                      push   %rax
  400661:       48 89 f0                mov    %rsi,%rax
  400664:       48 89 f9                mov    %rdi,%rcx
  400667:       48 85 c0                test   %rax,%rax
  40066a:       74 12                   je     40067e
<_Z2f19ArrayViewIiEPi+0x1e>
  40066c:       48 c1 e0 02             shl    $0x2,%rax
  400670:       48 89 d7                mov    %rdx,%rdi
  400673:       48 89 ce                mov    %rcx,%rsi
  400676:       48 89 c2                mov    %rax,%rdx
  400679:       e8 d2 fe ff ff          callq  400550 <memmove@plt>
  40067e:       58                      pop    %rax
  40067f:       c3                      retq   

0000000000400680 <_Z2f2RK9ArrayViewIiEPi>:
  400680:       50                      push   %rax
  400681:       48 8b 57 08             mov    0x8(%rdi),%rdx
  400685:       48 85 d2                test   %rdx,%rdx
  400688:       74 12                   je     40069c
<_Z2f2RK9ArrayViewIiEPi+0x1c>
  40068a:       48 c1 e2 02             shl    $0x2,%rdx
  40068e:       48 8b 07                mov    (%rdi),%rax
  400691:       48 89 f7                mov    %rsi,%rdi
  400694:       48 89 c6                mov    %rax,%rsi
  400697:       e8 b4 fe ff ff          callq  400550 <memmove@plt>
  40069c:       58                      pop    %rax
  40069d:       c3                      retq   

Note that while f1 uses one more instruction, f2 has to read two words from
memory (and its caller must have done the work to put them there).

[kwiberg-webrtc, 2015-11-04 18:17:18]

On 2015/11/04 15:05:49, kwiberg-webrtc wrote:

> On 2015/11/04 12:32:18, the sun wrote:
>
> > I only looked quickly at the files outside of webrtc/base, but I didn't see
a
> > lot of use for the methods added on ArrayView?

No, they were needed in just a few places.

> > Nevertheless, I'm fine with the
> > operator==/!=, but don't think implicit conversion to bool should be there.
> > The client can check size().
> 
> Fair enough. In that case, I'll add an empty() method; that's how the STL does
> it.

Done. New patch set posted.

[Andrew MacDonald, 2015-11-05 01:39:55]

webrtc/base lgtm

[the sun, 2015-11-05 09:24:32]

On 2015/11/05 01:39:55, Andrew MacDonald wrote:
> webrtc/base lgtm

LGTM for webrtc/base ; empty() is good, all I wanted to avoid was the implicit
conversion. Thanks for showing the compiler output!

[hlundin-webrtc, 2015-11-05 15:22:28]

lgtm

https://codereview.webrtc.org/1418423010/diff/40001/webrtc/base/array_view_un...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/1418423010/diff/40001/webrtc/base/array_view_un...
webrtc/base/array_view_unittest.cc:223: TEST(ArrayViewTest, TestCompare) {
Is it conceivable to have two ArrayViews with the same data pointer but
different lengths? If so, you're not testing that case here.

[kwiberg-webrtc, 2015-11-05 19:00:07]

https://codereview.webrtc.org/1418423010/diff/40001/webrtc/base/array_view_un...
File webrtc/base/array_view_unittest.cc (right):

https://codereview.webrtc.org/1418423010/diff/40001/webrtc/base/array_view_un...
webrtc/base/array_view_unittest.cc:223: TEST(ArrayViewTest, TestCompare) {
On 2015/11/05 15:22:28, hlundin-webrtc wrote:
> Is it conceivable to have two ArrayViews with the same data pointer but
> different lengths? If so, you're not testing that case here.

Yes, that's of course entirely possible. Will add test for that, and for the
case where the lengths are the same.

[kwiberg-webrtc, 2015-11-05 19:08:18]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2015-11-05 19:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, henrik.lundin@webrtc.org, andrew@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1418423010/#ps80001 (title:
"rebase")

[commit-bot: I haz the power, 2015-11-05 19:08:40]

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

[commit-bot: I haz the power, 2015-11-05 21:09:03]

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

[commit-bot: I haz the power, 2015-11-05 21:09:07]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2015-11-05 21:19:19]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-11-05 21:19:40]

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

[commit-bot: I haz the power, 2015-11-05 23:19:52]

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

[commit-bot: I haz the power, 2015-11-05 23:19:53]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2015-11-06 09:16:58]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-11-06 09:17:16]

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

[commit-bot: I haz the power, 2015-11-06 09:21:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-06 09:21:48]

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