Interfaces for encoded video sources

media/base/encoded_bitstream_buffer.h

+// 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 <ostream>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+// General encoded video bitstream buffer.

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

This is not a useful comment.
Things that are useful to include in class-level comments include: example
use-cases, lifecycle notes, threading information, and so on.
(ditto for the other .h file in this CL)

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+class MEDIA_EXPORT EncodedBitstreamBuffer :

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

this class looks single threaded. Can you either inherit from
base::NonThreadSafe or add a base::ThreadChecker member and check that methods
are being called on the correct thread?

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

Shouldn't the ctor take a EncodingMetadata struct instead of maintaining all the
same properties itself?

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

It could take EncodingMetadata, I added TODO to rebase this one on top of it
once it lands.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

I want this class to be readable concurrently from multiple thread contexts.
This class is supposed to be constructed from IPC message coming from another
process and be processed by one or more clients in parallel. To enforce this, I
also took your suggestions to go full const in the accessors.

+    public base::RefCountedThreadSafe<EncodedBitstreamBuffer> {

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

Unless you have a good reason for this to be refcounted, you shouldn't (prefer
scoped_ptr and explicit ownership to scoped_refptr and shared/vague ownership)

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

The original purpose of this class has been to carry out buffers from IPC calls
to the sandboxed clients.

As there are many involved threads and amount of data amount is considerable
enough to avoid unnecessary extra deep copies, I chose refcounted to be able to
implement transitional placeholder object who will carry data conveniently from
thread and message loop to another without requiring deep copies and minimal
ownership management.

Now this class works nicely as is:
1. Lifetime of the object is tied to existence of scoped_refptr's (as long as
somebody has the handle on the data).
2. Passing the buffer along threads and message loops without any deep copies is
easy and transparent.

I can't see any problems in the lifetime of the object as long as clients don't
store the scoped_refptr's returned by EVS API for longer than they should. I'll
add extra comment about this into the EVS API.

+ public:
+  // Constructor that allocates empty buffer of |size|.
+  EncodedBitstreamBuffer(size_t size, base::Time timestamp, int temporal_layer);

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

nit: const base::Time&

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

base::Time is just an int64; it's actually more efficient to pass it by copy
(see chromium-dev for relevant threads if curious).

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

In what situation will you want this overload?

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

According to Ami copy is more efficient, going with const, though.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

I thought that this could be a situation for an software encoder, where it wants
to already allocate a buffer for it's output before encoding. But I'll get rid
of it, as software encoder can as well use the other constructor and I decided
to go all const with respect to the buffer contained within this class.

+  // Constructor that copies |size| bytes of data from |buffer| into newly
+  // allocated buffer.
+  EncodedBitstreamBuffer(uint8* buffer,
+                         size_t size,
+                         base::Time timestamp,

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

same here

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

According to Ami copy is more efficient, going with const, though.

+                         int temporal_layer);
+
+  // Accessors for properties.
+  uint8* buffer() const;

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

nit: we rarely have const methods that provide write access to the class' data. 
Can we either make this:
const uint8* buffer() const;
or
uint8* buffer();

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

I'll take the full const way to emphasize read-only nature of the buffer for
clients.

+  size_t size() const;
+  bool key_frame() const;

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

these bool accessors (and members) probably want an is_ prefix

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+  bool altref_frame() const;
+  bool golden_frame() const;
+  base::Time timestamp() const;

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

nit: const base::Time& timestamp() const;

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+  int temporal_layer() const;

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

_id ?

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+  bool droppable() const;
+  bool layer_sync() const;
+
+  // Setters for properties.
+  void MarkKeyFrame();

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

s/Mark/Set/ here and below.
Although since they're simple setters, set_ is probably more correct.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

I'm taking these all down for benefit of the EncodingMetadata in the
constructor. Based on the feedback I'm making this class more constant to be
read-only after creation to enforce the thread-safety. These are anyway known
parameters at the time of encoding and should not be required after
initialization.

+  void MarkAltRef();
+  void MarkGolden();

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

These make this look like an EncodedVP8BitstreamBuffer, not simply an
EncodedBitstreamBuffer.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

