Fix padding removal handler (#868)

lynckia · Apr 24, 2017 · e483800 · e483800
1 parent 9668b17
commit e483800
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 17 deletions.
diff --git a/erizo/src/erizo/WebRtcConnection.cpp b/erizo/src/erizo/WebRtcConnection.cpp
@@ -277,8 +277,8 @@ void WebRtcConnection::initializePipeline() {
   pipeline_->addFront(RtpPaddingGeneratorHandler());
   pipeline_->addFront(PliPacerHandler());
   pipeline_->addFront(BandwidthEstimationHandler());
-  pipeline_->addFront(RtcpFeedbackGenerationHandler());
   pipeline_->addFront(RtpPaddingRemovalHandler());
+  pipeline_->addFront(RtcpFeedbackGenerationHandler());
   pipeline_->addFront(RtpRetransmissionHandler());
   pipeline_->addFront(SRPacketHandler());
   pipeline_->addFront(SenderBandwidthEstimationHandler());

diff --git a/erizo/src/erizo/rtp/RtpPaddingRemovalHandler.cpp b/erizo/src/erizo/rtp/RtpPaddingRemovalHandler.cpp
@@ -22,30 +22,67 @@ void RtpPaddingRemovalHandler::read(Context *ctx, std::shared_ptr<dataPacket> pa
 
   if (!chead->isRtcp() && enabled_ && packet->type == VIDEO_PACKET) {
     uint32_t ssrc = rtp_header->getSSRC();
-    auto translator_it = translator_map_.find(ssrc);
-    std::shared_ptr<SequenceNumberTranslator> translator;
-    if (translator_it != translator_map_.end()) {
-      translator = translator_it->second;
-    } else {
-      ELOG_DEBUG("message: no Translator found creating a new one, ssrc: %u", ssrc);
-      translator = std::make_shared<SequenceNumberTranslator>();
-      translator_map_[ssrc] = translator;
-    }
+    std::shared_ptr<SequenceNumberTranslator> translator = getTranslatorForSsrc(ssrc, true);
     if (!removePaddingBytes(packet, translator)) {
       return;
     }
     uint16_t sequence_number = rtp_header->getSeqNumber();
     SequenceNumber sequence_number_info = translator->get(sequence_number, false);
 
     if (sequence_number_info.type != SequenceNumberType::Valid) {
+      ELOG_DEBUG("Invalid translation %u, ssrc: %u", sequence_number, ssrc);
       return;
     }
+    ELOG_DEBUG("Changing seq_number from %u to %u, ssrc %u", sequence_number, sequence_number_info.output,
+     ssrc);
     rtp_header->setSeqNumber(sequence_number_info.output);
   }
   ctx->fireRead(packet);
 }
 
 void RtpPaddingRemovalHandler::write(Context *ctx, std::shared_ptr<dataPacket> packet) {
+  RtcpHeader* rtcp_head = reinterpret_cast<RtcpHeader*>(packet->data);
+  if (!enabled_ || packet->type != VIDEO_PACKET || !rtcp_head->isFeedback()) {
+    ctx->fireWrite(packet);
+    return;
+  }
+  uint32_t ssrc = rtcp_head->getSourceSSRC();
+  std::shared_ptr<SequenceNumberTranslator> translator = getTranslatorForSsrc(ssrc, false);
+  if (!translator) {
+    ELOG_DEBUG("No translator for ssrc %u, %s", ssrc, connection_->toLog());
+    ctx->fireWrite(packet);
+    return;
+  }
+  RtpUtils::forEachRRBlock(packet, [this, translator, ssrc](RtcpHeader *chead) {
+    if (chead->packettype == RTCP_RTP_Feedback_PT) {
+      RtpUtils::forEachNack(chead, [this, chead, translator, ssrc](uint16_t new_seq_num, uint16_t new_plb,
+      RtcpHeader* nack_header) {
+        uint16_t initial_seq_num = new_seq_num;
+        std::vector<uint16_t> seq_nums;
+        for (int i = -1; i <= 15; i++) {
+          uint16_t seq_num = initial_seq_num + i + 1;
+          SequenceNumber input_seq_num = translator->reverse(seq_num);
+          if (input_seq_num.type == SequenceNumberType::Valid) {
+            seq_nums.push_back(input_seq_num.input);
+          } else {
+            ELOG_DEBUG("Input is not valid for %u, ssrc %u, %s", seq_num, ssrc, connection_->toLog());
+          }
+          ELOG_DEBUG("Lost packet %u, input %u, ssrc %u", seq_num, input_seq_num.input, ssrc);
+        }
+        if (seq_nums.size() > 0) {
+          uint16_t pid = seq_nums[0];
+          uint16_t blp = 0;
+          for (uint16_t index = 1; index < seq_nums.size() ; index++) {
+            uint16_t distance = seq_nums[index] - pid - 1;
+            blp |= (1 << distance);
+          }
+          nack_header->setNackPid(pid);
+          nack_header->setNackBlp(blp);
+          ELOG_DEBUG("Translated pid %u, translated blp %u, ssrc %u, %s", pid, blp, ssrc, connection_->toLog());
+        }
+      });
+    }
+  });
   ctx->fireWrite(packet);
 }
 
@@ -58,22 +95,39 @@ bool RtpPaddingRemovalHandler::removePaddingBytes(std::shared_ptr<dataPacket> pa
   if (padding_length + header_length == packet->length) {
     uint16_t sequence_number = rtp_header->getSeqNumber();
     translator->get(sequence_number, true);
+    ELOG_DEBUG("Dropping packet %u, %s", sequence_number, connection_->toLog());
     return false;
   }
   packet->length -= padding_length;
   rtp_header->padding = 0;
   return true;
 }
 
+std::shared_ptr<SequenceNumberTranslator> RtpPaddingRemovalHandler::getTranslatorForSsrc(uint32_t ssrc,
+  bool should_create) {
+    auto translator_it = translator_map_.find(ssrc);
+    std::shared_ptr<SequenceNumberTranslator> translator;
+    if (translator_it != translator_map_.end()) {
+      ELOG_DEBUG("Found Translator for %u, %s", ssrc, connection_->toLog());
+      translator = translator_it->second;
+    } else if (should_create) {
+      ELOG_DEBUG("message: no Translator found creating a new one, ssrc: %u, %s", ssrc,
+      connection_->toLog());
+      translator = std::make_shared<SequenceNumberTranslator>();
+      translator_map_[ssrc] = translator;
+    }
+    return translator;
+  }
+
 void RtpPaddingRemovalHandler::notifyUpdate() {
   auto pipeline = getContext()->getPipelineShared();
   if (!pipeline) {
     return;
   }
-
   if (initialized_) {
     return;
   }
+  connection_ = pipeline->getService<WebRtcConnection>().get();
   initialized_ = true;
 }
 }  // namespace erizo
diff --git a/erizo/src/erizo/rtp/RtpPaddingRemovalHandler.h b/erizo/src/erizo/rtp/RtpPaddingRemovalHandler.h
@@ -1,3 +1,4 @@
+
 #ifndef ERIZO_SRC_ERIZO_RTP_RTPPADDINGREMOVALHANDLER_H_
 #define ERIZO_SRC_ERIZO_RTP_RTPPADDINGREMOVALHANDLER_H_
 
@@ -6,6 +7,7 @@
 #include "./logger.h"
 #include "pipeline/Handler.h"
 #include "rtp/SequenceNumberTranslator.h"
+#include "WebRtcConnection.h"
 
 namespace erizo {
 
@@ -32,11 +34,14 @@ class RtpPaddingRemovalHandler: public Handler, public std::enable_shared_from_t
  private:
   bool removePaddingBytes(std::shared_ptr<dataPacket> packet,
       std::shared_ptr<SequenceNumberTranslator> translator);
+  std::shared_ptr<SequenceNumberTranslator> getTranslatorForSsrc(uint32_t ssrc,
+    bool should_create);
 
  private:
   bool enabled_;
   bool initialized_;
   std::map<uint32_t, std::shared_ptr<SequenceNumberTranslator>> translator_map_;
+  WebRtcConnection* connection_;
 };
 }  // namespace erizo
 

