New class RtxReceiveStream.

webrtc/call/rtx_receive_stream.cc

Index: webrtc/call/rtx_receive_stream.cc
diff --git a/webrtc/call/rtx_receive_stream.cc b/webrtc/call/rtx_receive_stream.cc
new file mode 100644
index 0000000000000000000000000000000000000000..985778b83620c81d9558fe0d9ca519d609fb3efc
--- /dev/null
+++ b/webrtc/call/rtx_receive_stream.cc
@@ -0,0 +1,53 @@
+/*
+ *  Copyright (c) 2017 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/call/rtx_receive_stream.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
+
+namespace webrtc {
+
+RtxReceiveStream::RtxReceiveStream(
+    RtpPacketSinkInterface* media_sink,
+    const std::map<int, int>& rtx_payload_type_map,

[danilchap, 2017/05/17 12:13:05]

may be pass by value and move when settings - might save extra copy:
rtx_payload_type_map_(std::move(rtx_payload_type_map)),

[nisse-webrtc, 2017/05/17 13:19:34]

Done. A bit counter-intuitive, though. Typical case will be passing
VideoReceiveStream::Config::Rtp::rtc_payload_types.

[danilchap, 2017/05/17 13:43:56]

std::move probably is counter-intuitive [that is implementation details though],
but I can argue that passing by value rather than by const& is more intuitive.

Anyway, it is not that important in this case, just might be good to have habbit
to accept parameters by value when it might reduce number of copies at the cost
of few cheap moves.

+    uint32_t media_ssrc)
+    : media_sink_(media_sink),
+      rtx_payload_type_map_(rtx_payload_type_map),
+      media_ssrc_(media_ssrc) {}
+
+void RtxReceiveStream::OnRtpPacket(const RtpPacketReceived& rtx_packet) {
+  rtc::ArrayView<const uint8_t> payload = rtx_packet.payload();
+
+  if (payload.size() < kRtxHeaderSize) {
+    return;

[danilchap, 2017/05/17 12:13:05]

may be add a comment why drop packets silently (Think there was one in current
rtx unwind code)

[nisse-webrtc, 2017/05/17 13:19:34]

I could add a comment, but I find no guidance in
RTPPayloadRegistry::RestoreOriginalPacket.

It does have some logic to log unknown payload types, which I haven't copied
over.

[danilchap, 2017/05/17 13:43:56]

Sorry, can't find it either, probably I mixed it up with another comment.
I think the reason why it is normal and thus doesn't require any log is because
rtx packet without rtp payload are pure padding, processed by CC elsewhere and
shouldn't be forwarded to the media sink.

+  }
+
+  auto it = rtx_payload_type_map_.find(rtx_packet.PayloadType());
+  if (it == rtx_payload_type_map_.end()) {
+    return;
+  }
+  RtpPacketReceived media_packet;
+  media_packet.CopyHeaderFrom(rtx_packet);

[danilchap, 2017/05/17 12:13:05]

This is good: CopyHeaderFrom copies all extensions too.

[nisse-webrtc, 2017/05/17 13:19:34]

Including the result of IdentifyExtensions?

[danilchap, 2017/05/17 13:43:55]

yes.
(You can verify that with a test)

[nisse-webrtc, 2017/05/19 07:43:44]

I've added a few tests, but extensions still missing.

+
+  media_packet.SetSsrc(media_ssrc_);
+  media_packet.SetSequenceNumber((payload[0] << 8) + payload[1]);

[danilchap, 2017/05/17 12:13:05]

uint8_t + uint8_t will give you uint8_t, not uint16_t
may be use ByteReader from rtp_rtcp/source/byte_io.h if build rules allow to
include it:
SetSequenceNumber(ByteReader<uint16_t>::ReadBigEndian(payload.data()));

[nisse-webrtc, 2017/05/17 13:19:34]

Are you sure? Check the section on integer promotions on
http://en.cppreference.com/w/cpp/language/implicit_conversion

  In particular, arithmetic operators do not accept types smaller 
  than int as arguments, and integral promotions are automatically 
  applied after lvalue-to-rvalue conversion, if applicable. This 
  conversion always preserves the value.

As I read this, it works the same as plain C: Everything is automatically
promoted to int, which should be large enough.

[danilchap, 2017/05/17 13:43:56]

You right, I'm not sure. guess current code good as is.

+  media_packet.SetPayloadType(it->second);
+

[danilchap, 2017/05/17 12:13:05]

may be add
payload = payload.subview(kRtxHeaderSize);
after sequence_number extracted from 1st two bytes.
Then code below doesn't need to add/remove kRtxHeaderSize both for
payload.data() and payload.size()

[nisse-webrtc, 2017/05/17 13:19:34]

Done, but with a new variable |rtx_payload|.

[danilchap, 2017/05/17 13:43:55]

That is even better!

+  uint8_t* media_payload =
+      media_packet.AllocatePayload(payload.size() - kRtxHeaderSize);
+  if (!media_payload) {
+    return;

[danilchap, 2017/05/17 12:13:05]

may be RTC_DCHECK or LOG(LS_ERROR):

This might happen only in very strange setup: when rtx_packet has larger size
and capacity than default capacity (media_packet created with).

[nisse-webrtc, 2017/05/17 13:19:35]

Done.

+  }
+  memcpy(media_payload, payload.data() + kRtxHeaderSize,
+         payload.size() - kRtxHeaderSize);
+
+  media_sink_->OnRtpPacket(media_packet);
+}
+
+}  // namespace webrtc