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

webrtc/video/rtp_stream_receiver.cc

 /*
  *  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.
  */
 
@@ skipping 273 matching lines @@
     RTC_DCHECK(video_receiver_);
     if (video_receiver_->IncomingPacket(payload_data, payload_size,
                                         rtp_header_with_ntp) != 0) {
       // Check this...
       return -1;
     }
   }
   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;
   if (!rtp_header_parser_->Parse(rtp_packet, rtp_packet_length, &header)) {
     return false;
   }
   header.payload_type_frequency = kVideoPayloadTypeFrequency;
   bool in_order = IsPacketInOrder(header);
   return ReceivePacket(rtp_packet, rtp_packet_length, header, in_order);
 }
 
 // TODO(pbos): Remove as soon as audio can handle a changing payload type
 // without this callback.
 int32_t RtpStreamReceiver::OnInitializeDecoder(
     const int8_t payload_type,
     const char payload_name[RTP_PAYLOAD_NAME_SIZE],
     const int frequency,
     const size_t channels,
     const uint32_t rate) {
   RTC_NOTREACHED();
   return 0;
 }
 
 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_);
     if (!receiving_) {
       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;
+    {
+      // 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;
       }
-      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;
     }
   }
 
   // TODO(nisse): Delete use of GetHeader, but needs refactoring of
   // ReceivePacket and IncomingPacket methods below.
   RTPHeader header;
   packet.GetHeader(&header);
 
   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() {
   return rtp_rtcp_->RequestKeyFrame();
 }
 
 int32_t RtpStreamReceiver::SliceLossIndicationRequest(
     const uint64_t picture_id) {
   return rtp_rtcp_->SendRTCPSliceLossIndication(
       static_cast<uint8_t>(picture_id));
@@ skipping 289 matching lines @@
     return;
 
   if (!sprop_decoder.DecodeSprop(sprop_base64_it->second.c_str()))
     return;
 
   tracker_.InsertSpsPpsNalus(sprop_decoder.sps_nalu(),
                              sprop_decoder.pps_nalu());
 }
 
 }  // namespace webrtc