Make Call::OnRecoveredPacket parse RTP header and call OnRtpPacket.

webrtc/video/rtp_stream_receiver.cc

Index: webrtc/video/rtp_stream_receiver.cc
diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc
index cb1ffc90445ba869c6465ee97386e738eaa76209..264e031b49b83b02654c37f9f995b734ed01234c 100644
--- a/webrtc/video/rtp_stream_receiver.cc
+++ b/webrtc/video/rtp_stream_receiver.cc
@@ -291,6 +291,9 @@ int32_t RtpStreamReceiver::OnReceivedPayloadData(
   return 0;
 }
 
+// TODO(nisse): Try to delete this method. Obstacles: It is used by
+// ParseAndHandleEncapsulatingHeader, for handling Rtx packets. And
+// it's part of the RtpData interface which we implement.
 bool RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet,

[nisse-webrtc, 2017/02/14 10:24:33]

Is there any easy way to get rid of this method and the |rtp_header_parser_|
now? It doesn't seem quite right to pass rtx, red or ulpfec packets (it's not
entirely clear which of these use this method, and I don't really know what they
are, either) back to call just for parsing.

It would make more sense to pass them back to an RtpTransportReceiveController,
when we get that far with the refactoring.

[brandtr, 2017/02/15 12:20:50]

No, RTX, RED, and ULPFEC should not go back to Call, because they all belong to
the stream tied to this RtpStreamReceiver. The reason that FlexFEC reception is
not implemented here is that FlexFEC will in the future be able to do
multistream protection, so then the packets _should_ go back to Call.

Given that the RtpStreamReceiver already has information about the config (*)
for the VideoReceiveStream it's owned by, you could add a
RtpStreamReceiver::ParseRtpPacket method that would be called by the
ParseAndHandleEncapsulatedHeader method. The ParseRtpPacket method would then
replace the usage of |rtp_header_parser_|. Then I believe the recovered packets
(be it RTX, ULPFEC, FlexFEC, etc.) could go through the OnRtpPacket method,
after your changes below.

(*) Alternatively, you could pass down the new RtpReceiveConfig struct?

[nisse-webrtc, 2017/02/15 13:59:15]

This is tricky. Parsing extensions logically belongs to the stream. But then
Call (or the generic RTP receive code) needs the extensions to feed information
to the congestion controller.

One alternative might be to let call to only parse out the ssrc, invoke
OnRtpPacket with a non-const RtpPacketReceived with unparsed extension headers,
and let OnRtpPacket return with extensions parsed. We'd then have to add back
the return value, to indicate that parsing failed.

I'll have to think a bit more about this. But we really should parse the
extension headers at a single place.

                                           size_t rtp_packet_length) {
   RTPHeader header;
@@ -318,6 +321,8 @@ void RtpStreamReceiver::OnIncomingSSRCChanged(const uint32_t ssrc) {
   rtp_rtcp_->SetRemoteSSRC(ssrc);
 }
 
+// This method handles both regular Rtp packets and packets recovered

[brandtr, 2017/02/15 12:20:50]

Nits: "RTP packets". "FlexFEC".

[nisse-webrtc, 2017/02/15 13:59:15]

Done.

+// via Flexfec.
 void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) {
   {
     rtc::CritScope lock(&receive_cs_);
@@ -325,29 +330,30 @@ void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) {
       return;
     }
   }
+  if (!packet.recovered()) {
+    int64_t now_ms = clock_->TimeInMilliseconds();
 
-  int64_t now_ms = clock_->TimeInMilliseconds();
-
-  {
-    // Periodically log the RTP header of incoming packets.
-    rtc::CritScope lock(&receive_cs_);
-    if (now_ms - last_packet_log_ms_ > kPacketLogIntervalMs) {
-      std::stringstream ss;
-      ss << "Packet received on SSRC: " << packet.Ssrc()
-         << " with payload type: " << static_cast<int>(packet.PayloadType())
-         << ", timestamp: " << packet.Timestamp()
-         << ", sequence number: " << packet.SequenceNumber()
-         << ", arrival time: " << packet.arrival_time_ms();
-      int32_t time_offset;
-      if (packet.GetExtension<TransmissionOffset>(&time_offset)) {
-        ss << ", toffset: " << time_offset;
-      }
-      uint32_t send_time;
-      if (packet.GetExtension<AbsoluteSendTime>(&send_time)) {
-        ss << ", abs send time: " << send_time;
+    {
+      // Periodically log the RTP header of incoming packets.
+      rtc::CritScope lock(&receive_cs_);
+      if (now_ms - last_packet_log_ms_ > kPacketLogIntervalMs) {
+        std::stringstream ss;
+        ss << "Packet received on SSRC: " << packet.Ssrc()
+           << " with payload type: " << static_cast<int>(packet.PayloadType())
+           << ", timestamp: " << packet.Timestamp()
+           << ", sequence number: " << packet.SequenceNumber()
+           << ", arrival time: " << packet.arrival_time_ms();
+        int32_t time_offset;
+        if (packet.GetExtension<TransmissionOffset>(&time_offset)) {
+          ss << ", toffset: " << time_offset;
+        }
+        uint32_t send_time;
+        if (packet.GetExtension<AbsoluteSendTime>(&send_time)) {
+          ss << ", abs send time: " << send_time;
+        }
+        LOG(LS_INFO) << ss.str();
+        last_packet_log_ms_ = now_ms;
       }
-      LOG(LS_INFO) << ss.str();
-      last_packet_log_ms_ = now_ms;
     }
   }
 
@@ -359,13 +365,19 @@ void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) {
   header.payload_type_frequency = kVideoPayloadTypeFrequency;
 
   bool in_order = IsPacketInOrder(header);
-  rtp_payload_registry_.SetIncomingPayloadType(header);
+  if (!packet.recovered()) {

[brandtr, 2017/02/15 12:20:50]

Could you reorder the lines in this method, so that this if clause is merged
with the one above?

Alternatively: call OnRecoveredPacket here, since the functionality seems the
same. Then that method should probably be renamed.

[nisse-webrtc, 2017/02/15 13:59:15]

I'm not sure how order matters. And I'd prefer to not move the packet.GetHeader
earlier. Let's discuss.
 
Not easy, OnRecoveredPacket has other callers which wants it to parse the
packet.

So I think we'll either have to live with two recovered-packet paths for now, or
postpone this change until header parsing has settled more.

+    // TODO(nisse): Why isn't this done for recovered packets?
+    rtp_payload_registry_.SetIncomingPayloadType(header);
+  }
   ReceivePacket(packet.data(), packet.size(), header, in_order);
   // Update receive statistics after ReceivePacket.
   // Receive statistics will be reset if the payload type changes (make sure
   // that the first packet is included in the stats).
-  rtp_receive_statistics_->IncomingPacket(
-      header, packet.size(), IsPacketRetransmitted(header, in_order));
+  if (!packet.recovered()) {
+    // TODO(nisse): Should we pass a recovered flag to stats?

[brandtr, 2017/02/15 12:20:50]

That would be one way of implementing bugs.webrtc.org/6339.

[nisse-webrtc, 2017/02/15 13:59:15]

I'm adding a TODO and bug reference.

+    rtp_receive_statistics_->IncomingPacket(
+        header, packet.size(), IsPacketRetransmitted(header, in_order));
+  }
 }
 
 int32_t RtpStreamReceiver::RequestKeyFrame() {