Move AudioDecoder and related stuff to the api/ directory

webrtc/api/audio_codec/audio_decoder.cc

Index: webrtc/api/audio_codec/audio_decoder.cc
diff --git a/webrtc/modules/audio_coding/codecs/audio_decoder.cc b/webrtc/api/audio_codec/audio_decoder.cc
similarity index 66%
rename from webrtc/modules/audio_coding/codecs/audio_decoder.cc
rename to webrtc/api/audio_codec/audio_decoder.cc
index afa5115d5a1740036c566ded3c9c9019b78bb233..8697ac3f67f8f1225dfdc50265547f41b8478815 100644
--- a/webrtc/modules/audio_coding/codecs/audio_decoder.cc
+++ b/webrtc/api/audio_codec/audio_decoder.cc
@@ -8,7 +8,7 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "webrtc/modules/audio_coding/codecs/audio_decoder.h"
+#include "webrtc/api/audio_codec/audio_decoder.h"
 
 #include <assert.h>

[the sun, 2017/02/02 21:02:29]

We should use DCHECK here.

But perhaps it is better to do some due cleanups in a follow up CL? This one is
large enough as it is...

[kwiberg-webrtc, 2017/02/03 09:50:11]

Yes, exactly. I'm (mostly) just moving this file, and it's usually a good idea
to not change a bunch of unrelated things in the same CL.

 #include <memory>
@@ -18,10 +18,39 @@
 #include "webrtc/base/checks.h"
 #include "webrtc/base/sanitizer.h"
 #include "webrtc/base/trace_event.h"
