Moved Opus-specific payload splitting into AudioDecoderOpus.

webrtc/modules/audio_coding/neteq/packet.h

Index: webrtc/modules/audio_coding/neteq/packet.h
diff --git a/webrtc/modules/audio_coding/neteq/packet.h b/webrtc/modules/audio_coding/neteq/packet.h
index 4e17a88dcd5c12bc5f90ee95702371b8cab93646..4f777490ee9d2359b88f3c8ae4ebe3ee01f4b868 100644
--- a/webrtc/modules/audio_coding/neteq/packet.h
+++ b/webrtc/modules/audio_coding/neteq/packet.h
@@ -24,10 +24,37 @@ namespace webrtc {
 
 // Struct for holding RTP packets.
 struct Packet {
+  struct Priority {
+    uint8_t codec_level;

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

I'm not super psyched about the naming here, but it was the best I could come up
with at the time. Any suggestions to change codec_level and red_level to?

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

Nope.

+    uint8_t red_level;
+
+    // Priorities are sorted low-to-high, first on the level the codec
+    // prioritizes it, then on the level of RED packet it is; i.e. if it is a
+    // primary or secondary payload of a RED packet. For example: with Opus, an
+    // Fec packet (which the decoder prioritizes lower than a regular packet)
+    // will not be used if there is _any_ RED payload for the same
+    // timeframe. The highest priority packet will have levels {0, 0}.
+    bool operator<(const Priority& b) const {
+      if (codec_level == b.codec_level)
+        return red_level < b.red_level;
+
+      return codec_level < b.codec_level;
+    }
+    bool operator==(const Priority& b) const {
+      return codec_level == b.codec_level && red_level == b.red_level;
+    }
+    bool operator!=(const Priority& b) const { return !(*this == b); }
+    bool operator>(const Priority& b) const { return b < *this; }
+    bool operator<=(const Priority& b) const { return !(b > *this); }
+    bool operator>=(const Priority& b) const { return !(b < *this); }
+  };
+
+  static constexpr Priority kHighestPriority = {0, 0};

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

I'm a bit suspicious about this constant. Why is it ever a good idea to compare
an existing priority with the highest priority, as opposed to comparing two
existing priorities?

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

I was thinking it would be clearer to set something to "the highest priority"
rather than to {0, 0} or a default-instantiated Priority. At a couple of places,
we're also comparing against what was previously "primary flag set", which is
more-or-less the same as "the highest priority".

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

Makes sense as a short-hand---*if* priority values are restricted to be
nonnegative for some other reason.
Yes, this is what I was suspicious about. Why are we doing that?

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

I think stats is one reason. To me it makes sense to have a level for
"this-is-as-good-as-it-gets" in NetEq and std::numeric_limits<int>::min() is too
annoying to write..
Bad judgment? Poor education? A rough upbringing? I think the answer is, again,
stats. Maybe that should change, though. kHighestPriority is only compared
against in two places, and both seem slightly wrong.

+
   RTPHeader header;
   // Datagram excluding RTP header and header extension.
   rtc::Buffer payload;
-  bool primary = true;  // Primary, i.e., not redundant payload.
+  Priority priority = kHighestPriority;
   std::unique_ptr<TickTimer::Stopwatch> waiting_time;
   std::unique_ptr<AudioDecoder::EncodedAudioFrame> frame;
 
@@ -41,17 +68,16 @@ struct Packet {
   // primary payload is considered "smaller" than a secondary.
   bool operator==(const Packet& rhs) const {
     return (this->header.timestamp == rhs.header.timestamp &&
-        this->header.sequenceNumber == rhs.header.sequenceNumber &&
-        this->primary == rhs.primary);
+            this->header.sequenceNumber == rhs.header.sequenceNumber &&
+            this->priority == rhs.priority);
   }
   bool operator!=(const Packet& rhs) const { return !operator==(rhs); }
   bool operator<(const Packet& rhs) const {
     if (this->header.timestamp == rhs.header.timestamp) {
       if (this->header.sequenceNumber == rhs.header.sequenceNumber) {
-        // Timestamp and sequence numbers are identical - deem the left
-        // hand side to be "smaller" (i.e., "earlier") if it is primary, and
-        // right hand side is not.
-        return (this->primary && !rhs.primary);
+        // Timestamp and sequence numbers are identical - deem the left hand
+        // side to be "smaller" (i.e., "earlier") if it has higher priority.
+        return this->priority < rhs.priority;
       }
       return (static_cast<uint16_t>(rhs.header.sequenceNumber
           - this->header.sequenceNumber) < 0xFFFF / 2);