Bug 1513651 - Collect telemetry on sample description box entries when parsing mp4s. r=jya
authorBryce Van Dyk <bvandyk@mozilla.com>
Mon, 11 Feb 2019 20:07:51 +0000
changeset 459137 c387900cf31b23ff830fc3f101303c8ac761b544
parent 459136 5e8d2e49bb8c705a8f32d3a20fdfbd874624dc4b
child 459138 946f649535c1b323296e3ba2fc039db1fe4d0a63
push id35556
push userdvarga@mozilla.com
push dateFri, 15 Feb 2019 01:38:24 +0000
treeherdermozilla-central@b29c87add05f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjya
bugs1513651
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1513651 - Collect telemetry on sample description box entries when parsing mp4s. r=jya This patch adds telemetry to help us determine if mp4s with certain structures can occur in the wild. Specifically: - How often do mp4s have multiple entries in the sample description box? - Do we ever see mp4s with multiple codecs in the sample description box? - Do we ever see mp4s with multiple sets of crypto info in the sample description box? This information is collected each time we parse mp4 metadata. Remove some diagnostic asserts now that we're gracefully gathering info. Reshuffle of DecoderData function order to align with the order in the header. Differential Revision: https://phabricator.services.mozilla.com/D14426
dom/media/mp4/DecoderData.cpp
toolkit/components/telemetry/Histograms.json
--- a/dom/media/mp4/DecoderData.cpp
+++ b/dom/media/mp4/DecoderData.cpp
@@ -3,16 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "Adts.h"
 #include "AnnexB.h"
 #include "BufferReader.h"
 #include "DecoderData.h"
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/EndianUtils.h"
+#include "mozilla/Telemetry.h"
 #include "VideoUtils.h"
 
 // OpusDecoder header is really needed only by MP4 in rust
 #include "OpusDecoder.h"
 #include "mp4parse.h"
 
 using mozilla::media::TimeUnit;
 
@@ -35,23 +36,16 @@ mozilla::Result<mozilla::Ok, nsresult> C
     if (!reader.ReadArray(psshInfo.data, length)) {
       return mozilla::Err(NS_ERROR_FAILURE);
     }
     pssh.AppendElement(psshInfo);
   }
   return mozilla::Ok();
 }
 
