Reland "MSE: refactor mp4 AnnexB validation to also report keyframe-ness"

This is a reland of 568cc6a9, which had
been reverted in 8fba4f3d (bug 867520)
due to failing link on windows builders with proprietary_codecs in their
build configuration (bug 867536).

Versus the original change, this one includes an additional MEDIA_EXPORT
on the inner type "media::mp4::BitstreamConverter::AnalysisResult".

Original change's description:
> MSE: refactor mp4 AnnexB validation to also report keyframe-ness
>
> In preparation for logging when there appears to be a mismatch between
> MSE MP4 keyframe metadata and the encoded video bitstream's keyframe
> metadata, this change incorporates basic analysis of the latter as part
> of the existing Annex-B bitstream validation for MSE MP4 video.
>
> Note that mp4::BitstreamConverter::AnalysisResult contains
> base::Optional<bool> for each of conformance and keyframe-ness fields.
> If a field doesn't have a value, that portion of the result was not
> analyzed.
>
> Note that keyframe analysis is implemented only for AVC currently, but
> even that is skipped if the frame is detected as non-conformant before
> enough indications of whether or not it is a keyframe are detected.
> Neither AVC-DV nor HEVC AnnexB analyses do any actual keyframe analysis,
> since such was either skipped or not yet implemented previously,
> respectively.
>
> A subsequent CL will use the newly reported MSE MP4 video bitstream
> converter's keyframe analysis results, if that analysis was done, in
> reporting to chrome://media-internals when the bitstream keyframe-ness
> mismatches that of the mp4 container for a coded frame.
>
> BUG=860420,584384
>
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: I169c7774070ad232c86658bcd7803160323993ad
> Reviewed-on: https://chromium-review.googlesource.com/1144456
> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
> Reviewed-by: Sergey Volk <servolk@chromium.org>
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577717}

Bug: 860420, 584384
Change-Id: I8bbb3abc2079b53a14db8babbf04d5bad9e94767
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1150247Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578116}

media/filters/ffmpeg_demuxer_unittest.cc

@@ -32,6 +32,7 @@
 #include "media/filters/ffmpeg_demuxer.h"
 #include "media/filters/file_data_source.h"
 #include "media/formats/mp4/avc.h"
+#include "media/formats/mp4/bitstream_converter.h"
 #include "media/media_buildflags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -1199,8 +1200,8 @@ static void ValidateAnnexB(DemuxerStream* stream,
     subsamples = buffer->decrypt_config()->subsamples();
 
   bool is_valid =
