MSE: Prefer video keyframeness analysis over MP4 only if MseBufferByPts

To prevent potentially regressing legacy buffering-by-DTS MSE apps that
may deliberately provide incorrect video keyframeness metadata in their MP4,
this change preserves trust in that metadata when MseBufferByPts is not
enabled. If MseBufferByPts is enabled, this change preserves the recently
added behavior where video frame analysis results' keyframeness (if
implemented and determined) is used instead of the MP4's metadata.

BUG=892365
R=sandersd@chromium.org
TEST=MP4StreamParserTest.*AVC*Mismatches*, also manually confirmed
SPS/PPS injection/lack of injection matches the expected sequence of
Parsed{Non}Keyframe in those tests.

Change-Id: If756e0ee882ec17c0d13d7b400aa43626cedaca9
Reviewed-on: https://chromium-review.googlesource.com/c/1263426
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596954}

media/formats/mp4/avc.cc

@@ -8,8 +8,10 @@
 #include <memory>
 #include <utility>
 
+#include "base/feature_list.h"
 #include "base/logging.h"
 #include "media/base/decrypt_config.h"
+#include "media/base/media_switches.h"
 #include "media/formats/mp4/box_definitions.h"
 #include "media/formats/mp4/box_reader.h"
 #include "media/video/h264_parser.h"
@@ -345,12 +347,15 @@ bool AVCBitstreamConverter::ConvertAndAnalyzeFrame(
                                    subsamples));
 
   // |is_keyframe| may be incorrect. Analyze the frame to see if it is a
-  // keyframe. |is_keyframe| will be used if the analysis is inconclusive.
+  // keyframe. |is_keyframe| will be used if the analysis is inconclusive or if
+  // not kMseBufferByPts.
   // Also, provide the analysis result to the caller via out parameter
   // |analysis_result|.
   *analysis_result = Analyze(frame_buf, subsamples);
 
-  if (analysis_result->is_keyframe.value_or(is_keyframe)) {
+  if (base::FeatureList::IsEnabled(kMseBufferByPts)
+          ? analysis_result->is_keyframe.value_or(is_keyframe)
+          : is_keyframe) {
     // If this is a keyframe, we (re-)inject SPS and PPS headers at the start of
     // a frame. If subsample info is present, we also update the clear byte
     // count for that first subsample.

media/formats/mp4/mp4_stream_parser.cc

@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/callback_helpers.h"
+#include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/numerics/math_constants.h"
 #include "base/strings/string_number_conversions.h"
@@ -849,7 +850,11 @@ ParseResult MP4StreamParser::EnqueueSample(BufferQueueMap* buffers) {
         // they mismatch. If other out-of-order codecs in mp4 (e.g. HEVC, DV)
         // implement keyframe analysis in their frame_bitstream_converter, we'll
         // similarly trust that analysis instead of the mp4.
-        is_keyframe = analysis.is_keyframe.value();
+        // We'll only use the analysis to override the MP4 keyframeness if
+        // |media::kMseBufferByPts| is enabled.
+        if (base::FeatureList::IsEnabled(kMseBufferByPts)) {
+          is_keyframe = analysis.is_keyframe.value();
+        }
       }
     }
   }

media/formats/mp4/mp4_stream_parser_unittest.cc

@@ -323,13 +323,37 @@ TEST_F(MP4StreamParserTest, AVC_KeyAndNonKeyframeness_Match_Container) {
   ParseMP4File("bear-640x360-v-2frames_frag.mp4", 512);
 }
 
-TEST_F(MP4StreamParserTest, AVC_Keyframeness_Mismatches_Container) {
+TEST_F(MP4StreamParserTest, LegacyByDts_AVC_Keyframeness_Mismatches_Container) {
   // The first AVC video frame's keyframe-ness metadata matches the MP4:
   // Frame 0: AVC IDR, trun.first_sample_flags: NOT sync sample, DEPENDS on
   //          others.
   // Frame 1: AVC Non-IDR, tfhd.default_sample_flags: not sync sample, depends
   //          on others.
   InSequence s;  // The EXPECT* sequence matters for this test.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(kMseBufferByPts);
+  auto params = GetDefaultInitParametersExpectations();
+  params.detected_audio_track_count = 0;
+  InitializeParserWithInitParametersExpectations(params);
+  verifying_keyframeness_sequence_ = true;
+  EXPECT_MEDIA_LOG(DebugLog(
+      "ISO-BMFF container metadata for video frame indicates that the frame is "
+      "not a keyframe, but the video frame contents indicate the opposite."));
+  EXPECT_CALL(*this, ParsedNonKeyframe());
+  EXPECT_CALL(*this, ParsedNonKeyframe());
+  ParseMP4File("bear-640x360-v-2frames-keyframe-is-non-sync-sample_frag.mp4",
+               512);
+}
+
+TEST_F(MP4StreamParserTest, NewByPts_AVC_Keyframeness_Mismatches_Container) {
+  // The first AVC video frame's keyframe-ness metadata matches the MP4:
+  // Frame 0: AVC IDR, trun.first_sample_flags: NOT sync sample, DEPENDS on
+  //          others.
+  // Frame 1: AVC Non-IDR, tfhd.default_sample_flags: not sync sample, depends
+  //          on others.
+  InSequence s;  // The EXPECT* sequence matters for this test.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kMseBufferByPts);
   auto params = GetDefaultInitParametersExpectations();
   params.detected_audio_track_count = 0;
   InitializeParserWithInitParametersExpectations(params);
@@ -343,13 +367,38 @@ TEST_F(MP4StreamParserTest, AVC_Keyframeness_Mismatches_Container) {
                512);
 }
 
-TEST_F(MP4StreamParserTest, AVC_NonKeyframeness_Mismatches_Container) {
+TEST_F(MP4StreamParserTest,
+       LegacyByDts_AVC_NonKeyframeness_Mismatches_Container) {
+  // The second AVC video frame's keyframe-ness metadata matches the MP4:
+  // Frame 0: AVC IDR, trun.first_sample_flags: sync sample that doesn't
+  //          depend on others.
+  // Frame 1: AVC Non-IDR, tfhd.default_sample_flags: SYNC sample, DOES NOT
+  //          depend on others.
+  InSequence s;  // The EXPECT* sequence matters for this test.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(kMseBufferByPts);
+  auto params = GetDefaultInitParametersExpectations();
+  params.detected_audio_track_count = 0;
+  InitializeParserWithInitParametersExpectations(params);
+  verifying_keyframeness_sequence_ = true;
+  EXPECT_CALL(*this, ParsedKeyframe());
+  EXPECT_MEDIA_LOG(DebugLog(
+      "ISO-BMFF container metadata for video frame indicates that the frame is "
+      "a keyframe, but the video frame contents indicate the opposite."));
+  EXPECT_CALL(*this, ParsedKeyframe());
+  ParseMP4File("bear-640x360-v-2frames-nonkeyframe-is-sync-sample_frag.mp4",
+               512);
+}
+
+TEST_F(MP4StreamParserTest, NewByPts_AVC_NonKeyframeness_Mismatches_Container) {
   // The second AVC video frame's keyframe-ness metadata matches the MP4:
   // Frame 0: AVC IDR, trun.first_sample_flags: sync sample that doesn't
   //          depend on others.
   // Frame 1: AVC Non-IDR, tfhd.default_sample_flags: SYNC sample, DOES NOT
   //          depend on others.
   InSequence s;  // The EXPECT* sequence matters for this test.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kMseBufferByPts);
   auto params = GetDefaultInitParametersExpectations();
   params.detected_audio_track_count = 0;
   InitializeParserWithInitParametersExpectations(params);