Add still image capture interface for VideoCaptureDevice.

media/video/capture/video_capture_device.h

 // Copyright (c) 2012 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.
 //
 // VideoCaptureDevice is the abstract base class for realizing video capture
 // device support in Chromium. It provides the interface for OS dependent
 // implementations.
 // The class is created and functions are invoked on a thread owned by
 // VideoCaptureManager. Capturing is done on other threads, depending on the OS
 // specific implementation.
@@ skipping 198 matching lines @@
         const VideoCaptureFormat& buffer_format,
         const scoped_refptr<media::VideoFrame>& frame,
         base::TimeTicks timestamp) = 0;
 
     // An error has occurred that cannot be handled and VideoCaptureDevice must
     // be StopAndDeAllocate()-ed. |reason| is a text description of the error.
     virtual void OnError(const std::string& reason) = 0;
 
     // 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.
   // Return NULL if the hardware is not available.
   static VideoCaptureDevice* Create(
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
       const Name& device_name);
   virtual ~VideoCaptureDevice();
 
   // Gets the names of all video capture devices connected to this computer.
   static void GetDeviceNames(Names* device_names);
 
   // Gets the supported formats of a particular device attached to the system.
   // This method should be called before allocating or starting a device. In
   // case format enumeration is not supported, or there was a problem, the
   // formats array will be empty.
   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.
   virtual void AllocateAndStart(const VideoCaptureParams& params,
                                 scoped_ptr<Client> client) = 0;
 
   // Deallocates the camera, possibly asynchronously.
   //
   // This call requires the device to do the following things, eventually: put
   // camera hardware into a state where other applications could use it, free
   // the memory associated with capture, and delete the |client| pointer passed
   // into AllocateAndStart.
   //
   // If deallocation is done asynchronously, then the device implementation must
   // ensure that a subsequent AllocateAndStart() operation targeting the same ID
   // would be sequenced through the same task runner, so that deallocation
   // happens first.
   virtual void StopAndDeAllocate() = 0;
 
   // Gets the power line frequency from the current system time zone if this is
   // 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;
 };
 
 }  // namespace media
 
 #endif  // MEDIA_VIDEO_CAPTURE_VIDEO_CAPTURE_DEVICE_H_