That's right. In fact I had EncodedBitstreamBuffer and specialization for VP8,
VP8EncodedBitstreamBuffer, earlier but I decided to merge them as I had not seen
use case/client for H264 encoder in Chrome. All the code happening for WebRTC
right now is exclusively VP8.

Do you see H264 encoder coming up and how would structure this for generic
support w.r.t IPC and EncodedBitstreamBuffers?

+  void MarkDroppable();
+  void MarkLayerSync();

[holmer, 2013/03/04 13:36:40]

Can you comment on these properties? In particular, does MarkAltRef() mean that
this frame updates the AltRef reference buffer, or does it mean that it
references it? Do we need to be able to specify both?

[Ville-Mikko Rautio, 2013/03/05 13:02:18]

I thought these with "is-a" semantics. Does WebRTC need the reference
information after encoding?

And, as told in comments above, I'll remove Mark* functions and clarify the
"is-a" semantics.

[holmer, 2013/03/05 13:32:54]

But what does it mean that it is an AltRef frame? Does it mean that it
references the AltRef buffer, that it updates the AltRef buffer or both? And
does it mean that it only updates/references the AltRef buffer? Maybe there's a
definition in the vp8 spec of what it means if a frame "is an" AltRef, but it's
not clear to me right now.

On 2013/03/05 13:02:18, Ville-Mikko Rautio wrote:

[vmr, 2013/03/06 05:51:29]

Sorry, I should have been clearer.

Intention is that the buffer with altref frame flag set updates the alternate
reference buffer and is the altref reference for future frames. It does not tell
anything about the referencing structure used to encode the current frame.

My follow up question about the referencing structure was that would webrtc use
information about exact referencing structure (in addition to temporal layer id
plus layer sync and droppable flags)?

[holmer, 2013/03/07 14:20:32]

Thanks for clarifying.

WebRTC doesn't use it explicitly (don't even think it's possible for you to pass
this into WebRTC?). We may use it implicitly though by letting the decoder parse
that information out of the bitstream and then use it for signaling SLI and/or
RPSI messages. That is not currently in use though. Is that an answer to your
question?

I guess it's still not clear to me what the MarkAltRef() and MarkGolden() will
be used for, but now I at least know what they mean. :)

On 2013/03/06 05:51:29, vmr wrote:

+
+ protected:
+  virtual ~EncodedBitstreamBuffer(); // Destructor. Will deallocate the buffers.

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

Comment does not add value.
~ means destructor
scoped_ptr<> means |this| owns the member.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+  friend class base::RefCountedThreadSafe<EncodedBitstreamBuffer>;
+
+ private:
+  scoped_ptr<uint8[]> buffer_;
+  size_t size_;
+  base::Time timestamp_;
+  bool key_frame_;
+  int temporal_layer_;
+  bool altref_frame_;
+  bool golden_frame_;
+  // Determines whether the buffer is droppable. With droppable buffers decoding
+  // can be skipped without affecting the decoding of subsequent buffers.
+  bool droppable_;
+  // For temporal base layer, the field must be set to 0 only if it only depends
+  // on previous key frame. For temporal enhancement layers the layer sync must
+  // be set to true, if the buffer depends only on the previous temporal base
+  // layer buffer.
+  bool layer_sync_;
+  DISALLOW_COPY_AND_ASSIGN(EncodedBitstreamBuffer);

[Ami GONE FROM CHROMIUM, 2013/03/02 10:47:16]

Add an empty line before this one?

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

Done.

+};
+
+}  // namespace media
+
+// Pretty print operator for the EncodedBitstreamBuffer.
+std::ostream& operator<<(std::ostream& output,

[tommi (sloooow) - chröme, 2013/03/01 16:03:43]

although chromium uses streams for logging, I believe it is generally
discouraged/disallowed by the style guide.

[Ville-Mikko Rautio, 2013/03/04 14:02:25]

I know. I just really want to have a way to log these buffers in a uniform way
to ease up troubleshooting the problems we will undoubtedly face with the
production code. Timing dependent protocol issues will be nightmarish to debug
unless we can get meaningful logs from failing runs. Would ToString() member
function work better for such purpose?

+                         const media::EncodedBitstreamBuffer& b);
+
+#endif  // MEDIA_BASE_ENCODED_BITSTREAM_BUFFER_H_