Moved codec-specific audio packet splitting into decoders.

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

 /*
  *  Copyright (c) 2015 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/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 {
 
 AudioDecoderIlbc::AudioDecoderIlbc() {
   WebRtcIlbcfix_DecoderCreate(&dec_state_);
   WebRtcIlbcfix_Decoderinit30Ms(dec_state_);
 }
 
 AudioDecoderIlbc::~AudioDecoderIlbc() {
@@ skipping 18 matching lines @@
 }
 
 size_t AudioDecoderIlbc::DecodePlc(size_t num_frames, int16_t* decoded) {
   return WebRtcIlbcfix_NetEqPlc(dec_state_, decoded, num_frames);
 }
 
 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;
 }
 
 size_t AudioDecoderIlbc::Channels() const {
   return 1;
 }
 
 }  // namespace webrtc