Fix mp4 keyframe parsing, removing unused stss parsing.

media/formats/mp4/track_run_iterator.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/formats/mp4/track_run_iterator.h"
 
 #include <algorithm>
+#include <iomanip>
 
 #include "media/base/buffers.h"
 #include "media/formats/mp4/rcheck.h"
 #include "media/formats/mp4/sample_to_group_iterator.h"
 
 namespace media {
 namespace mp4 {
 
 struct SampleInfo {
   int size;
   int duration;
   int cts_offset;
   bool is_keyframe;
-  bool is_random_access_point;
   uint32 cenc_group_description_index;
 };
 
 struct TrackRunInfo {
   uint32 track_id;
   std::vector<SampleInfo> samples;
   int64 timescale;
   int64 start_dts;
   int64 sample_start_offset;
 
@@ skipping 50 matching lines @@
 }
 
 TrackRunIterator::TrackRunIterator(const Movie* moov,
                                    const scoped_refptr<MediaLog>& media_log)
     : moov_(moov), media_log_(media_log), sample_offset_(0) {
   CHECK(moov);
 }
 
 TrackRunIterator::~TrackRunIterator() {}
 
+static std::string HexFlags(uint32 flags) {
+  std::stringstream stream;
+  stream << std::setfill('0') << std::setw(sizeof(uint32)*2) << std::hex
+         << flags;
+  return stream.str();
+}
+
 static bool PopulateSampleInfo(const TrackExtends& trex,
                                const TrackFragmentHeader& tfhd,
                                const TrackFragmentRun& trun,
                                const int64 edit_list_offset,
                                const uint32 i,
                                SampleInfo* sample_info,
                                const SampleDependsOn sdtp_sample_depends_on,
+                               bool is_audio,
                                const scoped_refptr<MediaLog>& media_log) {
   if (i < trun.sample_sizes.size()) {
     sample_info->size = trun.sample_sizes[i];
   } else if (tfhd.default_sample_size > 0) {
     sample_info->size = tfhd.default_sample_size;
   } else {
     sample_info->size = trex.default_sample_size;
   }
 
   if (i < trun.sample_durations.size()) {
     sample_info->duration = trun.sample_durations[i];
   } else if (tfhd.default_sample_duration > 0) {
     sample_info->duration = tfhd.default_sample_duration;
   } else {
     sample_info->duration = trex.default_sample_duration;
   }
 
   if (i < trun.sample_composition_time_offsets.size()) {
     sample_info->cts_offset = trun.sample_composition_time_offsets[i];
   } else {
     sample_info->cts_offset = 0;
   }
   sample_info->cts_offset += edit_list_offset;
 
   uint32 flags;
   if (i < trun.sample_flags.size()) {
     flags = trun.sample_flags[i];
+    DVLOG(4) << __FUNCTION__ << " trun sample flags "  << HexFlags(flags);
   } else if (tfhd.has_default_sample_flags) {
     flags = tfhd.default_sample_flags;
+    DVLOG(4) << __FUNCTION__ << " tfhd sample flags "  << HexFlags(flags);
   } else {
     flags = trex.default_sample_flags;
+    DVLOG(4) << __FUNCTION__ << " trex sample flags "  << HexFlags(flags);
   }
 
   SampleDependsOn sample_depends_on =
       static_cast<SampleDependsOn>((flags >> 24) & 0x3);
+  if (sample_depends_on == kSampleDependsOnUnknown) {
+    sample_depends_on = sdtp_sample_depends_on;
+  }
+  DVLOG(4) << __FUNCTION__ << " sample_depends_on "  << sample_depends_on;
+  if (sample_depends_on == kSampleDependsOnReserved) {
+    MEDIA_LOG(ERROR, media_log) << "Reserved value used in sample dependency"
+                                   " info.";
+    return false;
+  }
 
-  if (sample_depends_on == kSampleDependsOnUnknown)
-    sample_depends_on = sdtp_sample_depends_on;
+  // Per spec (ISO 14496-12:2012), the definition for a "sync sample" is
+  // equivalent to the downstream code's "is keyframe" concept. But media exists
+  // that marks non-key video frames as sync samples (http://crbug.com/507916
+  // and http://crbug.com/310712). Hence, for video we additionally check that
+  // the sample does not depend on others (FFmpeg does to, see mov_read_trun).

[wolenetz, 2015/08/27 03:01:30]

nit: s/to/too/

[chcunningham, 2015/08/27 18:59:39]

Done.

+  // Sample dependency is not ignored for audio because encoded audio samples
+  // can depend on other samples and still be used for random access. Generally
+  // all audio samples are expected to be sync samples, but we  prefer to check
+  // the flags to catch badly muxed audio (for now anyway ;P). History of
+  // attempts to get this right discussed in http://crrev.com/1319813002
+  bool sample_is_sync_sample = !(flags & kSampleIsNonSyncSample);
+  bool sample_depends_on_others = sample_depends_on == kSampleDependsOnOthers;

[wolenetz, 2015/08/27 03:01:30]

Hmm. This patch drops the automatic compiler check that we're doing something
for each possible SampleDependsOn enumerated case (see the removed switch
statement). Does this patch make the code more fragile?

[chcunningham, 2015/08/27 18:59:39]

I don't think this is more fragile. There are just 4 possible values (just 2
bits). 

1. reserved - logs an error and bails
2. depends on others -> !keyframe
3. depends on unknown -> keyframe if is_sync
4. depends on no others -> keyframe if is sync

[wolenetz, 2015/08/28 18:23:37]

Acknowledged.

+  sample_info->is_keyframe = sample_is_sync_sample &&
+                             (!sample_depends_on_others || is_audio);
 
-  // ISO/IEC 14496-12  Section 8.8.3.1 : The negation of |sample_is_sync_sample|
-  // provides the same information as the sync sample table [8.6.2]. When
-  // |sample_is_sync_sample| is true for a sample, it is the same as if the
-  // sample were not in a movie fragment and marked with an entry in the sync
-  // sample table (or, if all samples are sync samples, the sync sample table
-  // were absent).
-  bool sample_is_sync_sample = !(flags & kSampleIsNonSyncSample);
-  sample_info->is_random_access_point = sample_is_sync_sample;
+  DVLOG(4) << __FUNCTION__ << " is_kf:" << sample_info->is_keyframe
+           << " is_sync:" << sample_is_sync_sample
+           << " deps:" << sample_depends_on_others
+           << " audio:" << is_audio;

[wolenetz, 2015/08/27 03:01:30]

Do we catch non-keyframe audio buffers in parser, coded frame processor or
elsewhere and LIMITED_MEDIA_LOG them at least (if not MEDIA_LOG( ERROR ) + parse
err)?

[chcunningham, 2015/08/27 18:59:39]

We used to (https://b.corp.google.com/issues/12218009#comment6), but thats not
the case today. I can add a warning log somewhere... does the all-audio-is-key
assumption hold for all mse codecs? if so, I lean toward putting it downstream
of mp4 parsing to catch other codecs as well.

[wolenetz, 2015/08/28 18:23:37]

I'll take a look at this (audio frames potentially not marked as keyframes by
Chrome MSE parsers) separately. It could be causing issues that I'm already
looking into since this CR started...

 
-  switch (sample_depends_on) {
-    case kSampleDependsOnUnknown:
-      sample_info->is_keyframe = sample_is_sync_sample;
-      break;
-
-    case kSampleDependsOnOthers:
-      sample_info->is_keyframe = false;
-      break;
-
-    case kSampleDependsOnNoOther:
-      sample_info->is_keyframe = true;
-      break;
-
-    case kSampleDependsOnReserved:
-      MEDIA_LOG(ERROR, media_log) << "Reserved value used in sample dependency"
-                                     " info.";
-      return false;
-  }
   return true;
 }
 
 static const CencSampleEncryptionInfoEntry* GetSampleEncryptionInfoEntry(
     const TrackRunInfo& run_info,
     uint32 group_description_index) {
   const std::vector<CencSampleEncryptionInfoEntry>* entries = nullptr;
 
   // ISO-14496-12 Section 8.9.2.3 and 8.9.4 : group description index
   // (1) ranges from 1 to the number of sample group entries in the track
@@ skipping 160 matching lines @@
         }
       } else {
         tri.aux_info_start_offset = -1;
         tri.aux_info_total_size = 0;
       }
 
       tri.samples.resize(trun.sample_count);
       for (size_t k = 0; k < trun.sample_count; k++) {
         if (!PopulateSampleInfo(*trex, traf.header, trun, edit_list_offset, k,
                                 &tri.samples[k], traf.sdtp.sample_depends_on(k),
-                                media_log_)) {
+                                tri.is_audio, media_log_)) {
           return false;
         }
 
         run_start_dts += tri.samples[k].duration;
 
         if (!is_sample_to_group_valid) {
           // Set group description index to 0 to read encryption information
           // from TrackEncryption Box.
           tri.samples[k].cenc_group_description_index = 0;
           continue;
@@ skipping 160 matching lines @@
 base::TimeDelta TrackRunIterator::duration() const {
   DCHECK(IsSampleValid());
   return TimeDeltaFromRational(sample_itr_->duration, run_itr_->timescale);
 }
 
 bool TrackRunIterator::is_keyframe() const {
   DCHECK(IsSampleValid());
   return sample_itr_->is_keyframe;
 }
 
-bool TrackRunIterator::is_random_access_point() const {
-  DCHECK(IsSampleValid());
-  return sample_itr_->is_random_access_point;
-}
-
 const TrackEncryption& TrackRunIterator::track_encryption() const {
   if (is_audio())
     return audio_description().sinf.info.track_encryption;
   return video_description().sinf.info.track_encryption;
 }
 
 scoped_ptr<DecryptConfig> TrackRunIterator::GetDecryptConfig() {
   DCHECK(is_encrypted());
 
   if (cenc_info_.empty()) {
@@ skipping 43 matching lines @@
 }
 
 uint8 TrackRunIterator::GetIvSize(size_t sample_index) const {
   uint32 index = GetGroupDescriptionIndex(sample_index);
   return (index == 0) ? track_encryption().default_iv_size
                       : GetSampleEncryptionInfoEntry(*run_itr_, index)->iv_size;
 }
 
 }  // namespace mp4
 }  // namespace media