Add still image capture interface for VideoCaptureDevice.

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 382dad6cdc461d7d6d20bf04ab8e7e91693ae503..5e05ca90581e526753516e4cfd07a67f546bae05 100644
--- a/media/video/capture/video_capture_device.h
+++ b/media/video/capture/video_capture_device.h
@@ -216,6 +216,40 @@ class MEDIA_EXPORT VideoCaptureDevice {
 
     // VideoCaptureDevice requests the |message| to be logged.
     virtual void OnLog(const std::string& message) {}
+
+    // The video stream has been muted. After this callback, no more
+    // OnIncomingCapturedData() will be called. This may happen when
+    // CaptureImage() has called. After the still image captured, the client

[Pawel Osciak, 2014/09/23 08:15:57]

s/captured/is captured/

So the design is that we can't capture a still image without having the device
streaming? Why this limitation?

[Pawel Osciak, 2014/09/23 08:15:58]

s/has called/has been called/

[Owen Lin, 2014/09/24 05:20:03]

1. Same as the JS interface. 
2. We need preview running before capturing an still image to adjust white
balance and exposure.

[Pawel Osciak, 2014/09/25 02:16:20]

We should tailor APIs to features/devices they expose, not to limitations of
some of the potential clients that have no relation to the entity this API
exposes.

Note that this is different from adding additional features to an API that
wouldn't be used by clients. In that case we could not implement unused
features, and potentially add them later as needed. But this is instead actually
making the API more limited/complicated, because of some clients, who can still
use it without this limitation. And impossible to extend later.

And VideoCaptureDevice is also not a JS-specific interface and JS is not the
only client.
Sure, but we don't know for how long anyway. And the hardware will normally do
this before capture automatically anyway. And it will do it for the exact time
needed and with minimal delay for the given situation and sensor.

With this, you instead force the client to turn on capture for an unspecified
time, and then take a guess for how long it has to run, which will in practice
always be too long anyway - before it can capture.

[Owen Lin, 2014/09/25 05:53:53]

Sorry, I still prefer the current design. Let me try to convinced you about
that.

We need the preview in most cases, if client don't want the preview, it can just
hide it. This will simplify the design and meet for most cases (I mean more than
99% use cases) I don't think we need to handle the complexity (consider if
preview has been start). 

On 2014/09/25 02:16:20, Pawel Osciak wrote:
My point is they design it in this way for reasons. I would believe they are
experienced experts. 

I believe it is the bridge from Chrome to the Camera devices. We don't have to
expose all features of Camera but only those will be used by Chrome.
It is limited but not simpler (I don't consider the case if the stream is
originally on or off). I agree there is some Camera (SLR) can take picture
without turn on the stream. But it won't be all of them on Chrome. I don't think
Chrome would take that into design in the future.
Before I design this API, I have asked kcwu, he said the first few frames from
camera are not looking good.

Besides, when user click on the button to take a picture. They want to take it
ASAP.

[Pawel Osciak, 2014/09/29 00:13:36]

My suggestion would not be adding new features. The current one however is
adding additional constraints, making the API more complicated to use.
Perhaps it's simpler for the implementation of the capturer, but not for the
client of the API, which now has to start streaming before it can capture and
then capture, instead of just capturing. Also, the client has to worry for how
long it has to be streaming before it can capture a still image (given that you
added this requirement to give the device time to focus, 3A, etc.).
That's just one camera, but of course others can work in a similar way.
That said, how do you guarantee that the client will stream long enough first,
that they would start looking good?
 
Well, requiring them to run streaming first for an unspecified amount of time
and then capture is not ASAP...

