Moved Opus-specific payload splitting into AudioDecoderOpus.

webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.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/opus/audio_decoder_opus.h"
 
+#include <utility>
+
 #include "webrtc/base/checks.h"
 
 namespace webrtc {
 
+namespace {
+class OpusFrame : public AudioDecoder::EncodedAudioFrame {
+ public:
+  OpusFrame(AudioDecoderOpus* decoder,
+            rtc::Buffer&& payload,
+            bool is_primary_payload)
+      : decoder_(decoder), is_primary_payload_(is_primary_payload) {
+    using std::swap;
+    swap(this->payload_, payload);

[hlundin-webrtc, 2016/09/15 09:33:30]

std::move?

[ossu, 2016/09/15 12:22:53]

Acknowledged.

+  }
+
+  size_t Duration() const override {
+    int ret;
+    if (is_primary_payload_) {
+      ret = decoder_->PacketDuration(payload_.data(), payload_.size());
+    } else {
+      ret = decoder_->PacketDurationRedundant(payload_.data(), payload_.size());
+    }

[kwiberg-webrtc, 2016/09/19 11:07:49]

const:

const int ret = (is_primary_payload_ ? decoder_->PacketDuration
                                     : decoder_->PacketDurationRedundant)(
    payload_.data(), payload_.size());

or however clang-format wants to lay that out...

[ossu, 2016/09/19 11:41:01]

Oh, god, no! I'll be surprised if it works but even if it does, I don't like it.
Trinary isn't always better than if/else. :)
I could _possibly_ imagine splitting it into two, first getting an auto
duration_function, then calling it on the line below, but we'll see...

[kwiberg-webrtc, 2016/09/19 11:55:16]

Hmm, you're right, I don't think it will work. And if you're not interested in
?:, rewriting it with lambdas won't be an improvement.

[hlundin-webrtc, 2016/09/19 12:19:13]

I'm with ossu: "Oh, God, no!"

[ossu, 2016/09/19 14:07:33]

?: is fine but it's easy to end up with a big, confusing expression. I opt for
leaving this as-is for now, with a look towards more restructuring when we
simplify the set of methods on AudioDecoder.

+    return (ret < 0) ? 0 : static_cast<size_t>(ret);
+  }
+
+  rtc::Optional<DecodeResult> Decode(
+      rtc::ArrayView<int16_t> decoded) const override {
+    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);
+    }

[kwiberg-webrtc, 2016/09/19 11:07:49]

The same ?: construction can be used to make ret const here (and there's a much
longer argument list to dedupe).

[kwiberg-webrtc, 2016/09/19 11:55:16]

No, it won't work for the same reason as above.

+
+    if (ret < 0)
+      return rtc::Optional<DecodeResult>();
+
+    return rtc::Optional<DecodeResult>({static_cast<size_t>(ret), speech_type});
+  }
+
+ private:
+  AudioDecoderOpus* decoder_;

[hlundin-webrtc, 2016/09/15 09:33:30]

const, const, const

[ossu, 2016/09/15 12:22:53]

Acknowledged. Acknowledged. Acknowledged.

[ossu, 2016/09/19 11:41:01]

Acknowledged.

[kwiberg-webrtc, 2016/09/19 11:55:16]

Acknowledged.

[hlundin-webrtc, 2016/09/19 12:19:13]

Roger that.

[kwiberg-webrtc, 2016/09/20 09:14:45]

Roger, Roger. What's our vector, Victor?

+  rtc::Buffer payload_;
+  bool is_primary_payload_;

[kwiberg-webrtc, 2016/09/19 11:07:49]

Not sure if this is what Henrik was pointing out, but can these two be const
too?

[hlundin-webrtc, 2016/09/19 12:19:13]

That is why I put three "const" in one comment. I was hoping to have them spread
out over the three adjacent lines, not all being applied to decoder_ (although I
guess that is possible).

[ossu, 2016/09/19 14:07:33]

I was only able to get two consts into the decoder_ line, and then it didn't
compile. :(

+};
+
+}  // namespace
+
 AudioDecoderOpus::AudioDecoderOpus(size_t num_channels)
     : channels_(num_channels) {
   RTC_DCHECK(num_channels == 1 || num_channels == 2);
   WebRtcOpus_DecoderCreate(&dec_state_, channels_);
   WebRtcOpus_DecoderInit(dec_state_);
 }
 
 AudioDecoderOpus::~AudioDecoderOpus() {
   WebRtcOpus_DecoderFree(dec_state_);
 }
 
+std::vector<AudioDecoder::ParseResult> AudioDecoderOpus::ParsePayload(
+    rtc::Buffer* payload,
+    uint32_t timestamp) {
+  std::vector<ParseResult> results;
+
+  if (PacketHasFec(payload->data(), payload->size())) {
+    const int duration =

[hlundin-webrtc, 2016/09/15 09:33:29]

What if PacketDurationRedundant returns an error (-1)?

[ossu, 2016/09/15 12:22:53]

I can't see that it's able to: it will call WebRtcOpus_FecDurationEst which in
turn calls opus_packet_get_samples_per_frame, whose return value is either >=
480 && <= 5760 or it returns 0.
Could to an RTC_DCHECK just to be safe, I guess.

[hlundin-webrtc, 2016/09/16 07:51:27]

But the method comment for AudioDecoder::PacketDuration(Redundant) says "... -1
in case of an error".

[ossu, 2016/09/19 11:41:01]

But it is only used within this implementation, i.e. there's no chance
PacketDurationRedundant _will_ return -1, even though the contract says it
could. Would you find it acceptable if I were to replace the call to
PacketdurationRedundant with a call to WebRtcOpus_FecDurationEst directly?
Eventually, I think we'll want to get rid of most of the "classic"
Decode*/PacketDuration* calls, if possible.

[hlundin-webrtc, 2016/09/19 12:19:13]

You are right. Since this is an internal affair of this implementation, I'd
settle for a DCHECK here.

[ossu, 2016/09/19 14:07:33]

Acknowledged.

+        PacketDurationRedundant(payload->data(), payload->size());
+    rtc::Buffer payload_copy(payload->data(), payload->size());
+    std::unique_ptr<EncodedAudioFrame> fec_frame(
+        new OpusFrame(this, std::move(payload_copy), false));
+    results.emplace_back(timestamp - duration, 1, std::move(fec_frame));
+  }
+  std::unique_ptr<EncodedAudioFrame> frame(
+      new OpusFrame(this, std::move(*payload), true));
+  results.emplace_back(timestamp, 0, std::move(frame));
+  return results;
+}
+
 int AudioDecoderOpus::DecodeInternal(const uint8_t* encoded,
                                      size_t encoded_len,
                                      int sample_rate_hz,
                                      int16_t* decoded,
                                      SpeechType* speech_type) {
   RTC_DCHECK_EQ(sample_rate_hz, 48000);
   int16_t temp_type = 1;  // Default is speech.
   int ret =
       WebRtcOpus_Decode(dec_state_, encoded, encoded_len, decoded, &temp_type);
   if (ret > 0)
@@ skipping 51 matching lines @@
 
 int AudioDecoderOpus::SampleRateHz() const {
   return 48000;
 }
 
 size_t AudioDecoderOpus::Channels() const {
   return channels_;
 }
 
 }  // namespace webrtc