Moved codec-specific audio packet splitting into decoders.

webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc

Index: webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc
diff --git a/webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc b/webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc
index dab5805f98afcc41eaa714d49e4cd8baec5ebe59..968f91abf0f0d6000d0fc16fad4370d1fe93ead8 100644
--- a/webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc
+++ b/webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc
@@ -11,6 +11,7 @@
 #include "webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h"
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
 #include "webrtc/modules/audio_coding/codecs/ilbc/ilbc.h"
 
 namespace webrtc {
@@ -49,6 +50,44 @@ void AudioDecoderIlbc::Reset() {
   WebRtcIlbcfix_Decoderinit30Ms(dec_state_);
 }
 
+std::vector<AudioDecoder::PacketSplit> AudioDecoderIlbc::SplitPacket(
+    rtc::ArrayView<const uint8_t> payload) const {
+  std::vector<PacketSplit> splits;

[kwiberg-webrtc, 2016/09/12 02:11:01]

Allocate this one much closer to first point of use?

[ossu, 2016/09/12 11:26:37]

Acknowledged.

[ossu, 2016/09/13 14:25:55]

I looked at it again and it's used five lines down to return an empty result, so
I think it's alright. 

+  size_t bytes_per_frame;
+  int timestamps_per_frame;
+  if (payload.size() >= 950) {

[kwiberg-webrtc, 2016/09/12 02:11:01]

As I think I've noted before, the only sizes that are actually problematic are
those that are zero both mod 38 and mod 50... so you could write this as

const bool frame_size_20ms = payload.size() % 38 == 0;
const bool frame_size_30ms = payload.size() % 50 == 0;
if (frame_size_20ms && frame_size_30ms) {
  RTC_DCHECK_GE(payload.size(), 950);  // LCM(38, 50) = 950.
  // handle error
} else if (frame_size_20ms) {
  // ...
} else if (frame_size_30ms) {
  // ...
} else {
  // handle error
}

This way, it's more obvious that you're simple handling all four possible cases.

But yes, I expect that towards the end of the CL, I'll discover that you've just
moved this piece of code from somewhere else, so never mind...

[ossu, 2016/09/12 11:26:37]

Yes, that is the case. If acceptable, I'd like to defer that change to a
separate CL down the line, so that I try to avoid introducing different behavior
with this CL, rather than "just" restructuring.

[kwiberg-webrtc, 2016/09/13 12:23:37]

Acknowledged.

+    LOG(LS_WARNING) << "AudioDecoderIlbc::SplitPacket: Payload too large";
+    return splits;
+  }
+  if (payload.size() % 38 == 0) {
+    // 20 ms frames.
+    bytes_per_frame = 38;
+    timestamps_per_frame = 160;
+  } else if (payload.size() % 50 == 0) {
+    // 30 ms frames.
+    bytes_per_frame = 50;
+    timestamps_per_frame = 240;
+  } else {
+    LOG(LS_WARNING) << "AudioDecoderIlbc::SplitPacket: Invalid payload";
+    return splits;
+  }
+
+  if (payload.size() == bytes_per_frame) {
+    // Special case. Do not split the payload.
+    return {PacketSplit{0, payload.size(), 0}};
+  }

[kwiberg-webrtc, 2016/09/12 02:11:01]

Why is this special case needed? Doesn't the loop below handle this case just
fine?

[ossu, 2016/09/12 11:26:37]

It does. You mentioned that on the previous CL and I just plum forgot to fix
that for this one. I'm sorry! :(

[kwiberg-webrtc, 2016/09/13 12:23:37]

Nice to know that even if my memory isn't what it used to be IIRC, other parts
of my brain are still reliable!

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

If you recall correctly, your memory is not what is used to be. Meta...

+
+  PacketSplit split = {0, bytes_per_frame, 0};
+  for (size_t len = payload.size(); len > 0; len -= bytes_per_frame) {

[kwiberg-webrtc, 2016/09/12 02:11:01]

Better DCHECK that payload.size() % bytes_per_frame == 0 before this loop, since
that's crucial.

[ossu, 2016/09/12 11:26:37]

Hmm. I see the problem but I'm not sure that's how to best solve it. I'll
rewrite.

[kwiberg-webrtc, 2016/09/13 12:23:37]

I don't think you need to do that. Just a DCHECK immediately before the loop to
remind the reader of the invariant that makes the loop safe should be enough.

[ossu, 2016/09/13 14:25:55]

Acknowledged.

+    RTC_DCHECK(len >= bytes_per_frame);
+    splits.push_back(split);
+    split.byte_offset += bytes_per_frame;
+    split.timestamp_offset += timestamps_per_frame;
+  }
+
+  return splits;
+}
+
 int AudioDecoderIlbc::SampleRateHz() const {
   return 8000;
 }