Fix FrameConfigs used for VP8 with four temporal layers.

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

 /* Copyright (c) 2013 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.
 */
 
 #include "webrtc/modules/video_coding/codecs/vp8/default_temporal_layers.h"
@@ skipping 77 matching lines @@
   RTC_NOTREACHED();
   return {0};
 }
 
 std::vector<bool> GetTemporalLayerSync(size_t num_layers) {
   switch (num_layers) {
     case 1:
       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) {
   // For indexing in the patterns described below (which temporal layers they
   // belong to), see the diagram above.
   // Layer sync is done similarly for all patterns (except single stream) and
   // happens every 8 frames:
   // TL1 layer syncs by periodically by only referencing TL0 ('last'), but still
   // updating 'golden', so it can be used as a reference by future TL1 frames.
   // TL2 layer syncs just before TL1 by only depending on TL0 (and not depending
   // on TL1's buffer before TL1 has layer synced).
   // TODO(pbos): Consider cyclically updating 'arf' (and 'golden' for 1TL) for
   // the base layer in 1-3TL instead of 'last' periodically on long intervals,
   // so that if scene changes occur (user walks between rooms or rotates webcam)
   // the 'arf' (or 'golden' respectively) is not stuck on a no-longer relevant
   // keyframe.
   switch (num_layers) {
     case 1:
       // All frames reference all buffers and the 'last' buffer is updated.
       return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kReference,
                                           TemporalLayers::kReference)};
     case 2:
       // 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'.
       return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
-                                          TemporalLayers::kUpdate,
+                                          TemporalLayers::kNone,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kUpdate,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kNone,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kNone,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
                                           TemporalLayers::kNone,
                                           TemporalLayers::kReference),
               TemporalLayers::FrameConfig(
                   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'.
       // TL2 references 'last' and 'golden', and references and updates 'arf'.
       // TL3 references all buffers but update none of them.
       return {TemporalLayers::FrameConfig(TemporalLayers::kReferenceAndUpdate,
                                           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,
                                           TemporalLayers::kNone),
               TemporalLayers::FrameConfig(
                   TemporalLayers::kReference, TemporalLayers::kReference,
                   TemporalLayers::kReference, TemporalLayers::kFreezeEntropy),
               TemporalLayers::FrameConfig(TemporalLayers::kReference,
                                           TemporalLayers::kReference,
                                           TemporalLayers::kReferenceAndUpdate),
@@ skipping 170 matching lines @@
   if (listener_ && !ExcludeOnTemporalLayersCreated(temporal_layers))
     listener_->OnTemporalLayersCreated(simulcast_id, tl);
   return tl;
 }
 
 void TemporalLayersFactory::SetListener(TemporalLayersListener* listener) {
   listener_ = listener;
 }
 
 }  // namespace webrtc