+    // will get notified by OnUnmute() and the video stream will be resumed.
+    virtual void OnMute() {}
+
+    // The video stream has resumed.
+    virtual void OnUnmute() {}
+  };
+
+  class MEDIA_EXPORT ImageClient {

[Pawel Osciak, 2014/09/23 08:15:57]

Please add documentation what this class is for.

[Owen Lin, 2014/09/24 05:20:02]

Done.

+   public:
+    virtual ~ImageClient() {}
+
+    // Callback function to notify the client the captured image is available.

[Pawel Osciak, 2014/09/23 08:15:57]

s/the/a/

[Owen Lin, 2014/09/24 05:20:03]

Done.

+    //
+    // The captured still image is stored at address |data| and of |length|

[Pawel Osciak, 2014/09/23 08:15:57]

s/of/is of/

[Owen Lin, 2014/09/24 05:20:02]

Done.

+    // bytes. The format of the frame is described by |format|, and is assumed
+    // to be tightly/ packed. The still image should be rotated for |rotation|

[wuchengli, 2014/09/23 07:09:22]

remove extra / after tightly

[Pawel Osciak, 2014/09/23 08:15:57]

s/should/is/ I think?
s/for//

[Owen Lin, 2014/09/24 05:20:02]

Done.

[Owen Lin, 2014/09/24 05:20:03]

There is no rotation information in the raw data. The rotation is actaully the
device's rotation degree. I think using "should" is correct here.

[Pawel Osciak, 2014/09/25 02:16:20]

So you mean the image doesn't contain EXIF information about rotation, but we
use the device orientation data instead? That's still valid information. In what
case would it be invalid?

[Owen Lin, 2014/09/25 05:53:53]

It's YUV so there is no rotation info. The rotation should be encoded to EXIF. I
say "should" because it is not in that way now. But we should do that. 

[Pawel Osciak, 2014/09/29 00:13:37]

Sorry, what should we do? If there is no rotation info, what is this argument
for?

+    // degrees clockwise for viewing.
+    //
+    // Note that the content in |data| would be freed after this callback.

[Pawel Osciak, 2014/09/23 08:15:57]

s/would be freed/will not be valid/
s/callback/callback returns/

[Owen Lin, 2014/09/24 05:20:02]

Done.

+    // Copy the content to use it later.
+    virtual void OnIncomingCapturedData(const uint8* data,
+                                        int length,

[Pawel Osciak, 2014/09/23 08:15:57]

s/int/size_t/

[Owen Lin, 2014/09/24 05:20:02]

For consistency with Client::OnIncomingCaptureData. I can fix it later.

[Pawel Osciak, 2014/09/25 02:16:21]

I don't believe we need this consistency. We have to start fixing things from
somewhere, and it's better to fix in new APIs before we have any users of them,
than to fix later when we already have users and have to change them as well.

[Owen Lin, 2014/09/25 05:53:53]

Done.

+                                        const ImageCaptureFormat& format,
+                                        int rotation,  // Clockwise

[Pawel Osciak, 2014/09/23 08:15:57]

Perhaps remove the comment, it's already in the doc above.

[Owen Lin, 2014/09/24 05:20:03]

Done.

+                                        base::TimeTicks timestamp) = 0;
+
+    // Callback function to notify client the failure of the image capture.

[Pawel Osciak, 2014/09/23 08:15:57]

s/client/the client/
s/the failure/about a failure/

[Owen Lin, 2014/09/24 05:20:03]

Done.

+    // The VideoCaptureDevice must be StopAndDeAllocate()-ed. |reason| is a

[Pawel Osciak, 2014/09/23 08:15:57]

s/is/contains/

[Owen Lin, 2014/09/24 05:20:03]

Done.

+    // text description of the error.
+    virtual void OnError(const std::string& reason) = 0;
   };
 
   // Creates a VideoCaptureDevice object.
