Added a ParsePayload method to AudioDecoder.

webrtc/modules/audio_coding/codecs/audio_decoder.h

 /*
  *  Copyright (c) 2012 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.
  */
 
 #ifndef WEBRTC_MODULES_AUDIO_CODING_NETEQ_INCLUDE_AUDIO_DECODER_H_
 #define WEBRTC_MODULES_AUDIO_CODING_NETEQ_INCLUDE_AUDIO_DECODER_H_
 
 #include <stdlib.h>  // NULL
 
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/buffer.h"
 #include "webrtc/base/constructormagic.h"
+#include "webrtc/base/optional.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 
 // This is the interface class for decoders in NetEQ. Each codec type will have
 // and implementation of this class.
 class AudioDecoder {
  public:
   enum SpeechType {
     kSpeech = 1,
     kComfortNoise = 2
   };
 
   // Used by PacketDuration below. Save the value -1 for errors.
   enum { kNotImplemented = -2 };
 
   AudioDecoder() = default;
   virtual ~AudioDecoder() = default;
 
+  class Frame {

[hlundin-webrtc, 2016/09/09 12:11:50]

Frame is too general, and already claimed for too many different concepts in
audio processing and coding. Consider EncodedFrame.

[ossu, 2016/09/12 10:31:37]

I agree - EncodedFrame is better. EncodedAudioFrame? I'm not sure. It's a bit
long, especially when prefixed with AudioDecoder::.

Hmm...

[ossu, 2016/09/13 13:37:46]

I've gone with EncodedAudioFrame. It makes more sense for naming with the
changes I'm making to LegacyFrame in the next CL.

+   public:
+    struct DecodeResult {
+      size_t num_decoded_samples;
+      SpeechType speech_type;
+    };
+
+    virtual ~Frame() = default;
+
+    // Returns the duration in samples-per-channel of this audio frame.
+    // If no duration can be ascertained, returns zero.
+    virtual size_t Duration() const = 0;
+
+    // Decodes this frame of audio and writes the result in |decoded|.

[hlundin-webrtc, 2016/09/09 12:11:49]

What can be expected of the state of the Frame after having called Decode? Is it
dead then? What if I call Decode twice on the same object?

[ossu, 2016/09/12 10:31:36]

I'm not sure. Practically, it currently acts as if doing AudioDecoder::Decode()
twice with the same payload, but that's just the implementation and not really a
contract. Besides this issue, I'm wondering about the contract for the size of
decoded versus the length of a frame. How will the implementer of an
AudioDecoder know how long a Frame is expected to be? How large can it be? Some
guidance like:
- An AudioDecoder::Frame is expected to be about 20 ms long, but must not be
larger than X ms.
- If the input payload is longer than X ms, it must be split up into multiple
frames.

We'll either need to decide on rules such as these, or that Decode() can be
called several times, each returning a new set of samples until the Frame is
actually completely decoded, at which point it will return zero decoded samples
and the Frame will be discarded.

[ossu, 2016/09/13 13:37:46]

I've clarified that Decode should only be called once per frame. It's not yet
enforced in any way, but I guess the implementations could keep track and DCHECK
if we really wanted to.

[hlundin-webrtc, 2016/09/15 08:12:16]

Acknowledged.

+    // Returns rtc::Optional containing the total number of samples across all
+    // channels, as well as whether the decoder produced comfort noise or
+    // speech.
+    virtual rtc::Optional<DecodeResult> Decode(

[hlundin-webrtc, 2016/09/09 12:11:50]

How will error codes from the decoder be handled? Today we are storing them for
later reporting. Will this change?

[ossu, 2016/09/12 10:31:36]

No idea! You tell me! :) Are these the error codes gotten from
AudioDecoder::ErrorCode? If so, I think that information should be put either as
a getter on the Frame, or as a return value in DecodeResult - which in turn
means it no longer makes sense to make it an Optional. It could still contain
some internal Optional stuff, but by then it's probably easier to just have a
|status| field and let the rest of the fields be valid only if |status| is kOK. 

[ossu, 2016/09/12 12:37:35]

From what I can see, the decoder error code is free-form and logged at one point
in neteq_impl.cc. It is also possible to get the latest decoder error through
NetEq::LastDecoderError(). This is only used in four places, all of which are in
NetEq unit tests. I see no reason why this information could not be put into
DecodeResult directly, even if we want to retain the current functionality (i.e.
LastDecoderError()). The question is: do we? If it's only used by a couple of
unit tests.

[hlundin-webrtc, 2016/09/15 08:12:16]

Right. Since no production code seems to care about the decoder error codes, we
might just scrap that.

However, this interface should still define how a decoder error is flagged (not
specified). Does an empty return value imply an error?

+        rtc::ArrayView<int16_t> decoded) const = 0;
+  };
+
+  struct ParseResult {
+    ParseResult();
+    ParseResult(uint32_t timestamp, bool primary, std::unique_ptr<Frame> frame);
+    ParseResult(ParseResult&& b);
+    ~ParseResult();
+
+    ParseResult& operator=(ParseResult&& b);
+
+    uint32_t timestamp;

[hlundin-webrtc, 2016/09/09 12:11:50]

rtp_timestamp? Or timestamp_in_samples? G.722...

[ossu, 2016/09/12 10:31:36]

I think it should be in samples, however I'm really not sure that that is the
case right now. If we want to decouple the decoders from the specifics of the
network transport (which I strongly think we do), then we shouldn't be using RTP
timestamps to communicate between AudioDecoder and NetEq, just as we're not
sending whole (parsed) RTP packets into the AudioDecoder.

[ossu, 2016/09/13 13:37:46]

I've looked through the calling code and the timestamp is in samples already.
Yay!

[hlundin-webrtc, 2016/09/15 08:12:16]

Acknowledged.

+    bool primary;
+    std::unique_ptr<Frame> frame;
+  };

