Move key frame flag from StreamParserBuffer to DecoderBuffer

media/base/decoder_buffer.h

Index: media/base/decoder_buffer.h
diff --git a/media/base/decoder_buffer.h b/media/base/decoder_buffer.h
index 092b2130c50257c1b0bc1aa31f3869026b4f1396..bc11fac9fdc7d4af8b0b5e967c99f40ab8e7f474 100644
--- a/media/base/decoder_buffer.h
+++ b/media/base/decoder_buffer.h
@@ -42,19 +42,23 @@ class MEDIA_EXPORT DecoderBuffer
   };
 
   // Allocates buffer with |size| >= 0.  Buffer will be padded and aligned
-  // as necessary.
+  // as necessary, and is_keyframe() will default to false.
   explicit DecoderBuffer(int size);
 
   // Create a DecoderBuffer whose |data_| is copied from |data|.  Buffer will be
   // padded and aligned as necessary.  |data| must not be NULL and |size| >= 0.
-  static scoped_refptr<DecoderBuffer> CopyFrom(const uint8* data, int size);
+  // |is_keyframe| indicates if the buffer is a random access point.
+  static scoped_refptr<DecoderBuffer> CopyFrom(const uint8* data, int size,
+                                               bool is_keyframe);

[xhwang, 2014/11/10 20:14:12]

We don't set a lot of variables in CopyFrom, e.g. the timestamp, duration etc.
Why |is_keyframe| is special? How about we treat it the same ways as timestamp?
(set with setter after construction)

This is also consistent with the ctor on l.46.

[wolenetz, 2014/11/11 22:39:41]

Done. This makes the change less extensive, too. Keeping the keyframe flag
populated correctly in code maintenance may be harder, though, as I mentioned
earlier.

 
   // Create a DecoderBuffer whose |data_| is copied from |data| and |side_data_|
   // is copied from |side_data|. Buffers will be padded and aligned as necessary
-  // Data pointers must not be NULL and sizes must be >= 0.
+  // Data pointers must not be NULL and sizes must be >= 0.  |is_keyframe|
+  // indicates if the buffer is a random access point.
   static scoped_refptr<DecoderBuffer> CopyFrom(const uint8* data, int size,
                                                const uint8* side_data,
-                                               int side_data_size);
+                                               int side_data_size,
+                                               bool is_keyframe);

[xhwang, 2014/11/10 20:14:12]

ditto.

[wolenetz, 2014/11/11 22:39:41]

Done.

 
   // Create a DecoderBuffer indicating we've reached end of stream.
   //
@@ -154,6 +158,16 @@ class MEDIA_EXPORT DecoderBuffer
     splice_timestamp_ = splice_timestamp;
   }
 
+  bool is_keyframe() const {
+    DCHECK(!end_of_stream());
+    return is_keyframe_;
+  }
+
+  void set_is_keyframe(bool is_keyframe) {
+    DCHECK(!end_of_stream());
+    is_keyframe_ = is_keyframe;
+  }
+
   // Returns a human-readable string describing |*this|.
   std::string AsHumanReadableString();
 
@@ -162,9 +176,11 @@ class MEDIA_EXPORT DecoderBuffer
 
   // Allocates a buffer of size |size| >= 0 and copies |data| into it.  Buffer
   // will be padded and aligned as necessary.  If |data| is NULL then |data_| is
-  // set to NULL and |buffer_size_| to 0.
+  // set to NULL and |buffer_size_| to 0.  |is_keyframe| indicates if the buffer
+  // is a random access point.
   DecoderBuffer(const uint8* data, int size,
-                const uint8* side_data, int side_data_size);
+                const uint8* side_data, int side_data_size,
+                bool is_keyframe);

[xhwang, 2014/11/10 20:14:12]

ditto

[wolenetz, 2014/11/11 22:39:41]

Done.

   virtual ~DecoderBuffer();
 
  private:
@@ -178,6 +194,7 @@ class MEDIA_EXPORT DecoderBuffer
   scoped_ptr<DecryptConfig> decrypt_config_;
   DiscardPadding discard_padding_;
   base::TimeDelta splice_timestamp_;
+  bool is_keyframe_;

[xhwang, 2014/11/10 20:14:12]

It seems to me key and frame should be two words, e.g.

http://en.wikipedia.org/wiki/Key_frame

For comparison, timestamp is one word according to wikipedia:

http://en.wikipedia.org/wiki/Timestamp

[wolenetz, 2014/11/11 22:39:41]

There is much inconsistency on the web about this one. I personally agree with
your comment, though "keyframe" pre-exists all over media/ (especially in stream
parsers and in source_buffer_stream*), and it shows up even in specs and docs
like:
http://matroska.org/technical/specs/index.html
http://www.webmproject.org/docs/container/

I'll not further propagate "keyframe" in this CL, though there are over 300
instances in media/ that this CL does not change.

 
   // Constructor helper method for memory allocations.
   void Initialize();