Moved Opus-specific payload splitting into AudioDecoderOpus.

webrtc/modules/audio_coding/neteq/packet.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_PACKET_H_
 #define WEBRTC_MODULES_AUDIO_CODING_NETEQ_PACKET_H_
 
 #include <list>
 #include <memory>
 
 #include "webrtc/base/buffer.h"
 #include "webrtc/modules/audio_coding/codecs/audio_decoder.h"
 #include "webrtc/modules/audio_coding/neteq/tick_timer.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/typedefs.h"
 
 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;
 
   Packet();
   ~Packet();
 
   // Comparison operators. Establish a packet ordering based on (1) timestamp,
   // (2) sequence number and (3) redundancy.
   // Timestamp and sequence numbers are compared taking wrap-around into
   // account. For two packets with the same sequence number and timestamp a
   // 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);
     }
     return (static_cast<uint32_t>(rhs.header.timestamp
         - this->header.timestamp) < 0xFFFFFFFF / 2);
   }
   bool operator>(const Packet& rhs) const { return rhs.operator<(*this); }
   bool operator<=(const Packet& rhs) const { return !operator>(rhs); }
   bool operator>=(const Packet& rhs) const { return !operator<(rhs); }
 
   bool empty() const { return !frame && payload.empty(); }
 };
 
 // A list of packets.
 typedef std::list<Packet*> PacketList;
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_NETEQ_PACKET_H_