diff --git a/erizo/src/erizo/rtp/RtpRetransmissionHandler.cpp b/erizo/src/erizo/rtp/RtpRetransmissionHandler.cpp
@@ -74,7 +74,8 @@ void RtpRetransmissionHandler::read(Context *ctx, std::shared_ptr<dataPacket> pa
     if (chead->packettype == RTCP_RTP_Feedback_PT) {
       contains_nack = true;
 
-      RtpUtils::forEachNack(chead, [this, chead, &is_fully_recovered](uint16_t new_seq_num, uint16_t new_plb) {
+      RtpUtils::forEachNack(chead, [this, chead, &is_fully_recovered](uint16_t new_seq_num,
+         uint16_t new_plb, RtcpHeader* nack_head) {
         uint16_t initial_seq_num = new_seq_num;
         uint16_t plb = new_plb;
 

diff --git a/erizo/src/erizo/rtp/RtpUtils.cpp b/erizo/src/erizo/rtp/RtpUtils.cpp
@@ -21,7 +21,7 @@ void RtpUtils::updateREMB(RtcpHeader *chead, uint bitrate) {
   }
 }
 
-void RtpUtils::forEachNack(RtcpHeader *chead, std::function<void(uint16_t, uint16_t)> f) {
+void RtpUtils::forEachNack(RtcpHeader *chead, std::function<void(uint16_t, uint16_t, RtcpHeader*)> f) {
   if (chead->packettype == RTCP_RTP_Feedback_PT) {
     int length = (chead->getLength() + 1)*4;
     int current_position = kNackCommonHeaderLengthBytes;
@@ -31,7 +31,7 @@ void RtpUtils::forEachNack(RtcpHeader *chead, std::function<void(uint16_t, uint1
       aux_chead = reinterpret_cast<RtcpHeader*>(aux_pointer);
       uint16_t initial_seq_num = aux_chead->getNackPid();
       uint16_t plb = aux_chead->getNackBlp();
-      f(initial_seq_num, plb);
+      f(initial_seq_num, plb, aux_chead);
       current_position += 4;
       aux_pointer += 4;
     }

diff --git a/erizo/src/erizo/rtp/RtpUtils.h b/erizo/src/erizo/rtp/RtpUtils.h
@@ -23,7 +23,7 @@ class RtpUtils {
 
   static bool isFIR(std::shared_ptr<dataPacket> packet);
 
-  static void forEachNack(RtcpHeader *chead, std::function<void(uint16_t, uint16_t)> f);
+  static void forEachNack(RtcpHeader *chead, std::function<void(uint16_t, uint16_t, RtcpHeader*)> f);
 
   static std::shared_ptr<dataPacket> createPLI(uint32_t source_ssrc, uint32_t sink_ssrc);
 

diff --git a/erizo/src/test/log4cxx.properties b/erizo/src/test/log4cxx.properties
@@ -53,6 +53,7 @@ log4j.logger.rtp.RtpAudioMuteHandler=ERROR
 log4j.logger.rtp.RtpSink=ERROR
 log4j.logger.rtp.RtpSource=ERROR
 log4j.logger.rtp.RtcpFeedbackGenerationHandler=ERROR
+log4j.logger.rtp.RtpPaddingRemovalHandler=ERROR
 log4j.logger.rtp.RtcpRrGenerator=ERROR
 log4j.logger.rtp.RtcpNackGenerator=ERROR
 log4j.logger.rtp.SRPacketHandler=ERROR

diff --git a/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp b/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp
@@ -58,7 +58,8 @@ class RtcpNackGeneratorTest :public ::testing::Test {
       total_length += rtcp_length;
 
       if (chead->packettype == RTCP_RTP_Feedback_PT) {
-        erizo::RtpUtils::forEachNack(chead, [chead, lost_seq_num, &found_nack](uint16_t seq_num, uint16_t plb) {
+        erizo::RtpUtils::forEachNack(chead, [chead, lost_seq_num, &found_nack](uint16_t seq_num,
+           uint16_t plb, RtcpHeader* nack_head) {
           uint16_t initial_seq_num = seq_num;
           if (initial_seq_num == lost_seq_num) {
              found_nack = true;
@@ -201,4 +202,3 @@ TEST_F(RtcpNackGeneratorTest, shouldNotRetransmitNacksMoreThanTwice) {
   receiver_report = generateRrWithNack();
   EXPECT_FALSE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1));
 }
-