Moved codec-specific audio packet splitting into decoders.

webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc

Index: webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc
diff --git a/webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc b/webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fd79dc9dd0eb6aa40846020c34d0ecd86564fdff
--- /dev/null
+++ b/webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc
@@ -0,0 +1,102 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h"
+
+#include <algorithm>
+#include <memory>
+#include <utility>
+
+namespace webrtc {
+
+LegacyEncodedAudioFrame::LegacyEncodedAudioFrame(AudioDecoder* decoder,
+                                                 rtc::Buffer* payload,

[hlundin-webrtc, 2016/09/15 08:49:14]

As discussed offline, and simply for the record, I'd prefer to use an rvalue ref
here. But, style guide says no.

[ossu, 2016/09/15 08:58:11]

I'd like that too. It's almost (but not completely) without precedent in WebRTC
code. A couple of constructors take an rvalue reference and some other
parameters, but it's far from the norm.

[kwiberg-webrtc, 2016/09/15 09:03:43]

Pass it by value, then.

For cheap-to-move move-only types (such as std::unique_ptr and rtc::Buffer)
pass-by-value is a good substitute for pass-by-rvalue-reference. I still agree
that passing by rvalue reference would be better here, since it may not be well
known that rtc::Buffer isn't copyable, but if you'd rather follow the style
guide than write easy-to-understand code, pass-by-value will work nicely here.

[ossu, 2016/09/15 09:15:13]

Of course I don't _prefer_ the style guide over easy-to-understand code. I
personally think references most places we now have pointers would be clearer of
intent. The style guide does, however, work as some sort of arbiter over what is
easy to understand, I guess.

We can probably get away with putting an rvalue reference in here, since it's a
constructor and it's a bit tucked away. I've realized this discussion should
also encompass AudioDecoder::ParsePayload. Since that method is intended to
steal the data, we should find a clear and uniform way of indicating that in
future interfaces. Passing rtc::Buffers by-value might be the only currently
acceptable variant.

[kwiberg-webrtc, 2016/09/15 09:29:53]

Sorry if I sounded a bit aggressive. But it is the case here that following the
style guide's prohibition of rvalue reference parameters leads to
harder-to-understand code, so you do have to make a choice.

As for the larger issue: the prohibition of mutable lvalue reference parameters
is distinct from the prohibition of rvalue reference parameters in my opinion,
since the rationales are different. The former is forbidden because it's hard to
tell at the call site that the callee may mutate the argument, whereas the
latter has no such problem, but is instead prohibited on the theory that rvalue
references are too unfamiliar to people. My position is that the lvalue ref
prohibition is good or at least OK, but that the rvalue ref prohibition should
be abolished because the increasingly small readability win doesn't outweigh the
cost.

[hlundin-webrtc, 2016/09/15 09:43:38]

I tricked you into discussing the merits of the style guide and C++11 features.
My work here is done.

[ossu, 2016/09/15 11:56:21]

So are we saying rtc::Buffer&& in LegacyEncodedAudioFrame _and_ in ParsePayload?
I'd be OK with that! Having them take a pointer is a bit dangerous. If a
subclass would _just_ copy the pointer, it would work for now, but break at some
point in the future. Forcing a copy or a move from the Buffer by passing as
value or rvalue-reference avoids that.
Oh, you! *shakes fist*

[kwiberg-webrtc, 2016/09/15 12:06:43]

Yes, I'd say taking an rtc::Buffer&& argument is best. Since we don't have a
styleguide owner team or anything like that, there's no one official to request
permission from when breaking the rules seems like the best option, so Henrik
and I will simply have to promise to look the other way when you land this. :-)

[ossu, 2016/09/15 14:41:05]

FOR GLORY!

+                                                 bool is_primary_payload)
+    : decoder_(decoder),
+      payload_(std::move(*payload)),
+      is_primary_payload_(is_primary_payload) {}
+
+LegacyEncodedAudioFrame::~LegacyEncodedAudioFrame() = default;
+
+size_t LegacyEncodedAudioFrame::Duration() const {
+  int ret;
+  if (is_primary_payload_) {
+    ret = decoder_->PacketDuration(payload_.data(), payload_.size());
+  } else {
+    ret = decoder_->PacketDurationRedundant(payload_.data(), payload_.size());
+  }
+  return (ret < 0) ? 0 : static_cast<size_t>(ret);
+}
+
+rtc::Optional<AudioDecoder::EncodedAudioFrame::DecodeResult>
+LegacyEncodedAudioFrame::Decode(rtc::ArrayView<int16_t> decoded) const {
+  AudioDecoder::SpeechType speech_type = AudioDecoder::kSpeech;
+  int ret;
+  if (is_primary_payload_) {
+    ret = decoder_->Decode(
+        payload_.data(), payload_.size(), decoder_->SampleRateHz(),
+        decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
+  } else {
+    ret = decoder_->DecodeRedundant(
+        payload_.data(), payload_.size(), decoder_->SampleRateHz(),
+        decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
+  }
+
+  if (ret < 0)
+    return rtc::Optional<DecodeResult>();
+
+  return rtc::Optional<DecodeResult>({static_cast<size_t>(ret), speech_type});
+}
+
+std::vector<AudioDecoder::ParseResult> LegacyEncodedAudioFrame::SplitBySamples(
+    AudioDecoder* decoder,
+    rtc::Buffer* payload,
+    uint32_t timestamp,
+    bool is_primary,
+    size_t bytes_per_ms,
+    uint32_t timestamps_per_ms) {
+  RTC_DCHECK(payload->data());
+  std::vector<AudioDecoder::ParseResult> results;
+  size_t split_size_bytes = payload->size();
+
+  // Find a "chunk size" >= 20 ms and < 40 ms.
+  const size_t min_chunk_size = bytes_per_ms * 20;
+  if (min_chunk_size >= payload->size()) {
+    std::unique_ptr<LegacyEncodedAudioFrame> frame(
+        new LegacyEncodedAudioFrame(decoder, payload, is_primary));
+    results.emplace_back(timestamp, is_primary, std::move(frame));
+  } else {
+    // Reduce the split size by half as long as |split_size_bytes| is at least
+    // twice the minimum chunk size (so that the resulting size is at least as
+    // large as the minimum chunk size).
+    while (split_size_bytes >= 2 * min_chunk_size) {
+      split_size_bytes >>= 1;

[hlundin-webrtc, 2016/09/15 08:49:14]

We could get rid of this very questionable "optimization" and simply /= 2
instead.

[ossu, 2016/09/15 08:58:11]

Acknowledged.

[kwiberg-webrtc, 2016/09/15 13:01:25]

+1. split_size_bytes is unsigned, so the compiler will generate the same code
anyway.

+    }
+
+    const uint32_t timestamps_per_chunk = static_cast<uint32_t>(
+        split_size_bytes * timestamps_per_ms / bytes_per_ms);
+    for (size_t byte_offset = 0, timestamp_offset = 0;
+         byte_offset < payload->size();
+         byte_offset += split_size_bytes,
+             timestamp_offset += timestamps_per_chunk) {
+      split_size_bytes =
+          std::min(split_size_bytes, payload->size() - byte_offset);
+      rtc::Buffer new_payload(payload->data() + byte_offset, split_size_bytes);
+      std::unique_ptr<LegacyEncodedAudioFrame> frame(
+          new LegacyEncodedAudioFrame(decoder, &new_payload, is_primary));
+      results.emplace_back(timestamp + timestamp_offset, is_primary,
+                           std::move(frame));
+    }
+  }
+
+  return results;
+}
+
+}  // namespace webrtc