-      mp4::AVC::IsValidAnnexB(buffer->data(), buffer->data_size(),
-                              subsamples);
+      mp4::AVC::AnalyzeAnnexB(buffer->data(), buffer->data_size(), subsamples)
+          .is_conformant.value_or(false);
   EXPECT_TRUE(is_valid);
 
   if (!is_valid) {

media/formats/mp4/avc.cc

@@ -171,15 +171,21 @@ bool AVC::ConvertConfigToAnnexB(const AVCDecoderConfigurationRecord& avc_config,
   return true;
 }
 
-// Verifies AnnexB NALU order according to ISO/IEC 14496-10 Section 7.4.1.2.3
-bool AVC::IsValidAnnexB(const uint8_t* buffer,
-                        size_t size,
-                        const std::vector<SubsampleEntry>& subsamples) {
+// static
+BitstreamConverter::AnalysisResult AVC::AnalyzeAnnexB(
+    const uint8_t* buffer,
+    size_t size,
+    const std::vector<SubsampleEntry>& subsamples) {
   DVLOG(3) << __func__;
   DCHECK(buffer);
 
-  if (size == 0)
-    return true;
+  BitstreamConverter::AnalysisResult result;
+  result.is_conformant = false;  // Will change if needed before return.
+
+  if (size == 0) {
+    result.is_conformant = true;
+    return result;
+  }
 
   H264Parser parser;
   parser.SetEncryptedStream(buffer, size, subsamples);
@@ -205,7 +211,7 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
           case H264NALU::kAUD:
             if (order_state > kAUDAllowed) {
               DVLOG(1) << "Unexpected AUD in order_state " << order_state;
-              return false;
+              return result;
             }
             order_state = kBeforeFirstVCL;
             break;
@@ -221,7 +227,7 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
             if (order_state > kBeforeFirstVCL) {
               DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
                        << " in order_state " << order_state;
-              return false;
+              return result;
             }
             order_state = kBeforeFirstVCL;
             break;
@@ -229,7 +235,7 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
           case H264NALU::kSPSExt:
             if (last_nalu_type != H264NALU::kSPS) {
               DVLOG(1) << "SPS extension does not follow an SPS.";
-              return false;
+              return result;
             }
             break;
 
@@ -240,22 +246,26 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
           case H264NALU::kIDRSlice:
             if (order_state > kAfterFirstVCL) {
               DVLOG(1) << "Unexpected VCL in order_state " << order_state;
-              return false;
+              return result;
             }
+
+            if (!result.is_keyframe.has_value())
+              result.is_keyframe = nalu.nal_unit_type == H264NALU::kIDRSlice;
+
             order_state = kAfterFirstVCL;
             break;
 
           case H264NALU::kCodedSliceAux:
             if (order_state != kAfterFirstVCL) {
               DVLOG(1) << "Unexpected extension in order_state " << order_state;
-              return false;
+              return result;
             }
             break;
 
           case H264NALU::kEOSeq:
             if (order_state != kAfterFirstVCL) {
               DVLOG(1) << "Unexpected EOSeq in order_state " << order_state;
-              return false;
+              return result;
             }
             order_state = kEOStreamAllowed;
             break;
@@ -263,7 +273,7 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
           case H264NALU::kEOStream:
             if (order_state < kAfterFirstVCL) {
               DVLOG(1) << "Unexpected EOStream in order_state " << order_state;
-              return false;
+              return result;
             }
             order_state = kNoMoreDataAllowed;
             break;
@@ -274,7 +284,7 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
                   order_state < kEOStreamAllowed)) {
               DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
                        << " in order_state " << order_state;
-              return false;
+              return result;
             }
             break;
 
@@ -284,25 +294,30 @@ bool AVC::IsValidAnnexB(const uint8_t* buffer,
                 order_state != kAfterFirstVCL) {
               DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
                        << " in order_state " << order_state;
-              return false;
+              return result;
             }
         }
         last_nalu_type = nalu.nal_unit_type;
         break;
 
       case H264Parser::kInvalidStream:
-        return false;
+        return result;
 
       case H264Parser::kUnsupportedStream:
         NOTREACHED() << "AdvanceToNextNALU() returned kUnsupportedStream!";
-        return false;
+        return result;
 
       case H264Parser::kEOStream:
         done = true;
     }
   }
 
