Reorganize media::VideoCapture* types

media/video/capture/video_capture_device.h

Index: media/video/capture/video_capture_device.h
diff --git a/media/video/capture/video_capture_device.h b/media/video/capture/video_capture_device.h
index 74a8d27d85db32c017165c893a5dc2687f038b2b..c4930be1144f1dc2e92dd7ebcee43717d06ebc16 100644
--- a/media/video/capture/video_capture_device.h
+++ b/media/video/capture/video_capture_device.h
@@ -20,6 +20,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "media/base/media_export.h"
+#include "media/base/video_frame.h"
 #include "media/video/capture/video_capture_types.h"
 
 namespace media {
@@ -172,7 +173,7 @@ class MEDIA_EXPORT VideoCaptureDevice {
         int rotation,  // Clockwise.
         bool flip_vert,
         bool flip_horiz,
-        const VideoCaptureCapability& frame_info) = 0;
+        const VideoCaptureFormat& frame_format) = 0;

[ncarter (slow), 2013/11/19 00:14:23]

In my mind, this was one boundary where passing a pixel format was necessary
(and that's why I thought Capability instead of Format made sense here
historically). You've addressed this by adding a pixel format to Format, and
switching away from Capabilities. That will allow Capabilities to evolve to
support the needs of GetDeviceSupportedFormats, which as far as I can tell is
the last use of VideoCaptureCapability -- e.g. it could evolve to support min
and max.

However, the addition of a pixel format field in VideoCaptureFormat means that
now there will be pixel formats in contexts where previously things were assumed
to be I420.

Here at the (Device -> OnIncomingCapturedFrame) boundary we have a much larger
range of supported formats than we will ever want to support at the IPC (Browser
-> Renderer) boundary. In my mind the two acceptable formats for the IPC
boundary are: shared-memory I420, and gpu-resident ARGB. And yet both spots are
tied to use the same type, VideoCaptureFormat.

In other words, by adding a pixel_format to Format, you wind up introducing a
parameter that will be ignored in a whole bunch of contexts. I think this is
appropriate only if you expect that the pixel_format parameter will eventually
come to be meaningful in all or most the following contexts:

1. VideoCaptureCapabilities returned by GetDeviceSupportedFormats()
2. VideoCaptureParams requested by the renderer when starting a stream.
3. VideoCaptureFormats returned by the browser upon delivery of a frame.

I'm skeptical that we want to involve pixel formats in cases [1] and [2]. Today
no camera needs outside advice in determining which capture format should be
used -- it simply will capture in the most efficiently obtained format, fully
knowing that the downstream stack is going to want I420 eventually.