[kwiberg-webrtc, 2016/09/10 07:34:59]

Why do you need ParseResult and Frame to be two different classes?

... Ah. So that Frame can be a pure interface.

[ossu, 2016/09/12 10:31:36]

Also, the stuff in ParseResult is used to create new Packet objects by cloning
the original packet and updating timestamp, primary and (the newly added) frame.
This could be changed by having a (simpler) Packet type that is shared by NetEq
and AudioDecoder, so that the AudioDecoder can create fully-fledged packets to
be put into NetEq's PacketBuffer directly, rather than having NetEq fill in the
missing details.

+
+  // Let the decoder parse this payload and prepare zero or more decodable
+  // frames. The decoder is free to steal the contents of the payload and retain
+  // them for as long as necessary.
+  virtual std::vector<ParseResult> ParsePayload(rtc::Buffer* payload,
+                                                uint32_t timestamp,
+                                                bool is_primary);

[kwiberg-webrtc, 2016/09/10 07:34:59]

"and retain them for as long as necessary" is redundant; it follows from being
allowed to steal the contents of |payload|.

Maybe it's good to be specific, and say that the callee is allowed to swap or
move the buffer.

[ossu, 2016/09/12 10:31:37]

Yeah, I'll change it to something more specific.

[ossu, 2016/09/13 13:37:46]

Also tried to address the contract of EncodedAudioFrame here, i.e. how big a
frame is supposed to be. Should we mention an ideal frame size as well? 10 ms?
20 ms?

[hlundin-webrtc, 2016/09/15 08:12:16]

kMaxFrameSize in neteq_impl.h defines that 120 ms per frame is the maximum
allowed. And lower than 10 ms is not recommended either. Other than that, I
would argue that the ideal frame size is between 10 and 30 ms.

+
   // Decodes |encode_len| bytes from |encoded| and writes the result in
   // |decoded|. The maximum bytes allowed to be written into |decoded| is
   // |max_decoded_bytes|. Returns the total number of samples across all
   // channels. If the decoder produced comfort noise, |speech_type|
   // is set to kComfortNoise, otherwise it is kSpeech. The desired output
   // sample rate is provided in |sample_rate_hz|, which must be valid for the
   // codec at hand.
   int Decode(const uint8_t* encoded,
              size_t encoded_len,
              int sample_rate_hz,
@@ skipping 69 matching lines @@
                                       int sample_rate_hz,
                                       int16_t* decoded,
                                       SpeechType* speech_type);
 
  private:
   RTC_DISALLOW_COPY_AND_ASSIGN(AudioDecoder);
 };
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_NETEQ_INCLUDE_AUDIO_DECODER_H_