Move key frame flag from StreamParserBuffer to DecoderBuffer

media/base/decoder_buffer.h

 // Copyright (c) 2012 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.
 
 #ifndef MEDIA_BASE_DECODER_BUFFER_H_
 #define MEDIA_BASE_DECODER_BUFFER_H_
 
 #include <string>
 #include <utility>
 
@@ skipping 24 matching lines @@
   enum {
     kPaddingSize = 32,
 #if defined(ARCH_CPU_ARM_FAMILY)
     kAlignmentSize = 16
 #else
     kAlignmentSize = 32
 #endif
   };
 
   // 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.
   //
   // Calling any method other than end_of_stream() on the resulting buffer
   // is disallowed.
   static scoped_refptr<DecoderBuffer> CreateEOSBuffer();
 
   base::TimeDelta timestamp() const {
     DCHECK(!end_of_stream());
     return timestamp_;
@@ skipping 79 matching lines @@
     return splice_timestamp_;
   }
 
   // When set to anything but kNoTimestamp() indicates this buffer is part of a
   // splice around |splice_timestamp|.
   void set_splice_timestamp(base::TimeDelta splice_timestamp) {
     DCHECK(!end_of_stream());
     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();
 
  protected:
   friend class base::RefCountedThreadSafe<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:
   base::TimeDelta timestamp_;
   base::TimeDelta duration_;
 
   int size_;
   scoped_ptr<uint8, base::AlignedFreeDeleter> data_;
   int side_data_size_;
   scoped_ptr<uint8, base::AlignedFreeDeleter> side_data_;
   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();
 
   DISALLOW_COPY_AND_ASSIGN(DecoderBuffer);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_DECODER_BUFFER_H_