Fix FrameConfigs used for VP8 with four temporal layers.

webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc

Index: webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc
diff --git a/webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc b/webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc
index 979c64e24324bca7493001b16f37b94ed14b18d1..7e7effbd36f4fcf39f7e24a7e6179c4a4688625e 100644
--- a/webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc
+++ b/webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.cc
@@ -95,17 +95,21 @@ std::vector<bool> GetTemporalLayerSync(size_t num_layers) {
       return {false};
     case 2:
       return {false, true, false, false, false, false, false, false};
-    case 3:
-      return {false, true, true, false, false, false, false, false};
+    case 3: {
+      std::vector<bool> sync_pattern = {false, true,  true,  false,
+                                        false, false, false, false};
+      if (field_trial::IsEnabled("WebRTC-AltTemporalLayers"))

[pbos1, 2017/08/31 19:54:38]

Actually if this is not supposed to be a subset below pattern I prefer if you
use the old path of just returning {false, true, true, false}.

[pbos1, 2017/08/31 19:54:38]

Also do you think we could generate GetTemporalLayerSync from
std::vector<TemporalLayers::FrameConfig> from GetTemporalPattern() instead of
having this pattern have to match below?

[sprang_webrtc, 2017/09/01 14:32:56]

Potentially, but it's not super straight forward.
I've added a generalized test. I'm thinking that in a follow-up CL we can modify
that to generate the sync bit instead, or even refactor the test into a class
the can validate the frame configs actually going out of the encoder in say
debug mode. That way we could have found (in eg full stack tests) the bug that
sometimes caused incorrect sync bit for screenshare. wdyt?

[sprang_webrtc, 2017/09/01 14:32:56]

Done.

+        sync_pattern.resize(4);
+      return sync_pattern;
+    }
     case 4:
-      return {false, true, true,  true, true,  true, false, true,
-              false, true, false, true, false, true, false, true};
+      return {false, true,  true,  false, true,  false, false, false,
+              false, false, false, false, false, false, false, false};
     default:
-      RTC_NOTREACHED();
       break;
   }
-  RTC_NOTREACHED();
-  return {false};
+  RTC_NOTREACHED() << num_layers;
+  return {};
 }
 
 std::vector<TemporalLayers::FrameConfig> GetTemporalPattern(size_t num_layers) {
@@ -134,7 +138,7 @@ std::vector<TemporalLayers::FrameConfig> GetTemporalPattern(size_t num_layers) {
       // TL0 also references and updates the 'last' buffer.
       // TL1 also references 'last' and references and updates 'golden'.
       return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
-                                          TemporalLayers::kUpdate,
+                                          TemporalLayers::kNone,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kUpdate,
@@ -158,35 +162,56 @@ std::vector<TemporalLayers::FrameConfig> GetTemporalPattern(size_t num_layers) {
                   TemporalLayers::kReference, TemporalLayers::kReference,
                   TemporalLayers::kReference, TemporalLayers::kFreezeEntropy)};
     case 3:
-      // All layers can reference but not update the 'alt' buffer, this means
-      // that the 'alt' buffer reference is effectively the last keyframe.
-      // TL0 also references and updates the 'last' buffer.
-      // TL1 also references 'last' and references and updates 'golden'.
-      // TL2 references both 'last' and 'golden' but updates no buffer.
-      return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
-                                          TemporalLayers::kUpdate,
-                                          TemporalLayers::kReference),
-              TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kNone,
-                  TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
-              TemporalLayers::FrameConfig(TemporalLayers::kReference,
-                                          TemporalLayers::kUpdate,
-                                          TemporalLayers::kReference),
-              TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kReference,
-                  TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
-              TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
-                                          TemporalLayers::kNone,
-                                          TemporalLayers::kReference),
-              TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kReference,
-                  TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
-              TemporalLayers::FrameConfig(TemporalLayers::kReference,
-                                          TemporalLayers::kReferenceAndUpdate,
-                                          TemporalLayers::kReference),
-              TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kReference,
-                  TemporalLayers::kReference, TemporalLayers::kFreezeEntropy)};
+      if (field_trial::IsEnabled("WebRTC-AltTemporalLayers")) {

[pbos1, 2017/08/31 19:54:38]

Can you put down here why this is meaningfully different from the other longer
pattern? What's this trying to investigate?

Also, can you make the field trial name more meaningful? Not sure if that's
possible, but maybe something like "WebRTC-UseShortTL3Pattern" is 
more meaningful?

FWIW it would've been meaningful to keep the pattern bug fixes and the
experiment separate, suggest doing so in the future. (The bug fixes would've
been submittable straight away.)

[sprang_webrtc, 2017/09/01 14:32:56]

Done.
Done.
True, that's what I normally do. This CL started out investigating the
experiment, I just happened to find the bugs along the way and was lazy...

+        // TL0 references and updates the 'last' buffer.
+        // TL1  references 'last' and references and updates 'golden'.
+        // TL2 references both 'last' & 'golden' and references and updates
+        // 'arf'.
+        return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
+                                            TemporalLayers::kNone,
+                                            TemporalLayers::kNone),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kNone,
+                                            TemporalLayers::kUpdate),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kUpdate,
+                                            TemporalLayers::kNone),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kReference,
+                                            TemporalLayers::kReference,
+                                            TemporalLayers::kFreezeEntropy)};
+      } else {
+        // All layers can reference but not update the 'alt' buffer, this means
+        // that the 'alt' buffer reference is effectively the last keyframe.
+        // TL0 also references and updates the 'last' buffer.
+        // TL1 also references 'last' and references and updates 'golden'.
+        // TL2 references both 'last' and 'golden' but updates no buffer.
+        return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
+                                            TemporalLayers::kNone,
+                                            TemporalLayers::kReference),
+                TemporalLayers::FrameConfig(
+                    TemporalLayers::kReference, TemporalLayers::kNone,
+                    TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kUpdate,
+                                            TemporalLayers::kReference),
+                TemporalLayers::FrameConfig(
+                    TemporalLayers::kReference, TemporalLayers::kReference,
+                    TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
+                TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
+                                            TemporalLayers::kNone,
+                                            TemporalLayers::kReference),
+                TemporalLayers::FrameConfig(
+                    TemporalLayers::kReference, TemporalLayers::kReference,
+                    TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kReferenceAndUpdate,
+                                            TemporalLayers::kReference),
+                TemporalLayers::FrameConfig(TemporalLayers::kReference,
+                                            TemporalLayers::kReference,
+                                            TemporalLayers::kReference,
+                                            TemporalLayers::kFreezeEntropy)};
+      }
     case 4:
       // TL0 references and updates only the 'last' buffer.
       // TL1 references 'last' and updates and references 'golden'.
@@ -196,13 +221,13 @@ std::vector<TemporalLayers::FrameConfig> GetTemporalPattern(size_t num_layers) {
                                           TemporalLayers::kNone,
                                           TemporalLayers::kNone),
               TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kReference,
-                  TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
+                  TemporalLayers::kReference, TemporalLayers::kNone,
+                  TemporalLayers::kNone, TemporalLayers::kFreezeEntropy),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kNone,
                                           TemporalLayers::kUpdate),
               TemporalLayers::FrameConfig(
-                  TemporalLayers::kReference, TemporalLayers::kReference,
+                  TemporalLayers::kReference, TemporalLayers::kNone,
                   TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kUpdate,