@@ -235,6 +269,12 @@ class MEDIA_EXPORT VideoCaptureDevice {
   static void GetDeviceSupportedFormats(const Name& device,
                                         VideoCaptureFormats* supported_formats);
 
+  // Gets the supported formats for still image of a particular device attached

[Pawel Osciak, 2014/09/23 08:15:56]

Gets formats for still image capture supported by a particular...

s/device/|device|/

[Owen Lin, 2014/09/24 05:20:03]

Done.

+  // to the system.
+  static void GetDeviceSupportedImageFormats(
+      const Name& device,
+      ImageCaptureFormats* supported_formats);
+
   // Prepares the camera for use. After this function has been called no other
   // applications can use the camera. StopAndDeAllocate() must be called before
   // the object is deleted.
@@ -258,6 +298,30 @@ class MEDIA_EXPORT VideoCaptureDevice {
   // defined, otherwise returns 0.
   int GetPowerLineFrequencyForLocation() const;
 
+  // Initializes the device for still image capture for the given image format.
+  //
+  // This function must be called between AllocateAndStart() and
+  // StopAndDeAllocate().

[Pawel Osciak, 2014/09/23 08:15:57]

Why do we have this requirement?

[Owen Lin, 2014/09/24 05:20:02]

We need the preview on before taking a picture to adjust white balance and
exposure.

[Pawel Osciak, 2014/09/29 00:13:36]

What will happen if it's called at a wrong time (in terms of API), error,
callback, etc.?

+  virtual void InitializeImageCapture(const ImageCaptureFormat& image_format,

[Pawel Osciak, 2014/09/23 08:15:57]

I think we need to have a bool return in case this fails?

[Owen Lin, 2014/09/24 05:20:03]

Fail will be reported by ImageClient::onError().

[Pawel Osciak, 2014/09/25 02:16:20]

I'm assuming you need Initialize to be asynchronous? You need OnInitialized
then. Otherwise how does the client know the Initialization is done and it can
request capture? If it Initializes and calls CaptureImage, how does it know if
it got an OnError for Initialize or Capture? How does it know if it needs to
expect another OnError for Capture?

[Owen Lin, 2014/09/25 05:53:53]

Good point. Let me make this synchronous.

+                                      scoped_ptr<ImageClient> client) {}
+
+  // Releases resources for image capture.
+  //
+  // The ImageClient passed from InitializeImageCapture would be freed. This

[Pawel Osciak, 2014/09/23 08:15:57]

s/would/will/

+  // method must be called between InitializeImageCapture() and

[Pawel Osciak, 2014/09/29 00:13:37]

What will happen if it's called at a wrong time (in terms of API), error,
callback, etc.?

+  // StopAndDeAllocate().
+  virtual void ReleaseImageCapture() {}
+
+  // Gets one image from the device asynchronously.

[Pawel Osciak, 2014/09/23 08:15:57]

Asynchronously to what?

[Owen Lin, 2014/09/24 05:20:03]

Did I misuse it ? Asynchronously to this function. Client will get the image
after this function returns.

[Pawel Osciak, 2014/09/25 02:16:20]

You should probably say Request instead of Get.

[Owen Lin, 2014/09/25 05:53:53]

Thanks. 

+  //
+  // It will return the image by the ImageClient::OnIncomingCapturedData()

[Pawel Osciak, 2014/09/23 08:15:56]

s/It will return the image by/The image will be returned via/

[Owen Lin, 2014/09/25 05:53:53]

Done.

+  // callback. If the video stream has to be stopped to capture the still image,
+  // the Client::OnMute() and Client::OnUnmute() will be called.
+  //
+  // This function must be called between InitializeImageCapture() and

[Pawel Osciak, 2014/09/23 08:15:57]

Why this requirement?

[Owen Lin, 2014/09/24 05:20:03]

It's the life time of the image_client instance. 

[Pawel Osciak, 2014/09/29 00:13:36]

What will happen if that's not the case (in terms of API), error, callback,
etc.?

+  // ReleaseImageCapture().
+  virtual void CaptureImage() {}
+
  protected:
   static const int kPowerLine50Hz = 50;
   static const int kPowerLine60Hz = 60;