From ae81975bd3621a32c1f322a2f48aa1e70d97333a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=85sa=20Persson?= <asapersson@webrtc.org>
Date: Tue, 10 Oct 2017 11:05:59 +0200
Subject: [PATCH] Make PictureIdTest more strict.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Only allow gaps in picture id for key frames.
When a VideoSendStream is destroyed, frames in the queue not yet sent are lost. The recreated stream
should start with a key frame.

Also enable PictureIdIncreasingAfterStreamCountChangeSimulcastEncoderAdapter if forced fallback is
enabled. In this case, the picture id is set in the PayloadRouter and the sequence should be
continuous.


Bug: none
Change-Id: If7987166c86d6a8edbe5e479701f7f04c49cd89c
Reviewed-on: https://webrtc-review.googlesource.com/7363
Commit-Queue: Åsa Persson <asapersson@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20216}
---
 video/picture_id_tests.cc | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/video/picture_id_tests.cc b/video/picture_id_tests.cc
index 3ebf8637c2..5896d552c6 100644
--- a/video/picture_id_tests.cc
+++ b/video/picture_id_tests.cc
@@ -110,7 +110,17 @@ class PictureIdObserver : public test::RtpRtcpObserver {
       // Picture id should not increase more than expected.
       const int picture_id_diff = ForwardDiff<uint16_t, kPictureIdWraparound>(
           last_observed_picture_id_[ssrc], picture_id);
-      EXPECT_LE(picture_id_diff - 1, max_expected_picture_id_gap_);
+
+      // For delta frames, expect continuously increasing picture id.
+      if (parsed_payload.frame_type != kVideoFrameKey) {
+        EXPECT_EQ(picture_id_diff, 1);
+      }
+      // Any frames still in queue is lost when a VideoSendStream is destroyed.
+      // The first frame after recreation should be a key frame.
+      if (picture_id_diff > 1) {
+        EXPECT_EQ(kVideoFrameKey, parsed_payload.frame_type);
+        EXPECT_LE(picture_id_diff - 1, max_expected_picture_id_gap_);
+      }
     }
     last_observed_timestamp_[ssrc] = timestamp;
     last_observed_picture_id_[ssrc] = picture_id;
@@ -247,7 +257,7 @@ void PictureIdTest::TestPictureIdContinuousAfterReconfigure(
   EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
 
   // Reconfigure VideoEncoder and test picture id increase.
-  // Expect continously increasing picture id, equivalent to no gaps.
+  // Expect continuously increasing picture id, equivalent to no gaps.
   observer.SetMaxExpectedPictureIdGap(0);
   for (int ssrc_count : ssrc_counts) {
     video_encoder_config_.number_of_streams = ssrc_count;
@@ -353,16 +363,20 @@ TEST_P(PictureIdTest,
 // When using the simulcast encoder adapter, the picture id is randomly set
 // when the ssrc count is reduced and then increased. This means that we are
 // not spec compliant in that particular case.
-TEST_P(
-    PictureIdTest,
-    DISABLED_PictureIdIncreasingAfterStreamCountChangeSimulcastEncoderAdapter) {
-  cricket::InternalEncoderFactory internal_encoder_factory;
-  SimulcastEncoderAdapter simulcast_encoder_adapter(&internal_encoder_factory);
-  // Make sure that that the picture id is not reset if the stream count goes
-  // down and then up.
-  std::vector<int> ssrc_counts = {3, 1, 3};
-  SetupEncoder(&simulcast_encoder_adapter);
-  TestPictureIdContinuousAfterReconfigure(ssrc_counts);
+TEST_P(PictureIdTest,
+       PictureIdIncreasingAfterStreamCountChangeSimulcastEncoderAdapter) {
+  // If forced fallback is enabled, the picture id is set in the PayloadRouter
+  // and the sequence should be continuous.
+  if (GetParam() == kVp8ForcedFallbackEncoderEnabled) {
+    cricket::InternalEncoderFactory internal_encoder_factory;
+    SimulcastEncoderAdapter simulcast_encoder_adapter(
+        &internal_encoder_factory);
+    // Make sure that that the picture id is not reset if the stream count goes
+    // down and then up.
+    std::vector<int> ssrc_counts = {3, 1, 3};
+    SetupEncoder(&simulcast_encoder_adapter);
+    TestPictureIdContinuousAfterReconfigure(ssrc_counts);
+  }
 }
 
 }  // namespace webrtc