Interfaces for encoded video sources

media/base/encoded_bitstream_buffer.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

Drop the (c) (here and elsewhere)
https://code.google.com/p/chromium/issues/detail?id=140977

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_
+#define MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_
+
+#include <ostream>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "media/base/media_export.h"
+#include "media/video/video_encode_types.h"
+
+namespace media {
+
+// Class to represent encoded video bitstream buffers encapsulating both the
+// bitstream data and the metadata from the encoding context into single object.
+// Class implements reference counting mechanism so that instances can be passed
+// between threads (and possibly to multiple clients) conveniently without
+// requiring deep copies and lifetime of each object is tied to scoped_refptrs
+// held by the clients for the duration of the required processing.

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

The sentence of l.20-23 sounds like PR for scoped_refptr, and I feel is almost
exactly backwards.

Nobody knows what the lifetime of a refcounted object is supposed to be, much
less what it actually is.
RefCountedThreadSafe doesn't buy you anything vis-a-vis deep copies.
Ref-counting, once added, becomes much more difficult to remove than it would
have been to avoid in the first place.

Of course, this is an entirely theoretical argument, because there is no code in
this CL that actually instantiates this class.  See my first comment in this
review.

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

I am still defending the use of reference counting mechanism on the buffers. I
think it is very effective way to pass over buffers over thread boundaries and
not worry about ownership of these objects that are for one-shot consumption
only.
I have to draw the line somewhere on how wide I am going to expand this CL to
ever get it in, which is why I put stubs into IPC in the latest patch. I am
completely ok to get rid of the reference counting if nobody else sees the
benefits it provides on the follow up CLs that instantiate this class.

+//
+// As long as constantness of the buffer is respected by the clients class is
+// also thread safe.
+class MEDIA_EXPORT EncodedBitstreamBuffer :
+    public base::RefCountedThreadSafe<EncodedBitstreamBuffer> {
+ public:
+  // Constructor that copies |size| bytes of data from |buffer| into newly
+  // allocated buffer.
+  EncodedBitstreamBuffer(const uint8* buffer,
+                         size_t size,
+                         const media::BufferEncodingMetadata& metadata);
+  // TODO(vmr): SharedMemory handle in addition to (or instead of) copy
+  //            constructor?
+
+  // Accessors for properties.
+  const uint8* buffer() const;
+  size_t size() const;
+  const base::Time timestamp() const;

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

For this and all the following accessors, why not just have
const media::BufferEncodingMetadata& metadata() { return metadata_; }
?

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Done.

+  int temporal_layer_id() const;
+  bool is_key_frame() const;
+  bool is_altref_frame() const;
+  bool is_golden_frame() const;
+  bool is_droppable() const;
+  bool is_layer_sync() const;
+
+ protected:
+  virtual ~EncodedBitstreamBuffer();
+  friend class base::RefCountedThreadSafe<EncodedBitstreamBuffer>;
+
+ private:
+  scoped_ptr<uint8[]> buffer_;
+  size_t size_;
+  media::BufferEncodingMetadata metadata_;
+
+  DISALLOW_COPY_AND_ASSIGN(EncodedBitstreamBuffer);
+};
+
+}  // namespace media
+
+// Pretty print operator for the EncodedBitstreamBuffer.
+std::ostream& operator<<(std::ostream& output,
+                         const media::EncodedBitstreamBuffer& b);

[Ami GONE FROM CHROMIUM, 2013/03/18 22:53:45]

This isn't landable.
You want a ToDebugString() method instead.

[Ville-Mikko Rautio, 2013/03/19 16:45:43]

Done.

+
+#endif  // MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_