-#include "webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.h"
 
 namespace webrtc {
 
+namespace {
+
+class EncFrame final : public AudioDecoder::EncodedAudioFrame {

[ossu, 2017/02/01 15:05:26]

Please pick a better name, even if it's just temporary; like FallbackFrame or
something. Also make a note that it should go away when we remove the old-style
API of calling Decode directly on the audio decoder.

[kwiberg-webrtc, 2017/02/01 20:15:02]

Will "EncodedFrame" do? That's what it is, and what EncFrame was supposed to be
short for.

[ossu, 2017/02/06 12:54:16]

No, I don't think that expresses what this is, really, and it's terribly close
to the EncodedAudioFrame interface that it implements. Aagh, naming is hard!

OldStyleEncodedFrame? OldApiEncodedFrame? And a little comment above it to
explain why it's there and that we hope to get rid of it soon?

[kwiberg-webrtc, 2017/02/06 14:19:36]

OK, I'll pick a name like that. Maybe OldStyle* or Legacy*.

[ossu, 2017/02/06 14:44:06]

Legacy might clash (at least mentally) with the other LegacyEncodedAudioFrame,
although that may be alright since they're very alike. OldStyle won't, but it
might also strange that we have such similar functionality under different
names... I'm not sure this is helping. :)

Pick one for now and we'll put this effort towards getting rid of it instead. 

+ public:
+  EncFrame(AudioDecoder* decoder, rtc::Buffer&& payload)
+      : decoder_(decoder), payload_(std::move(payload)) {}
+
+  size_t Duration() const override {
+    const int ret = decoder_->PacketDuration(payload_.data(), payload_.size());
+    return ret < 0 ? 0 : static_cast<size_t>(ret);
+  }
+
+  rtc::Optional<DecodeResult> Decode(
+      rtc::ArrayView<int16_t> decoded) const override {
+    auto speech_type = AudioDecoder::kSpeech;
+    const int ret = decoder_->Decode(
+        payload_.data(), payload_.size(), decoder_->SampleRateHz(),
+        decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
+    return ret < 0 ? rtc::Optional<DecodeResult>()
+                   : rtc::Optional<DecodeResult>(
+                         {static_cast<size_t>(ret), speech_type});
+  }
+
+ private:
+  AudioDecoder* const decoder_;

[the sun, 2017/02/02 21:02:29]

So does the decoder own all its frames, so they are deleted once the decoder is?

If not, we have an ownership ambiguity here. If you won't fix in this CL, add a
TODO/file an issue.

[kwiberg-webrtc, 2017/02/03 09:50:11]

The docs for ParsePayload, which returns these, say "The caller must ensure that
the AudioDecoder object outlives any frame objects returned by this call." I'd
say that's clear enough.

The alternative is to force every AudioDecoder implementation to have
EncodedAudioFrame instances that can survive the demise of their AudioDecoder.
This makes them harder to implement correctly, and we (i.e. NetEq) have no need
for it.

[ossu, 2017/02/06 12:54:16]

I'd argue that is unreasonable to have a frame outlive its decoder. If it were
hard to enforce these lifetimes, we _could_ allow decoders to be carried in
scoped_refptrs, which would allow frames to hold on to them until they were no
longer needed. I expect most codecs would have to have some shared state that
would have to be shared like this anyways. I'd strongly prefer if we could avoid
adding more shared ownership to the code and instead insist on whatever is using
the decoder to ensure it outlives its frames. It doesn't seem obviously
difficult to me.

+  const rtc::Buffer payload_;
+};
+
+}  // namespace
+
 AudioDecoder::ParseResult::ParseResult() = default;
 AudioDecoder::ParseResult::ParseResult(ParseResult&& b) = default;
 AudioDecoder::ParseResult::ParseResult(uint32_t timestamp,
@@ -41,14 +70,17 @@ std::vector<AudioDecoder::ParseResult> AudioDecoder::ParsePayload(
     uint32_t timestamp) {
   std::vector<ParseResult> results;
   std::unique_ptr<EncodedAudioFrame> frame(
-      new LegacyEncodedAudioFrame(this, std::move(payload)));
+      new EncFrame(this, std::move(payload)));
   results.emplace_back(timestamp, 0, std::move(frame));
   return results;
 }
 
-int AudioDecoder::Decode(const uint8_t* encoded, size_t encoded_len,
-                         int sample_rate_hz, size_t max_decoded_bytes,
-                         int16_t* decoded, SpeechType* speech_type) {
+int AudioDecoder::Decode(const uint8_t* encoded,
+                         size_t encoded_len,
+                         int sample_rate_hz,
+                         size_t max_decoded_bytes,
+                         int16_t* decoded,
+                         SpeechType* speech_type) {
   TRACE_EVENT0("webrtc", "AudioDecoder::Decode");
   rtc::MsanCheckInitialized(rtc::MakeArrayView(encoded, encoded_len));
   int duration = PacketDuration(encoded, encoded_len);
@@ -60,9 +92,12 @@ int AudioDecoder::Decode(const uint8_t* encoded, size_t encoded_len,
                         speech_type);
 }
 
-int AudioDecoder::DecodeRedundant(const uint8_t* encoded, size_t encoded_len,
-                                  int sample_rate_hz, size_t max_decoded_bytes,
-                                  int16_t* decoded, SpeechType* speech_type) {
+int AudioDecoder::DecodeRedundant(const uint8_t* encoded,
+                                  size_t encoded_len,
+                                  int sample_rate_hz,
+                                  size_t max_decoded_bytes,
+                                  int16_t* decoded,
+                                  SpeechType* speech_type) {
   TRACE_EVENT0("webrtc", "AudioDecoder::DecodeRedundant");
   rtc::MsanCheckInitialized(rtc::MakeArrayView(encoded, encoded_len));
   int duration = PacketDurationRedundant(encoded, encoded_len);
@@ -76,13 +111,16 @@ int AudioDecoder::DecodeRedundant(const uint8_t* encoded, size_t encoded_len,
 
 int AudioDecoder::DecodeRedundantInternal(const uint8_t* encoded,
                                           size_t encoded_len,
-                                          int sample_rate_hz, int16_t* decoded,
+                                          int sample_rate_hz,
+                                          int16_t* decoded,
                                           SpeechType* speech_type) {
   return DecodeInternal(encoded, encoded_len, sample_rate_hz, decoded,
                         speech_type);
 }
 
-bool AudioDecoder::HasDecodePlc() const { return false; }
+bool AudioDecoder::HasDecodePlc() const {
+  return false;
+}
 
 size_t AudioDecoder::DecodePlc(size_t num_frames, int16_t* decoded) {
   return 0;
@@ -96,7 +134,9 @@ int AudioDecoder::IncomingPacket(const uint8_t* payload,
   return 0;

[the sun, 2017/02/02 21:02:29]

Remove as many default implementations of overridable methods as possible.
Forcing implementers to consider whether they need each function is more
important than what we possibly gain by providing defaults.

[kwiberg-webrtc, 2017/02/03 09:50:11]

I agree. But not in this CL.

 }
 
-int AudioDecoder::ErrorCode() { return 0; }
+int AudioDecoder::ErrorCode() {
+  return 0;
+}
 
 int AudioDecoder::PacketDuration(const uint8_t* encoded,
                                  size_t encoded_len) const {