Define EncodedVideoSource and RtcCapturedEncodingVideoCapturer.

media/base/encoded_bitstream_buffer.h

Index: media/base/encoded_bitstream_buffer.h
diff --git a/media/base/encoded_bitstream_buffer.h b/media/base/encoded_bitstream_buffer.h
new file mode 100644
index 0000000000000000000000000000000000000000..d90ffcd2c5a8d9c6460b77e9d37ef50b2d7fa18f
--- /dev/null
+++ b/media/base/encoded_bitstream_buffer.h
@@ -0,0 +1,48 @@
+// Copyright (c) 2013 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_ENCODED_BITSTREAM_BUFFER_H_
+#define MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/shared_memory.h"
+#include "base/time.h"
+#include "media/base/media_export.h"
+#include "media/video/video_encode_types.h"
+
+namespace media {
+
+// General encoded video bitstream buffer.

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

This sort of comment (which appears elsewhere as well) is not useful.

Things that can be useful to doco in a class-level comment include:
- intended use-cases
- clients
- caller conventions: lifecycle, thread-safety/friendliness/hostility
etc.

+class EncodedBitstreamBuffer :

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

Seems strange to have this time given that media::BitstreamBuffer exists.

Can you extend that with the metadata you need instead?
(might not be able to if you really don't want SHM; see below)

[hshi1, 2013/06/12 17:52:39]

I'm leaving this as-is for now. Need to review sheu@'s CL to see how EBB is used
there.

+    public base::RefCountedThreadSafe<EncodedBitstreamBuffer> {
+ public:
+  // Constructor that maps the shared memory with the given size to the current
+  // process and associates the given metadata with the buffer.
+  EncodedBitstreamBuffer(int buffer_id,
+                         uint8* buffer,

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

comment (and off-reviewlog convo) says "shared memory" but this param is a
pointer; is there SHM here or not?
if not, you need to doco the ownership/lifecycle of |buffer|; does |this| now
own |buffer| and expected to release it at some point?
I suspect the answer is "no" (since buffer_ is not a scoped_ptr and the dtor
doesn't delete it) but then something must be keeping track of the memory & the
need to free it later.  I'm surprised this doesn't have something like
VideoFrame::no_longer_needed_cb_.

+                         size_t size,
+                         const media::BufferEncodingMetadata& metadata);

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

Is there any reason for this to be a separate struct rather than part of EBB?

+  // Accessors for properties.
+  int buffer_id() const;
+  const uint8* buffer() const;
+  size_t size() const;
+  const media::BufferEncodingMetadata& metadata() const;
+
+ protected:

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

private?

[hshi1, 2013/06/12 17:52:39]

Done.

+  // Destructor that deallocates the buffers.

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

Comment is a lie given the impl.

[hshi1, 2013/06/12 17:52:39]

Removed; but see sheu@'s CL. I believe he intends to re-use this class as a
container for EVB on the brower side also, which creates a situation where the
EVB may or may not own the SHM.

+  virtual ~EncodedBitstreamBuffer();
+  friend class base::RefCountedThreadSafe<EncodedBitstreamBuffer>;
+
+ private:
+  int buffer_id_;
+  uint8* buffer_;
+  size_t size_;

[Ami GONE FROM CHROMIUM, 2013/06/08 00:18:01]

IPC guidelines require this piece of data to be explicitly sized in the IPC so I
think you're better off making it uint64 everywhere.

[hshi1, 2013/06/12 17:52:39]

Wouldn't an int do? I don't think bitstream buffer is ever going to exceed 32
bits.

+  media::BufferEncodingMetadata metadata_;
+
+  DISALLOW_COPY_AND_ASSIGN(EncodedBitstreamBuffer);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_