-bool MP4AudioInfo::IsValid() const {
-  return mChannels > 0 && mRate > 0 &&
-         // Accept any mime type here, but if it's aac, validate the profile.
-         (!mMimeType.EqualsLiteral("audio/mp4a-latm") || mProfile > 0 ||
-          mExtendedProfile > 0);
-}
-
 static MediaResult UpdateTrackProtectedInfo(mozilla::TrackInfo& aConfig,
                                             const Mp4parseSinfInfo& aSinf) {
   if (aSinf.is_encrypted != 0) {
     if (aSinf.scheme_type == MP4_PARSE_ENCRYPTION_SCHEME_TYPE_CENC) {
       aConfig.mCrypto.mCryptoScheme = CryptoScheme::Cenc;
     } else if (aSinf.scheme_type == MP4_PARSE_ENCRYPTION_SCHEME_TYPE_CBCS) {
       aConfig.mCrypto.mCryptoScheme = CryptoScheme::Cbcs;
     } else {
@@ -67,50 +61,101 @@ static MediaResult UpdateTrackProtectedI
     aConfig.mCrypto.mCryptByteBlock = aSinf.crypt_byte_block;
     aConfig.mCrypto.mSkipByteBlock = aSinf.skip_byte_block;
     aConfig.mCrypto.mConstantIV.AppendElements(aSinf.constant_iv.data,
                                                aSinf.constant_iv.length);
   }
   return NS_OK;
 }
 
-MediaResult MP4AudioInfo::Update(const Mp4parseTrackInfo* track,
-                                 const Mp4parseTrackAudioInfo* audio) {
-  MOZ_DIAGNOSTIC_ASSERT(audio->sample_info_count > 0,
-                        "Must have at least one audio sample info");
-  if (audio->sample_info_count == 0) {
-    return MediaResult(
-        NS_ERROR_DOM_MEDIA_METADATA_ERR,
-        RESULT_DETAIL("Got 0 audio sample info while updating audio track"));
-  }
+// Verify various information shared by Mp4ParseTrackAudioInfo and
+// Mp4ParseTrackVideoInfo and record telemetry on that info. Returns an
+// appropriate MediaResult indicating if the info is valid or not.
+// This verifies:
+// - That we have a sample_info_count > 0 (valid tracks should have at least one
+//   sample description entry)
+// - That only a single codec is used across all sample infos, as we don't
+//   handle multiple.
+// - That only a single sample info contains crypto info, as we don't handle
+//  multiple.
+//
+// Telemetry is also recorded on the above. As of writing, the
+// telemetry is recorded to give us early warning if MP4s exist that we're not
+// handling. Note, if adding new checks and telemetry to this function,
+// telemetry should be recorded before returning to ensure it is gathered.
+template <typename Mp4ParseTrackAudioOrVideoInfo>
+static MediaResult VerifyAudioOrVideoInfoAndRecordTelemetry(
+    Mp4ParseTrackAudioOrVideoInfo* audioOrVideoInfo) {
+  Telemetry::Accumulate(
+      Telemetry::MEDIA_MP4_PARSE_NUM_SAMPLE_DESCRIPTION_ENTRIES,
+      audioOrVideoInfo->sample_info_count);
 
   bool hasCrypto = false;
-  Mp4parseCodec codecType = audio->sample_info[0].codec_type;
-  for (uint32_t i = 0; i < audio->sample_info_count; i++) {
-    if (audio->sample_info[0].codec_type != codecType) {
-      // Different codecs in a single track. We don't handle this.
-      return MediaResult(
-          NS_ERROR_DOM_MEDIA_METADATA_ERR,
-          RESULT_DETAIL(
-              "Multiple codecs encountered while updating audio track"));
+  bool hasMultipleCodecs = false;
+  bool hasMultipleCrypto = false;
+  Mp4parseCodec codecType = audioOrVideoInfo->sample_info[0].codec_type;
+  for (uint32_t i = 0; i < audioOrVideoInfo->sample_info_count; i++) {
+    if (audioOrVideoInfo->sample_info[0].codec_type != codecType) {
+      hasMultipleCodecs = true;
     }
 
     // Update our encryption info if any is present on the sample info.
-    if (audio->sample_info[i].protected_data.is_encrypted) {
+    if (audioOrVideoInfo->sample_info[i].protected_data.is_encrypted) {
       if (hasCrypto) {
-        // Multiple crypto entries found. We don't handle this.
-        return MediaResult(
-            NS_ERROR_DOM_MEDIA_METADATA_ERR,
-            RESULT_DETAIL(
-                "Multiple crypto info encountered while updating audio track"));
+        hasMultipleCrypto = true;
       }
+      hasCrypto = true;
+    }
+  }
+
+  Telemetry::Accumulate(
+      Telemetry::
+          MEDIA_MP4_PARSE_SAMPLE_DESCRIPTION_ENTRIES_HAVE_MULTIPLE_CODECS,
+      hasMultipleCodecs);
+
+  Telemetry::Accumulate(
+      Telemetry::
+          MEDIA_MP4_PARSE_SAMPLE_DESCRIPTION_ENTRIES_HAVE_MULTIPLE_CRYPTO,
+      hasMultipleCrypto);
+
+  if (audioOrVideoInfo->sample_info_count == 0) {
+    return MediaResult(
+        NS_ERROR_DOM_MEDIA_METADATA_ERR,
+        RESULT_DETAIL("Got 0 sample info while verifying track."));
+  }
+
+  if (hasMultipleCodecs) {
+    // Different codecs in a single track. We don't handle this.
+    return MediaResult(
+        NS_ERROR_DOM_MEDIA_METADATA_ERR,
+        RESULT_DETAIL("Multiple codecs encountered while verifying track."));
+  }
+
+  if (hasMultipleCrypto) {
+    // Multiple crypto entries found. We don't handle this.
+    return MediaResult(
+        NS_ERROR_DOM_MEDIA_METADATA_ERR,
+        RESULT_DETAIL(
+            "Multiple crypto info encountered while verifying track."));
+  }
+  return NS_OK;
+}
+
+MediaResult MP4AudioInfo::Update(const Mp4parseTrackInfo* track,
+                                 const Mp4parseTrackAudioInfo* audio) {
+  auto rv = VerifyAudioOrVideoInfoAndRecordTelemetry(audio);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  Mp4parseCodec codecType = audio->sample_info[0].codec_type;
+  for (uint32_t i = 0; i < audio->sample_info_count; i++) {
+    if (audio->sample_info[i].protected_data.is_encrypted) {
       auto rv =
           UpdateTrackProtectedInfo(*this, audio->sample_info[i].protected_data);
       NS_ENSURE_SUCCESS(rv, rv);
-      hasCrypto = true;
+      break;
     }
   }
 
   // We assume that the members of the first sample info are representative of
   // the entire track. This code will need to be updated should this assumption
   // ever not hold. E.g. if we need to handle different codecs in a single
   // track, or if we have different numbers or channels in a single track.
   Mp4parseByteData codecSpecificConfig =
@@ -153,50 +198,35 @@ MediaResult MP4AudioInfo::Update(const M
   Mp4parseByteData extraData = audio->sample_info[0].extra_data;
   // If length is 0 we append nothing
   mExtraData->AppendElements(extraData.data, extraData.length);
   mCodecSpecificConfig->AppendElements(codecSpecificConfig.data,
                                        codecSpecificConfig.length);
   return NS_OK;
 }
 
+bool MP4AudioInfo::IsValid() const {
+  return mChannels > 0 && mRate > 0 &&
+         // Accept any mime type here, but if it's aac, validate the profile.
+         (!mMimeType.EqualsLiteral("audio/mp4a-latm") || mProfile > 0 ||
+          mExtendedProfile > 0);
+}
+
 MediaResult MP4VideoInfo::Update(const Mp4parseTrackInfo* track,
                                  const Mp4parseTrackVideoInfo* video) {
-  MOZ_DIAGNOSTIC_ASSERT(video->sample_info_count > 0,
-                        "Must have at least one video sample info");
-  if (video->sample_info_count == 0) {
-    return MediaResult(
-        NS_ERROR_DOM_MEDIA_METADATA_ERR,
-        RESULT_DETAIL("Got 0 audio sample info while updating video track"));
-  }
+  auto rv = VerifyAudioOrVideoInfoAndRecordTelemetry(video);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  bool hasCrypto = false;
   Mp4parseCodec codecType = video->sample_info[0].codec_type;
   for (uint32_t i = 0; i < video->sample_info_count; i++) {
-    if (video->sample_info[0].codec_type != codecType) {
-      // Different codecs in a single track. We don't handle this.
-      return MediaResult(
-          NS_ERROR_DOM_MEDIA_METADATA_ERR,
-          RESULT_DETAIL(
-              "Multiple codecs encountered while updating video track"));
-    }
-
-    // Update our encryption info if any is present on the sample info.
     if (video->sample_info[i].protected_data.is_encrypted) {
-      if (hasCrypto) {
-        // Multiple crypto entries found. We don't handle this.
-        return MediaResult(
-            NS_ERROR_DOM_MEDIA_METADATA_ERR,
-            RESULT_DETAIL(
-                "Multiple crypto info encountered while updating video track"));
-      }
       auto rv =
           UpdateTrackProtectedInfo(*this, video->sample_info[i].protected_data);
       NS_ENSURE_SUCCESS(rv, rv);
-      hasCrypto = true;
+      break;
     }
   }
 
   // We assume that the members of the first sample info are representative of
   // the entire track. This code will need to be updated should this assumption
   // ever not hold. E.g. if we need to handle different codecs in a single
   // track, or if we have different numbers or channels in a single track.
   if (codecType == MP4PARSE_CODEC_AVC) {
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -8675,16 +8675,41 @@
     "record_in_processes": ["main", "content"],
     "alert_emails": ["jya@mozilla.com", "drno@ohlmeier.org"],
     "bug_numbers": [1368596],
     "expires_in_version": "63",
     "kind": "enumerated",
     "n_values": 10,
     "description": "Counts types of deprecation warnings logged on every successful call to navigator.requestMediaKeySystemAccess(). 0=No warnings logged, 1=MediaEMENoCapabilitiesDeprecatedWarning, 2=MediaEMENoCodecsDeprecatedWarning."
   },
+  "MEDIA_MP4_PARSE_SAMPLE_DESCRIPTION_ENTRIES_HAVE_MULTIPLE_CODECS": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["bvandyk@mozilla.com", "jya@mozilla.com", "drno@ohlmeier.org"],
+    "bug_numbers": [1513651],
+    "expires_in_version": "never",
+    "kind": "boolean",
+    "description": "Records if multiple codecs are present in a track's sample description entries. Recorded each time we process a track's metadata while parsing mp4s."
+  },
+  "MEDIA_MP4_PARSE_SAMPLE_DESCRIPTION_ENTRIES_HAVE_MULTIPLE_CRYPTO": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["bvandyk@mozilla.com", "jya@mozilla.com", "drno@ohlmeier.org"],
+    "bug_numbers": [1513651],
+    "expires_in_version": "never",
+    "kind": "boolean",
+    "description": "Records if multiple sets of crypto info are present in a track's sample description entries. Recorded each time we process a track's metadata while parsing mp4s."
+  },
+  "MEDIA_MP4_PARSE_NUM_SAMPLE_DESCRIPTION_ENTRIES": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["bvandyk@mozilla.com", "jya@mozilla.com", "drno@ohlmeier.org"],
+    "bug_numbers": [1513651],
+    "expires_in_version": "never",
+    "kind": "enumerated",
+    "n_values": 5,
+    "description": "Counts the number of entries in the sample description box (stsd) for a track in an mp4. Recorded each time we process a track's metadata while parsing mp4s."
+  },
   "MEDIACACHE_WATERMARK_KB": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["gsquelart@mozilla.com"],
     "bug_numbers": [1366929],
     "expires_in_version": "60",
     "kind": "linear",
     "high": 520000,
     "n_buckets": 66,