-  return order_state >= kAfterFirstVCL;
+  if (order_state < kAfterFirstVCL)
+    return result;
+
+  result.is_conformant = true;
+  DCHECK(result.is_keyframe.has_value());
+  return result;
 }
 
 AVCBitstreamConverter::AVCBitstreamConverter(
@@ -338,14 +353,16 @@ bool AVCBitstreamConverter::ConvertFrame(
   return true;
 }
 
-bool AVCBitstreamConverter::IsValid(
+BitstreamConverter::AnalysisResult AVCBitstreamConverter::Analyze(
     std::vector<uint8_t>* frame_buf,
     std::vector<SubsampleEntry>* subsamples) const {
 #if BUILDFLAG(ENABLE_DOLBY_VISION_DEMUXING)
-  if (disable_validation_)
-    return true;
+  if (disable_validation_) {
+    BitstreamConverter::AnalysisResult result;
+    return result;
+  }
 #endif  // BUILDFLAG(ENABLE_DOLBY_VISION_DEMUXING)
-  return AVC::IsValidAnnexB(frame_buf->data(), frame_buf->size(), *subsamples);
+  return AVC::AnalyzeAnnexB(frame_buf->data(), frame_buf->size(), *subsamples);
 }
 
 }  // namespace mp4

media/formats/mp4/avc.h

@@ -44,14 +44,17 @@ class MEDIA_EXPORT AVC {
       const AVCDecoderConfigurationRecord& avc_config,
       std::vector<uint8_t>* buffer);
 
-  // Verifies that the contents of |buffer| conform to
-  // Section 7.4.1.2.3 of ISO/IEC 14496-10.
+  // Analyzes the contents of |buffer| for conformance to Section 7.4.1.2.3 of
+  // ISO/IEC 14496-10. Also analyzes |buffer| and reports if it looks like a
+  // keyframe, if such can be determined. Determination of keyframe-ness is done
+  // only if |buffer| is conformant or if lack of conformance is detected after
+  // detecting keyframe-ness.
   // |subsamples| contains the information about what parts of the buffer are
   // encrypted and which parts are clear.
-  // Returns true if |buffer| contains conformant Annex B data
-  static bool IsValidAnnexB(const uint8_t* buffer,
-                            size_t size,
-                            const std::vector<SubsampleEntry>& subsamples);
+  static BitstreamConverter::AnalysisResult AnalyzeAnnexB(
+      const uint8_t* buffer,
+      size_t size,
+      const std::vector<SubsampleEntry>& subsamples);
 
   // Given a |buffer| and |subsamples| information and |pts| pointer into the
   // |buffer| finds the index of the subsample |ptr| is pointing into.
@@ -81,8 +84,9 @@ class AVCBitstreamConverter : public BitstreamConverter {
                     bool is_keyframe,
                     std::vector<SubsampleEntry>* subsamples) const override;
 
-  bool IsValid(std::vector<uint8_t>* frame_buf,
-               std::vector<SubsampleEntry>* subsamples) const override;
+  AnalysisResult Analyze(
+      std::vector<uint8_t>* frame_buf,
+      std::vector<SubsampleEntry>* subsamples) const override;
 
  private:
   ~AVCBitstreamConverter() override;

media/formats/mp4/avc_unittest.cc

@@ -6,12 +6,16 @@
 #include <stdint.h>
 #include <string.h>
 
+#include <ostream>
+
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "media/base/decrypt_config.h"
 #include "media/base/stream_parser_buffer.h"
 #include "media/formats/mp4/avc.h"
+#include "media/formats/mp4/bitstream_converter.h"
 #include "media/formats/mp4/box_definitions.h"
 #include "media/video/h264_parser.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -36,6 +40,15 @@ static H264NALU::Type StringToNALUType(const std::string& name) {
   if (name == "I")
     return H264NALU::kIDRSlice;
 
+  if (name == "SDA")
+    return H264NALU::kSliceDataA;
+
+  if (name == "SDB")
+    return H264NALU::kSliceDataB;
+
+  if (name == "SDC")
+    return H264NALU::kSliceDataC;
+
   if (name == "SEI")
     return H264NALU::kSEIMessage;
 
@@ -132,9 +145,9 @@ static void WriteStartCodeAndNALUType(std::vector<uint8_t>* buffer,
 // The output buffer will contain a valid-looking Annex B (it's valid-looking in
 // the sense that it has start codes and correct NALU types, but the actual NALU
 // payload is junk).
-void StringToAnnexB(const std::string& str,
-                    std::vector<uint8_t>* buffer,
-                    std::vector<SubsampleEntry>* subsamples) {
+static void StringToAnnexB(const std::string& str,
+                           std::vector<uint8_t>* buffer,
+                           std::vector<SubsampleEntry>* subsamples) {
   DCHECK(!str.empty());
 
   std::vector<std::string> subsample_specs = base::SplitString(
@@ -176,8 +189,30 @@ void StringToAnnexB(const std::string& str,
   }
 }
 
-std::string AnnexBToString(const std::vector<uint8_t>& buffer,
-                           const std::vector<SubsampleEntry>& subsamples) {
+// Helper to compare two results of AVC::Analyze().
+static bool AnalysesMatch(const BitstreamConverter::AnalysisResult& r1,
+                          const BitstreamConverter::AnalysisResult& r2) {
+  return r1.is_conformant == r2.is_conformant &&
+         r1.is_keyframe == r2.is_keyframe;
+}
+
+// Helper output operator, for debugging/testability.
+std::ostream& operator<<(std::ostream& os,
+                         const BitstreamConverter::AnalysisResult& r) {
+  os << "{ is_conformant: "
+     << (r.is_conformant.has_value()
+             ? (r.is_conformant.value() ? "true" : "false")
+             : "nullopt/unknown")
+     << ", is_keyframe: "
+     << (r.is_keyframe.has_value() ? (r.is_keyframe.value() ? "true" : "false")
+                                   : "nullopt/unknown")
+     << " }";
+  return os;
+}
+
+static std::string AnnexBToString(
+    const std::vector<uint8_t>& buffer,
+    const std::vector<SubsampleEntry>& subsamples) {
   std::stringstream ss;
 
   H264Parser parser;
@@ -230,7 +265,14 @@ TEST_P(AVCConversionTest, ParseCorrectly) {
   std::vector<SubsampleEntry> subsamples;
   MakeInputForLength(GetParam(), &buf);
   EXPECT_TRUE(AVC::ConvertFrameToAnnexB(GetParam(), &buf, &subsamples));
-  EXPECT_TRUE(AVC::IsValidAnnexB(buf.data(), buf.size(), subsamples));
+
+  BitstreamConverter::AnalysisResult expected;
+  expected.is_conformant = true;
+  expected.is_keyframe = false;
+  EXPECT_PRED2(AnalysesMatch,
+               AVC::AnalyzeAnnexB(buf.data(), buf.size(), subsamples),
+               expected);
+
   EXPECT_EQ(buf.size(), sizeof(kExpected));
   EXPECT_EQ(0, memcmp(kExpected, &buf[0], sizeof(kExpected)));
   EXPECT_EQ("P,SDC", AnnexBToString(buf, subsamples));
@@ -350,63 +392,107 @@ TEST_F(AVCConversionTest, StringConversionFunctions) {
   std::vector<uint8_t> buf;
   std::vector<SubsampleEntry> subsamples;
   StringToAnnexB(str, &buf, &subsamples);
-  EXPECT_TRUE(AVC::IsValidAnnexB(buf.data(), buf.size(), subsamples));
+
+  BitstreamConverter::AnalysisResult expected;
+  expected.is_conformant = true;
+  expected.is_keyframe = true;
+  EXPECT_PRED2(AnalysesMatch,
+               AVC::AnalyzeAnnexB(buf.data(), buf.size(), subsamples),
+               expected);
 
   EXPECT_EQ(str, AnnexBToString(buf, subsamples));
 }
 
 TEST_F(AVCConversionTest, ValidAnnexBConstructs) {
-  const char* test_cases[] = {
-    "I",
-    "I I I I",
-    "AUD I",
-    "AUD SPS PPS I",
-    "I EOSeq",
-    "I EOSeq EOStr",
-    "I EOStr",
-    "P",
-    "P P P P",
-    "AUD SPS PPS P",
-    "SEI SEI I",
-    "SEI SEI R14 I",
-    "SPS SPSExt SPS PPS I P",
-    "R14 SEI I",
-    "AUD,I",
-    "AUD,SEI I",
-    "AUD,SEI,SPS,PPS,I"
+  struct {
+    const char* case_string;
+    const bool is_keyframe;
+  } test_cases[] = {
+      {"I", true},
+      {"I I I I", true},
+      {"AUD I", true},
+      {"AUD SPS PPS I", true},
+      {"I EOSeq", true},
+      {"I EOSeq EOStr", true},
+      {"I EOStr", true},
+      {"P", false},
+      {"P P P P", false},
+      {"AUD SPS PPS P", false},
+      {"SEI SEI I", true},
+      {"SEI SEI R14 I", true},
+      {"SPS SPSExt SPS PPS I P", true},
+      {"R14 SEI I", true},
+      {"AUD,I", true},
+      {"AUD,SEI I", true},
+      {"AUD,SEI,SPS,PPS,I", true},
+
+      // In reality, these might not always be conformant/valid, but assuming
+      // they are, they're not keyframes because a non-IDR slice preceded the
+      // IDR slice, if any.
+      {"SDA SDB SDC", false},
+      {"P I", false},
+      {"SDA I", false},
+      {"SDB I", false},
+      {"SDC I", false},
   };
 
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     std::vector<uint8_t> buf;
     std::vector<SubsampleEntry> subsamples;
-    StringToAnnexB(test_cases[i], &buf, NULL);
-    EXPECT_TRUE(AVC::IsValidAnnexB(buf.data(), buf.size(), subsamples))
-        << "'" << test_cases[i] << "' failed";
+    StringToAnnexB(test_cases[i].case_string, &buf, NULL);
+
+    BitstreamConverter::AnalysisResult expected;
+    expected.is_conformant = true;
+    expected.is_keyframe = test_cases[i].is_keyframe;
+    EXPECT_PRED2(AnalysesMatch,
+                 AVC::AnalyzeAnnexB(buf.data(), buf.size(), subsamples),
+                 expected)
+        << "'" << test_cases[i].case_string << "' failed";
   }
 }
 
 TEST_F(AVCConversionTest, InvalidAnnexBConstructs) {
-  static const char* test_cases[] = {
-    "AUD",  // No VCL present.
-    "AUD,SEI", // No VCL present.
-    "SPS PPS",  // No VCL present.
-    "SPS PPS AUD I",  // Parameter sets must come after AUD.
-    "SPSExt SPS P",  // SPS must come before SPSExt.
-    "SPS PPS SPSExt P",  // SPSExt must follow an SPS.
-    "EOSeq",  // EOSeq must come after a VCL.
-    "EOStr",  // EOStr must come after a VCL.
-    "I EOStr EOSeq",  // EOSeq must come before EOStr.
-    "I R14",  // Reserved14-18 must come before first VCL.
-    "I SEI",  // SEI must come before first VCL.
-    "P SPS P", // SPS after first VCL would indicate a new access unit.
+  struct {
+    const char* case_string;
+    const base::Optional<bool> is_keyframe;
+  } test_cases[] = {
+      // For these cases, lack of conformance is determined before detecting any
+      // IDR or non-IDR slices, so the non-conformant frames' keyframe analysis
+      // reports base::nullopt (which means undetermined analysis result).
+      {"AUD", base::nullopt},            // No VCL present.
+      {"AUD,SEI", base::nullopt},        // No VCL present.
+      {"SPS PPS", base::nullopt},        // No VCL present.
+      {"SPS PPS AUD I", base::nullopt},  // Parameter sets must come after AUD.
+      {"SPSExt SPS P", base::nullopt},   // SPS must come before SPSExt.
+      {"SPS PPS SPSExt P", base::nullopt},  // SPSExt must follow an SPS.
+      {"EOSeq", base::nullopt},             // EOSeq must come after a VCL.
+      {"EOStr", base::nullopt},             // EOStr must come after a VCL.
+
+      // For these cases, IDR slice is first VCL and is detected before
+      // conformance failure, so the non-conformant frame is reported as a
+      // keyframe.
+      {"I EOStr EOSeq", true},  // EOSeq must come before EOStr.
+      {"I R14", true},          // Reserved14-18 must come before first VCL.
+      {"I SEI", true},          // SEI must come before first VCL.
+
+      // For this case, P slice is first VCL and is detected before conformance
+      // failure, so the non-conformant frame is reported as a non-keyframe.
+      {"P SPS P",
+       false},  // SPS after first VCL would indicate a new access unit.
   };
 
+  BitstreamConverter::AnalysisResult expected;
+  expected.is_conformant = false;
+
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     std::vector<uint8_t> buf;
     std::vector<SubsampleEntry> subsamples;
-    StringToAnnexB(test_cases[i], &buf, NULL);
-    EXPECT_FALSE(AVC::IsValidAnnexB(buf.data(), buf.size(), subsamples))
-        << "'" << test_cases[i] << "' failed";
+    StringToAnnexB(test_cases[i].case_string, &buf, NULL);
+    expected.is_keyframe = test_cases[i].is_keyframe;
+    EXPECT_PRED2(AnalysesMatch,
+                 AVC::AnalyzeAnnexB(buf.data(), buf.size(), subsamples),
+                 expected)
+        << "'" << test_cases[i].case_string << "' failed";
   }
 }
 
@@ -442,6 +528,10 @@ TEST_F(AVCConversionTest, InsertParamSetsAnnexB) {
   avc_config.pps_list[0].push_back(0x56);
   avc_config.pps_list[0].push_back(0x78);
 
+  BitstreamConverter::AnalysisResult expected;
+  expected.is_conformant = true;
+  expected.is_keyframe = true;
+
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     std::vector<uint8_t> buf;
     std::vector<SubsampleEntry> subsamples;
@@ -450,7 +540,9 @@ TEST_F(AVCConversionTest, InsertParamSetsAnnexB) {
 
     EXPECT_TRUE(AVC::InsertParamSetsAnnexB(avc_config, &buf, &subsamples))
         << "'" << test_cases[i].input << "' insert failed.";
-    EXPECT_TRUE(AVC::IsValidAnnexB(buf.data(), buf.size(), subsamples))
+    EXPECT_PRED2(AnalysesMatch,
+                 AVC::AnalyzeAnnexB(buf.data(), buf.size(), subsamples),
+                 expected)
         << "'" << test_cases[i].input << "' created invalid AnnexB.";
     EXPECT_EQ(test_cases[i].expected, AnnexBToString(buf, subsamples))
         << "'" << test_cases[i].input << "' generated unexpected output.";

media/formats/mp4/bitstream_converter.cc

@@ -7,6 +7,13 @@
 namespace media {
 namespace mp4 {
 
+BitstreamConverter::AnalysisResult::AnalysisResult(){};
+
+BitstreamConverter::AnalysisResult::AnalysisResult(const AnalysisResult& other)
+    : is_conformant(other.is_conformant), is_keyframe(other.is_keyframe) {}
+
+BitstreamConverter::AnalysisResult::~AnalysisResult() = default;
+
 BitstreamConverter::~BitstreamConverter() = default;
 
 }  // namespace mp4

media/formats/mp4/bitstream_converter.h

@@ -10,6 +10,8 @@
 #include <vector>
 
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
+#include "media/base/media_export.h"
 
 namespace media {
 
@@ -20,9 +22,21 @@ namespace mp4 {
 // BitstreamConverter provides a unified interface for performing some common
 // bitstream conversions (e.g. H.264 MP4 bitstream to Annex B, and elementary
 // AAC stream to ADTS).
-class BitstreamConverter
+class MEDIA_EXPORT BitstreamConverter
     : public base::RefCountedThreadSafe<BitstreamConverter> {
  public:
+  // Describes the result of Analyze(). Not all analyses are implemented or
+  // enabled across mp4::BitstreamConverter implementations, hence the use of
+  // base::Optional<>.
+  struct MEDIA_EXPORT AnalysisResult {
+    AnalysisResult();
+    AnalysisResult(const AnalysisResult&);
+    ~AnalysisResult();
+
+    base::Optional<bool> is_conformant;
+    base::Optional<bool> is_keyframe;
+  };
+
   // Converts a single frame/buffer |frame_buf| into the output format.
   // Returns true iff the conversion was successful.
   // |frame_buf| is an input/output parameter, it contains input frame data and
@@ -37,11 +51,12 @@ class BitstreamConverter
                             bool is_keyframe,
                             std::vector<SubsampleEntry>* subsamples) const = 0;
 
-  // Checks a converted frame for conformance.
-  // Note: may return true even if the frame is not conformant; the checks may
-  // not be exhaustive (or implemented at all).
-  virtual bool IsValid(std::vector<uint8_t>* frame_buf,
-                       std::vector<SubsampleEntry>* subsamples) const = 0;
+  // Inspects an already converted frame for conformance. If conformant,
+  // inspects further to see if the converted frame appears to be a keyframe.
+  // Note, the checks may not be exhaustive (or implemented at all).
+  virtual AnalysisResult Analyze(
+      std::vector<uint8_t>* frame_buf,
+      std::vector<SubsampleEntry>* subsamples) const = 0;
 
  protected:
   friend class base::RefCountedThreadSafe<BitstreamConverter>;

media/formats/mp4/hevc.cc

@@ -137,11 +137,13 @@ VideoCodecProfile HEVCDecoderConfigurationRecord::GetVideoProfile() const {
 static const uint8_t kAnnexBStartCode[] = {0, 0, 0, 1};
 static const int kAnnexBStartCodeSize = 4;
 
+// static
 bool HEVC::InsertParamSetsAnnexB(
     const HEVCDecoderConfigurationRecord& hevc_config,
     std::vector<uint8_t>* buffer,
     std::vector<SubsampleEntry>* subsamples) {
-  DCHECK(HEVC::IsValidAnnexB(buffer->data(), buffer->size(), *subsamples));
+  DCHECK(HEVC::AnalyzeAnnexB(buffer->data(), buffer->size(), *subsamples)
+             .is_conformant.value_or(true));
 
   std::unique_ptr<H265Parser> parser(new H265Parser());
   const uint8_t* start = buffer->data();
@@ -179,10 +181,12 @@ bool HEVC::InsertParamSetsAnnexB(
   buffer->insert(config_insert_point,
                  param_sets.begin(), param_sets.end());
 
-  DCHECK(HEVC::IsValidAnnexB(buffer->data(), buffer->size(), *subsamples));
+  DCHECK(HEVC::AnalyzeAnnexB(buffer->data(), buffer->size(), *subsamples)
+             .is_conformant.value_or(true));
   return true;
 }
 
+// static
 bool HEVC::ConvertConfigToAnnexB(
     const HEVCDecoderConfigurationRecord& hevc_config,
     std::vector<uint8_t>* buffer) {
@@ -204,17 +208,23 @@ bool HEVC::ConvertConfigToAnnexB(
   return true;
 }
 
-// Verifies AnnexB NALU order according to section 7.4.2.4.4 of ISO/IEC 23008-2.
-bool HEVC::IsValidAnnexB(const uint8_t* buffer,
-                         size_t size,
-                         const std::vector<SubsampleEntry>& subsamples) {
+// static
+BitstreamConverter::AnalysisResult HEVC::AnalyzeAnnexB(
+    const uint8_t* buffer,
+    size_t size,
+    const std::vector<SubsampleEntry>& subsamples) {
   DCHECK(buffer);
 
-  if (size == 0)
-    return true;
+  BitstreamConverter::AnalysisResult result;
 
-  // TODO(servolk): Implement this, see crbug.com/527595
-  return true;
+  if (size == 0) {
+    result.is_conformant = true;
+    return result;
+  }
+
+  // TODO(servolk): Implement this, see https://crbug.com/527595. For now, we
+  // report that neither conformance nor keyframe analyses were performed.
+  return result;
 }
 
 HEVCBitstreamConverter::HEVCBitstreamConverter(
@@ -243,10 +253,10 @@ bool HEVCBitstreamConverter::ConvertFrame(
   return true;
 }
 
-bool HEVCBitstreamConverter::IsValid(
+BitstreamConverter::AnalysisResult HEVCBitstreamConverter::Analyze(
     std::vector<uint8_t>* frame_buf,
     std::vector<SubsampleEntry>* subsamples) const {
-  return HEVC::IsValidAnnexB(frame_buf->data(), frame_buf->size(), *subsamples);
+  return HEVC::AnalyzeAnnexB(frame_buf->data(), frame_buf->size(), *subsamples);
 }
 
 }  // namespace mp4

media/formats/mp4/hevc.h

@@ -78,14 +78,15 @@ class MEDIA_EXPORT HEVC {
       std::vector<uint8_t>* buffer,
       std::vector<SubsampleEntry>* subsamples);
 
-  // Verifies that the contents of |buffer| conform to
-  // Section 7.4.2.4.4 of ISO/IEC 23008-2.
+  // Analyzes the contents of |buffer| for conformance to
+  // Section 7.4.2.4.4 of ISO/IEC 23008-2, and if conformant, further inspects
+  // |buffer| to report whether or not it looks like a keyframe.
   // |subsamples| contains the information about what parts of the buffer are
   // encrypted and which parts are clear.
-  // Returns true if |buffer| contains conformant Annex B data
-  static bool IsValidAnnexB(const uint8_t* buffer,
-                            size_t size,
-                            const std::vector<SubsampleEntry>& subsamples);
+  static BitstreamConverter::AnalysisResult AnalyzeAnnexB(
+      const uint8_t* buffer,
+      size_t size,
+      const std::vector<SubsampleEntry>& subsamples);
 };
 
 class HEVCBitstreamConverter : public BitstreamConverter {
@@ -98,8 +99,9 @@ class HEVCBitstreamConverter : public BitstreamConverter {
                     bool is_keyframe,
                     std::vector<SubsampleEntry>* subsamples) const override;
 
-  bool IsValid(std::vector<uint8_t>* frame_buf,
-               std::vector<SubsampleEntry>* subsamples) const override;
+  AnalysisResult Analyze(
+      std::vector<uint8_t>* frame_buf,
+      std::vector<SubsampleEntry>* subsamples) const override;
 
  private:
   ~HEVCBitstreamConverter() override;

media/formats/mp4/mp4_stream_parser.cc

@@ -802,12 +802,21 @@ ParseResult MP4StreamParser::EnqueueSample(BufferQueueMap* buffers) {
             << "Failed to prepare video sample for decode";
         return ParseResult::kError;
       }
-      if (!runs_->video_description().frame_bitstream_converter->IsValid(
-              &frame_buf, &subsamples)) {
+      BitstreamConverter::AnalysisResult analysis =
+          runs_->video_description().frame_bitstream_converter->Analyze(
+              &frame_buf, &subsamples);
+      // If conformance analysis was not actually performed, assume the frame is
+      // conformant.  If it was performed and found to be non-conformant, log
+      // it.
+      if (!analysis.is_conformant.value_or(true)) {
         LIMITED_MEDIA_LOG(DEBUG, media_log_, num_invalid_conversions_,
                           kMaxInvalidConversionLogs)
             << "Prepared video sample is not conformant";
       }
+
+      // TODO(wolenetz): Use |analysis.is_keyframe|, if it was actually
+      // performed, for at least logging if the result mismatches container's
+      // keyframe metadata for |frame_buf|.
     }
   }