Define EncodedVideoSource and RtcCapturedEncodingVideoCapturer.

media/video/encoded_video_source.h

Index: media/video/encoded_video_source.h
diff --git a/media/video/encoded_video_source.h b/media/video/encoded_video_source.h
new file mode 100644
index 0000000000000000000000000000000000000000..7be3d6b1f65fd8ed31048dcc1de0fccd0db8ae3b
--- /dev/null
+++ b/media/video/encoded_video_source.h
@@ -0,0 +1,111 @@
+// 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_

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

TODO+crbug(m30) for merging this into VEA

[hshi1, 2013/06/11 00:48:20]

Done.

+#define MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
+
+#include "base/memory/ref_counted.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/06/08 00:18:01]

o rly?

[hshi1, 2013/06/11 19:51:22]

Proposed change: remove the entire EVB definition. Move EVB::Client into
EVS::Client.

+class MEDIA_EXPORT EncodedVideoBitstream :

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

Is it not the case that bitstreams are owned by sources?  As such I'd expect to
see this as an inner class of EVS since these don't make sense by themselves.

+    public base::RefCountedThreadSafe<EncodedVideoBitstream> {
+ public:
+  class MEDIA_EXPORT Client {
+   public:
+  // After OnStreaming callback stream shall be considered to be streaming.

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

Oh, shall it??

Can you make a pass and cull useless comments and add useful ones where mystery
might exist?

[hshi1, 2013/06/11 19:51:22]

Got rid of the EVB altogether.

+  virtual void OnBitstreamStreaming(

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

Drop "Bitstream" from the name of each of the functions in this interface?

[hshi1, 2013/06/11 00:48:20]

Made the following changes:
OnBitstreamStreaming -> OnStreaming
OnBitstreamClosed -> OnClosed
OnBitstreamReady -> OnBufferReady
OnBitstreamConfigChanged -> OnConfigChanged

+      scoped_refptr<EncodedVideoBitstream> bitstream,

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

The |bitstream| param to each of these methods is not actually needed (viz the
impl doesn't use them).

[hshi1, 2013/06/11 00:48:20]

You're correct; removed.

+      const VideoEncodingParameters& params) = 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(

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

s/Bitstream/Buffer/

[hshi1, 2013/06/11 00:48:20]

Done.

+      scoped_refptr<EncodedVideoBitstream> bitstream,
+      scoped_refptr<const EncodedBitstreamBuffer> buffer) = 0;
+
+  // OnBitstreamConfigChanged informs about change in bitstream parameters.
+  virtual void OnBitstreamConfigChanged(
+      scoped_refptr<EncodedVideoBitstream> bitstream,
+      const RuntimeVideoEncodingParameters& params) = 0;
+  };
+
+  // TrySetBitstreamConfig issues a reconfiguration request. Old configuration

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

comment out of date (and tortured)

[hshi1, 2013/06/11 00:48:20]

Done.

+  // 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;
+
+ protected:

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

Here and elsewhere you seem to think that RCTS<> requires dtor & friend decl to
be "protected" but actually they can be private instead.

[hshi1, 2013/06/11 19:51:22]

N/A (removed).

+  virtual ~EncodedVideoBitstream() {};
+  friend class base::RefCountedThreadSafe<EncodedVideoBitstream>;
+};
+
+// Class to represent any encoded video source. Anything that provides encoded
+// video can be an EncodedVideoSource. Notable examples of this can be video
+// encoder and webcam that has encoding capability.
+class MEDIA_EXPORT EncodedVideoSource {

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

Is there no need to report errors other than OnBitstreamRemoved?

[hshi1, 2013/06/11 00:48:20]

Currently no; do you think I should add a separate OnBitstreamError IPC, or
re-use OnBitstreamClosed() IPC with an error code?

+ public:
+
+  class Observer {
+   public:
+    // Invoked when an encoded video source's capabilities become available.
+    virtual void OnCapabilitiesAvailable(EncodedVideoSource* source) = 0;

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

Single-method interfaces are better served by a callback parameter to the
relevant method, which leaves implementors more freedom for how to implement.

[hshi1, 2013/06/11 00:48:20]

Removed this caps observer business entirely (see comment below also).

+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  virtual ~EncodedVideoSource() {};
+
+  // Adds/removes observer to receive OnCapabilitiesAvailable notification.
+  virtual void AddCapabilitiesObserver(Observer* observer) = 0;
+  virtual void RemoveCapabilitiesObserver(Observer* observer) = 0;
+
+  // StartFetchCapabilities initiates an asynchronous query for the types of
+  // encoded bitstream supported by the encoder. When the results become
+  // available, EncodedVideoSource will notify observers via the
+  // OnCapabilitiesAvailable() method.
+  virtual void StartFetchCapabilities() = 0;
+
+  // 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;

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

Observer, {Add,Remove}CapabilitiesObserver, StartFetchCapabilities, and
GetCapabilities() is a pretty baroque set of stuff that could all be replaced
with a single:

virtual void RequestCapabilities(const
base::Callback<void(VideoEncodingCapability)>& capability_cb) = 0;

don't you think?

[hshi1, 2013/06/11 00:48:20]

Thanks for the suggestion, I've removed the observer interface and added
RequestCapabilities. There is one minor snag though, namely the EVS impl can't
actually start sending the IPC for caps until capture has started (at which
point the device_id is actually registered on the render_host side). But the new
CL I've uploaded seems to work correctly.

+
+  // 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.

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

Again, this comment is craxy.

[hshi1, 2013/06/11 19:51:22]

Now that EVB is gone, I've simplified this to be a void function. Only one
bitstream can be opened per EVS.

+  virtual scoped_refptr<EncodedVideoBitstream> OpenBitstream(

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

This API was built to support the notion of multiple clients consuming the same
output of an encoding camera.  I suspect you could cut the amount of code in
this CL significantly by making it so that a VideoCapturer is limited to feeding
a single bitstream.

[hshi1, 2013/06/11 00:48:20]

Talked to sheu@. I'm removing all stream_id and assume that each device can only
have one bitstream.

+      EncodedVideoBitstream::Client* client,

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

doco lifecycle anytime ownership is not clarified by use of descriptive ptr
type.

+      const VideoEncodingParameters& params) = 0;
+  // CloseBitstream removes the bitstream.
+  virtual void CloseBitstream(
+      scoped_refptr<media::EncodedVideoBitstream> bitstream) = 0;
+
+  // ReturnBitstreamBuffer notifies that the data within the buffer has been
+  // processed and it can be reused to encode upcoming bitstream.
+  virtual void ReturnBitstreamBuffer(

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

It's craxy that buffers are handed to EVB::Client but are returned by EVS (not
EVB).

Looks like your model is that the EVS (browser process) creates & owns shared
memory buffers and the client (renderer process) notifies when they're no longer
needed.
This differs from VDA & VEA where the {de,en}coder requests buffers from the
client and the client ultimately owns them.
Might be worth calling out in a NOTE comment since this will require redoing
when dropping EVS for VEA.

[hshi1, 2013/06/11 19:51:22]

EVB is now gone and EVB::Client is moved to become EVS::Client.

+      scoped_refptr<media::EncodedVideoBitstream> bitstream,
+      scoped_refptr<const media::EncodedBitstreamBuffer> buffer) = 0;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
+