I am not hard set on this (I'm open to being swayed otherwise), but my instinct
is probably to do the following:
 - VideoCaptureCapability should not have a pixel format
 - VideoCaptureParams should not have a pixel format
 - VideoCaptureFormat should not have a pixel format
 - Pass a pixelformat at this boundary, either as an explicit param or else as a
new struct defined in this file (VideoCaptureDeviceFormat ?)

[sheu, 2013/11/19 00:56:13]

I'm inclined to start letting users of the captured buffer specify what format
they want.  In this model the Browser/Renderer negotiation just reports
supported formats to the user, and forwards the users's request.

So yes, there will be more formats at the IPC boundary.  The two immediate ones
are as you mentioned -- shared-memory I420, and gpu-resident ARGB.  It might be
possible that we would want later shared-memory ARGB as well.

This would also have the benefit of allowing us to look at moving the various
format conversions out of the browser process into the individual renderer,
where arguably it belongs.  There are a few points that I see can be raised with
regards to this:

* If different renderers both request the same conversion, then we could
potentially be doing extra work.  I don't see this to be more than a corner case
though.
* Buffers passed from browser to renderer might become larger (i.e. ARGB vs.
I420).  However we're passing around shared-memory segments, not copying them,
so I don't think this is an issue.  Those large buffers have to exist somewhere
in the existing pipe anyways.
[1] and [2] are the reasons that I moved the pixel format in.  I'm envisioning a
case where the renderer side will request either a software buffer, or a
hardware buffer depending what the renderer knows will be required of the
buffer.  If there is no CPU access to be done on the buffer, then the pipeline
can be optimized by requesting a hardware buffer.
If [1] and [2] are taken to require pixel formats, then it does make sense to
have pixel format in all three.

[ncarter (slow), 2013/11/19 01:22:42]

By "user" above, I understand you're referring to one of the codec consumers
(today we have two, the Rtc consumer and the PPAPI consumer?). In what
circumstances do you expect these consumers to want anything other than
"whatever's the fastest way to get to I420 at the end of the day"?

Suppose we've plumbed things as you've described, and now the WebRTC encoder (or
some JS running on the page) is told that the camera supports capturing at X
resolution in either MJPEG or NV16. How on earth would this consumer go about
deciding between the two? I don't see these consumers as being in a position to
do anything with this information, and for that reason, it makes sense to hide
the information from the consumer layer.
I agree, this is a corner case.
This is incorrect, it is not a wash. The memory bandwidth demands of the memcpy
into and out of the shared memory buffer are 3x higher if the conversion happens
in the renderer. ARGB is 3x the size of I420. This will particularly hurt on
mobile.
I think the hardware/software buffer distinction is worth adding some signaling
for (e.g., a hardware-buffer-capable-camera needs to know that it's not talking
to a PPAPI consumer that can't use the hardware encode). But I think the needs
for that signaling are different from the needs at the VCD / VCDC interface,
where we support a whole kaleidoscope (or colitiscope, depending on your view of
libyuv) of formats that the rest of the pipeline, including the consumers,
shouldn't have to handle in their switch statements.

[sheu, 2013/11/19 03:26:22]

I'd certainly hide this from JS, but I wouldn't necessarily hide this from the
WebRTC implementation in Chrome, for its own pipeline optimization purposes.
Exposing more formats output from the capture lets us dispense with a
unnecessary conversions in the pipeline.  For example, Hangouts embellishments
and effects hooked up through libjingle internally takes the I420 frame we give
it from the capturer and does a reconversion to ARGB to do video enhancement,
party hats, and the like.  (Other effects run on YUV, so YMMV.)  An
unconditional ARGB->YUV conversion in the browser doesn't make sense here.
Mostly the pipeline should just pass the data along, and it's the consumers that
should be dealing with the switch statements.  Which already is the case,
actually, since libjingle takes multiple formats and does the appropriate
conversion already.

[ncarter (slow), 2013/11/19 18:54:24]

You've convinced me that it'll be okay to put a pixel format in the
VideoCaptureFormat.

 
     // Captured a new video frame, held in |buffer|.
     //
@@ -208,12 +209,10 @@ class MEDIA_EXPORT VideoCaptureDevice {
                                         VideoCaptureCapabilities* formats);
 
   // Prepare the camera for use. After this function has been called no other
-  // applications can use the camera. On completion Client::OnFrameInfo()
-  // is called informing of the resulting resolution and frame rate.
-  // StopAndDeAllocate() must be called before the object is deleted.
-  virtual void AllocateAndStart(
-      const VideoCaptureCapability& capture_format,
-      scoped_ptr<Client> client) = 0;
+  // applications can use the camera. StopAndDeAllocate() must be called before
+  // the object is deleted.
+  virtual void AllocateAndStart(const VideoCaptureParams& params,

[ncarter (slow), 2013/11/19 00:14:23]

This function should not receive a session_id. I think it makes sense for this
to be a Format.

[sheu, 2013/11/19 00:56:13]

I think this is about the same situation as
VideoCaptureImpl::DoStartCaptureOnCaptureThread().  We're making a request for
the device to start.

If we were to change to a Format, we'd have to make allow_resolution_change an
explicit parameter to this entry point as well.  Thoughts?

[ncarter (slow), 2013/11/19 18:54:24]

I feel strongly that there should not be any session id's visible to the device.
This is because of my personal experience; when I first tried to understand this
code, I found it deeply confusing that many classes had a dead or unused
session_id field. Only the Name should identify the device.

Can't this be structured differently so that we get the information we expect to
act on, but not the session_id? I think passing the allow_resolution_change
parameter here seems fine, as does inventing an additional struct type, as does
removing session_id from params and making it an explicit IPC and function
parameter where it's needed.

[sheu, 2013/11/20 20:35:45]

Since this has come up before, I'd opt to remove session_id from params.  Going
to do that.

+                                scoped_ptr<Client> client) = 0;
 
   // Deallocates the camera, possibly asynchronously.
   //