Interfaces for encoded video sources

media/video/encoded_video_source.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_VIDEO_ENCODED_VIDEO_SOURCE_H_
+#define MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "media/base/encoded_bitstream_buffer.h"
+#include "media/video/video_encode_types.h"
+
+namespace media {
+
+// Class to represent encoded video bitstream.

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

Please make an editorial pass on the comments in this file. 
(there's a fair bit of obvious things stated, and a fair bit of information
omitted)

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

Done.

+class MEDIA_EXPORT EncodedVideoBitstream :
+    public base::RefCountedThreadSafe<EncodedVideoBitstream> {

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

same class level comments as for the previous class.

+ public:
+  class MEDIA_EXPORT Client {
+   public:
+    // After OnStreaming callback stream shall be considered to be streaming.
+    virtual void OnBitstreamStreaming(
+        scoped_refptr<EncodedVideoBitstream> bitstream, bool modifiable) = 0;
+
+    // After OnRemoved client has to stop using the bitstream object and
+    // expecting bitstream buffers for that stream. OnRemoved will be called
+    // also in case of any unrecoverable failure in the capturer. After
+    // OnRemoved is called from a stream it is guaranteed that that there will
+    // be no longer pending calls coming in.
+    virtual void OnBitstreamRemoved(
+        scoped_refptr<EncodedVideoBitstream> bitstream) = 0;
+
+    // OnBitstreamReady delivers the captured bitstream buffer by buffer to the
+    // client.
+    virtual void OnBitstreamReady(
+        scoped_refptr<EncodedVideoBitstream> bitstream,
+        scoped_refptr<const EncodedBitstreamBuffer> buffer) = 0;
+
+    // OnBitstreamConfigChanged informs about change in bitstream parameters.
+    virtual void OnBitstreamConfigChanged(

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

nit: All the methods in this class have the word "Bitstream" in them. Would it
make sense to name the class BitstreamClient or BitstreamObserver and change the
method names: OnBitstreamStreaming->OnStreaming etc?

I don't really mind the way it is now but just reading through the methods
sounds a bit redundant.

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

Done.

+        scoped_refptr<EncodedVideoBitstream> bitstream,
+        const RuntimeVideoEncodingParameters& params) = 0;
+  };
+
+  // TryConfigure issues a reconfiguration request. Old configuration must be
+  // considered to be the valid configuration until OnBitstreamConfigChanged
+  // callback has been issued with value signalling successful change.
+  virtual void TryConfigure(
+      const RuntimeVideoEncodingParameters& params) = 0;
+
+  // RequestSpecialFrame allows the client to request special frames from the
+  // encoded video bitstream. The effect of the request is only visible in the
+  // bitstream buffers passed to client through the OnBitstreamReady callback.
+  // This request is served on best-effort basis and client is not given any
+  // guarantees of the realization or timing of the request. Flags parameters
+  // will be interpreted in format specific manner using enumerations.
+  virtual void RequestSpecialFrame(int flags) = 0;
+
+ protected:
+  virtual ~EncodedVideoBitstream() {};
+  friend class base::RefCountedThreadSafe<EncodedVideoBitstream>;
+};

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

Like I wrote in the _messages.h review, this interface seems to be lacking some
significant API surface such as Encode(VideoFrame) and lifecycle management ;)

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

I added fair amount of comments about this. Trying to emphasize that this is not
complete encoder interface... it is more of interface to configure and handle
output from video source that has encoding capability.

+
+// Class to represent any encoded video source. Anything that provides encoded
+// video can be an EncodedVideoSource. Notable examples of this can be video
+// encoder (duh!) and webcam that has encoding capability.
+class MEDIA_EXPORT EncodedVideoSource {
+ public:
+  virtual ~EncodedVideoSource() {};
+
+  // GetCapabilities informs the client what type of encoded bitstream encoder
+  // is capable to provide. Constraints in the reported capabilities should be
+  // obeyed when configuring bitstreams.
+  virtual VideoEncodingCapability GetCapabilities() = 0;
+
+  // OpenBitstream returns object for the stream being added. Client must
+  // consider the returned stream valid until OnBitstreamRemoved callback is
+  // called with the id. Client should check the parameters agains limits
+  // reported by GetCapabilities before trying to issue an request to add an
+  // encoded bitstream. EncodedVideoSource retains the ownership of
+  // EncodedVideoBitstream and client just has the pointer to it.
+  virtual scoped_refptr<EncodedVideoBitstream> OpenBitstream(
+      base::WeakPtr<EncodedVideoBitstream::Client> client,

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

Putting WeakPtr in the API imposes threading constraints.  Please be explicit
about expected use patterns in class-level comments.

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

I added some comments on this, but left also TODO to think about the WeakPtr
through again from threading perspective.

+      const VideoEncodingParameters& params) = 0;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
+