Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545053002:
Add still image capture interface for VideoCaptureDevice. (Closed)
|
Created:
6 years, 3 months ago by Owen Lin Modified:
6 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd still image capture interface for VideoCaptureDevice.
BUG=413086
TEST=Run the media_unittests on peach_pit.
Committed: https://crrev.com/fd5bdd2688768683e30b27941f181597a849656d
Cr-Commit-Position: refs/heads/master@{#298009}
Patch Set 1 : #
Total comments: 28
Patch Set 2 : #
Total comments: 13
Patch Set 3 : Address reviewer's comments #
Total comments: 9
Patch Set 4 : #
Total comments: 30
Patch Set 5 : #Patch Set 6 : address review comments #
Total comments: 51
Patch Set 7 : address review comment. #
Total comments: 1
Patch Set 8 : #Patch Set 9 : rebase #Patch Set 10 : fix compiling errors #
Messages
Total messages: 34 (7 generated)
Patchset #1 (id:1) has been deleted
owenlin@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL.
wuchengli@chromium.org changed reviewers: + jchuang@chromium.org
Add BUG number. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.cc:82: client->OnError("Not supported"); The client can know if image capture is supported by GetDeviceSupportedImageFormats. I think we can just have an empty implementation like CloseImageCapture and CaptureImage. If you want to call OnError, the other two functions should call OnError, too. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:220: // The video stream is being muted. After this callback, no more s/is being/has/ https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:221: // OnIncomingCapturedData() will be called. Explain OnMute and OnUnmute are called when image capture is started and ended is called. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:224: // The video stream is being resumed. s/is being/has/ https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:240: // The VideoCaptureDevice must be StopAndDeAllocate()-ed. |reason| is a s/StopAndDeAllocate/CloseImageCapture/ https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:292: // This function is used to allocate resources. Does the caller require to start the video stream first to use image capture? https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:293: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, Using AllocateImageCapture and DeAllocateImageCapture is more consistent? https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:296: // Releases used resources for image capture. s/used// Does CloseImageCapture have to be called before the object is deleted like StopAndDeAllocate? Or the destructor handles it? https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:299: // Gets one image from the device. It will returne the picture by the s/returne/return/. s/picture/image/. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:300: // ImageClient's OnIncomingCapturedData() callback. ImageClient::OnIncomingCapturedData(). Also mention OnMute and OnUnmute will be called. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. Add PIXEL_FORMAT_JPEG, https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:60: class MEDIA_EXPORT ImageCaptureFormat { Add comments like VideoCaptureFormat.
Thanks for the comments. Please take another look. Thanks. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.cc:82: client->OnError("Not supported"); On 2014/09/09 05:53:21, wuchengli wrote: > The client can know if image capture is supported by > GetDeviceSupportedImageFormats. I think we can just have an empty implementation > like CloseImageCapture and CaptureImage. If you want to call OnError, the other > two functions should call OnError, too. Good point. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:220: // The video stream is being muted. After this callback, no more On 2014/09/09 05:53:22, wuchengli wrote: > s/is being/has/ Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:221: // OnIncomingCapturedData() will be called. On 2014/09/09 05:53:21, wuchengli wrote: > Explain OnMute and OnUnmute are called when image capture is started and ended > is called. Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:224: // The video stream is being resumed. On 2014/09/09 05:53:22, wuchengli wrote: > s/is being/has/ Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:240: // The VideoCaptureDevice must be StopAndDeAllocate()-ed. |reason| is a On 2014/09/09 05:53:22, wuchengli wrote: > s/StopAndDeAllocate/CloseImageCapture/ I do mean StopAndDeallocate. In the current implementation, the device would go to an error state and has to be restart again. No error is expected so I don't think we need to spend too much effort in resuming from the error state for still image capture. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:292: // This function is used to allocate resources. On 2014/09/09 05:53:22, wuchengli wrote: > Does the caller require to start the video stream first to use image capture? Yes. Comment added. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:293: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, On 2014/09/09 05:53:22, wuchengli wrote: > Using AllocateImageCapture and DeAllocateImageCapture is more consistent? The problem is we may not really allocate resources here (like the current implementation). https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:296: // Releases used resources for image capture. On 2014/09/09 05:53:22, wuchengli wrote: > s/used// > > Does CloseImageCapture have to be called before the object is deleted like > StopAndDeAllocate? Or the destructor handles it? Command added. It has to be called before StopAndDeAllocate(). https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:299: // Gets one image from the device. It will returne the picture by the On 2014/09/09 05:53:21, wuchengli wrote: > s/returne/return/. s/picture/image/. Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_device.h:300: // ImageClient's OnIncomingCapturedData() callback. On 2014/09/09 05:53:21, wuchengli wrote: > ImageClient::OnIncomingCapturedData(). Also mention OnMute and OnUnmute will be > called. Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. On 2014/09/09 05:53:22, wuchengli wrote: > Add PIXEL_FORMAT_JPEG, Why here? We don't expect to get JPEG directly from ImageCaptureDevice, do we? https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:60: class MEDIA_EXPORT ImageCaptureFormat { On 2014/09/09 05:53:22, wuchengli wrote: > Add comments like VideoCaptureFormat. Done.
Can this add tests in video_capture_device_unittest.cc? https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. On 2014/09/11 06:18:00, Owen Lin wrote: > On 2014/09/09 05:53:22, wuchengli wrote: > > Add PIXEL_FORMAT_JPEG, > > Why here? We don't expect to get JPEG directly from ImageCaptureDevice, do we? Which file do you plan to encode the image to JPEG? https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:222: // CaptureImage() is being called. After the still image has been s/is being/has/ 80 char alignment. Run git cl format https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:223: // captured, the video stream will be resumed and the client would be Remove the extra space. I think the order should be switched. That is, the client will be notified by OnUnmute() and the video stream will be resumed. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:267: static void GetDeviceSupportedImageFormats( Does this compile? You need to implement it somewhere. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:303: virtual void CloseImageCapture() {} s/CloseImageCapture/ReleaseImageCapture/. Close should be opposite to Open. I think Release is better. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:305: // Gets one image from the device asynchronously. It will return the image More readable to put a blank line after "asynchronously" like StopAndDeAllocate. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:308: // Client::OnMute() and Client::OnUnmute() will be called. s/Client/ImageClient/ https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_types.h:62: // frame captured and returned to a client. It is also sued to specify a s/sued/used/
PTAL. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. On 2014/09/11 08:50:05, wuchengli wrote: > On 2014/09/11 06:18:00, Owen Lin wrote: > > On 2014/09/09 05:53:22, wuchengli wrote: > > > Add PIXEL_FORMAT_JPEG, > > > > Why here? We don't expect to get JPEG directly from ImageCaptureDevice, do we? > Which file do you plan to encode the image to JPEG? Done. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. Don't know yet. I guess it maybe somewhere in the renderer process. I think it won't be this file. Anyway, we can add FORMAT_JPEG later. On 2014/09/11 08:50:05, wuchengli wrote: > On 2014/09/11 06:18:00, Owen Lin wrote: > > On 2014/09/09 05:53:22, wuchengli wrote: > > > Add PIXEL_FORMAT_JPEG, > > > > Why here? We don't expect to get JPEG directly from ImageCaptureDevice, do we? > Which file do you plan to encode the image to JPEG? https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:222: // CaptureImage() is being called. After the still image has been On 2014/09/11 08:50:05, wuchengli wrote: > s/is being/has/ > > 80 char alignment. Run git cl format I think git cl won't help us on comment. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:223: // captured, the video stream will be resumed and the client would be On 2014/09/11 08:50:05, wuchengli wrote: > Remove the extra space. I think the order should be switched. That is, the > client will be notified by OnUnmute() and the video stream will be resumed. Done. Thanks. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:267: static void GetDeviceSupportedImageFormats( On 2014/09/11 08:50:05, wuchengli wrote: > Does this compile? You need to implement it somewhere. Yes. It compiles. I don't need if no one use it. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:303: virtual void CloseImageCapture() {} On 2014/09/11 08:50:05, wuchengli wrote: > s/CloseImageCapture/ReleaseImageCapture/. Close should be opposite to Open. I > think Release is better. Sure. However, I remember this is the name you suggest. :D https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_device.h:308: // Client::OnMute() and Client::OnUnmute() will be called. On 2014/09/11 08:50:05, wuchengli wrote: > s/Client/ImageClient/ It's Client. The callback is go to the client of the video stream. https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_types.h:62: // frame captured and returned to a client. It is also sued to specify a On 2014/09/11 08:50:05, wuchengli wrote: > s/sued/used/ Done.
Add BUG to the change description. https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/20001/media/video/capture/vide... media/video/capture/video_capture_types.h:30: PIXEL_FORMAT_TEXTURE, // Capture format as a GL texture. On 2014/09/12 09:07:55, Owen Lin wrote: > Don't know yet. I guess it maybe somewhere in the renderer process. I think it > won't be this file. Anyway, we can add FORMAT_JPEG later. > > On 2014/09/11 08:50:05, wuchengli wrote: > > On 2014/09/11 06:18:00, Owen Lin wrote: > > > On 2014/09/09 05:53:22, wuchengli wrote: > > > > Add PIXEL_FORMAT_JPEG, > > > > > > Why here? We don't expect to get JPEG directly from ImageCaptureDevice, do > we? > > Which file do you plan to encode the image to JPEG? > I thought the request to encode would be in browser process. I'll discuss with you offline. https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_device.h:242: // Callback function to notify client the failure of the image capturing. s/capturing/capture/ https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; We want an integer error to report to the error callback of pepper api. Change string to int or enum. https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_types.h:62: // frame captured and returned to a client. It is also used to specify a s/frame/still image/ https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_types.h:63: // supported capture format by a device. add "still image" before capture
https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_device.h:242: // Callback function to notify client the failure of the image capturing. On 2014/09/15 06:14:13, wuchengli wrote: > s/capturing/capture/ Done. https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; On 2014/09/15 06:14:13, wuchengli wrote: > We want an integer error to report to the error callback of pepper api. Change > string to int or enum. Can we keep it for consistency ? We are still far from the PPAPI. I think all of the errors here could be aggregated as one error in the PPAPI, e.g. PLATFORM_ERROR? https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_types.h:62: // frame captured and returned to a client. It is also used to specify a On 2014/09/15 06:14:13, wuchengli wrote: > s/frame/still image/ Done. https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_types.h:63: // supported capture format by a device. On 2014/09/15 06:14:13, wuchengli wrote: > add "still image" before capture Done.
https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/60001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; The current errors are 1. Fail to open the device 2. The opened device doesn't support video capture 3. No supported format found on the device. 4. IOCTL fail: S_FMT, S_PARM, VIDIOC_STREAMON, VIDIOC_STREAMOFF, VIDIOC_QBUF, VIDIOC_REQBUFS, VIDIOC_QUERYBUF .... 5. Timeout on waiting a frame. All the errors are not expected. Once happen, the device is in an error state. Need to restart the device to recover. On 2014/09/15 09:04:36, Owen Lin wrote: > On 2014/09/15 06:14:13, wuchengli wrote: > > We want an integer error to report to the error callback of pepper api. Change > > string to int or enum. > > Can we keep it for consistency ? We are still far from the PPAPI. I think all of > the errors here could be aggregated as one error in the PPAPI, e.g. > PLATFORM_ERROR? >
Please add owners to review this CL. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; OK. Let's keep this for now. PPAPI uses the error codes from pp_errors.h. If we need an integer error code, we can change it in the future. https://codereview.chromium.org/391323002/diff/400001/ppapi/api/private/ppb_i... https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.h:41: // supported still image capture format by a device. This belongs to ImageCaptureFormat, not here.
owenlin@chromium.org changed reviewers: + perkj@chromium.org
https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.h:41: // supported still image capture format by a device. On 2014/09/17 06:54:29, wuchengli wrote: > This belongs to ImageCaptureFormat, not here. Oops.
lgtm when the nits are fixed. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image has been captured, the s has been https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.cc:22: ImageCaptureFormat::ImageCaptureFormat() : pixel_format(PIXEL_FORMAT_UNKNOWN) { Please move ImageCaptureFormat to after all VideoCaptureFormat methods in this file.
https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Please add a more detailed explanation to the CL description. Please add an implementation of this to FakeVideoCaptureDevice and implement a unittest using that. https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image has been captured, the s/has called/was called/ Is there any other case that this may happen? If not, don't say "may happen". https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:231: class MEDIA_EXPORT ImageClient { StillImageClient here and everywhere? https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:235: // Callback function to notify the client the caputred image is available. s/caputred/captured https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:236: virtual void OnIncomingCapturedData(const uint8* data, Let's call this differently, in case one class wants to subclass both Clients. https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:239: int rotation, Please specify the parameters in the doc above. https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; I'm wondering if we need to have a separate OnError? The difference between this and the regular client is that we have many OnIncomingCapturedData() there and checking for error in the impl of it would be ugly and slightly inefficient. In this case we only have one OnIncomingCapturedData, which can either be a success or failure, so maybe checking for error or success would be ok in its implementation? Just wondering what others think about this... https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:267: static void GetDeviceSupportedImageFormats( StillImageFormats? https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:269: ImageCaptureFormats* supported_formats); What is the expectation for lifetime/ownership of this pointer? Please document how this works. https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... File media/video/capture/video_capture_types.h (right): https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_types.h:61: // This class is used by the video capture device to specify the format of the s/the format of the/the format of a/ https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_types.h:62: // still image captured and returned to a client. It is also used to specify a A list of these is also provided when client queries supported formats for still image capture.
https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:231: class MEDIA_EXPORT ImageClient { On 2014/09/17 11:29:42, Pawel Osciak wrote: > StillImageClient here and everywhere? Image is symmetric to video. For example, ImageCaptureFormats vs VideoCaptureFormats. StillImageCaptureFormats doesn't look good. https://chromiumcodereview.appspot.com/545053002/diff/80001/media/video/captu... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; On 2014/09/17 11:29:42, Pawel Osciak wrote: > I'm wondering if we need to have a separate OnError? The difference between this > and the regular client is that we have many OnIncomingCapturedData() there and > checking for error in the impl of it would be ugly and slightly inefficient. In > this case we only have one OnIncomingCapturedData, which can either be a success > or failure, so maybe checking for error or success would be ok in its > implementation? > Just wondering what others think about this... One thing to consider is we'll soon add doAutoFocus and setting camera parameters. Autofocus needs error callback. If setting parameters is asynchronous, it may also need error callback. Another way is to combine Client and ImageClient. But Client already has lots of functions. Maybe duplicating OnError is not too bad given autofocus will also use it.
https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/09/17 11:29:42, Pawel Osciak wrote: > Please add a more detailed explanation to the CL description. > Please add an implementation of this to FakeVideoCaptureDevice and implement a > unittest using that. CL description updated. I have another CL for adding the implementation for video_capture_device_linux and also the unittest to run it. I think it is good to add the implementation/test of still image capture for the FakeVideoCaptureDevice. But let me do it in another CL. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image has been captured, the On 2014/09/17 11:29:42, Pawel Osciak wrote: > s/has called/was called/ > Is there any other case that this may happen? If not, don't say "may happen". Hi, Pawel, I was thinking if the implementation doesn't require the video stream to stop. We won't call OnMute() and OnUnmute(). Do you think it's a case we need to consider or we must pause the video stream when take still image. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:235: // Callback function to notify the client the caputred image is available. On 2014/09/17 11:29:42, Pawel Osciak wrote: > s/caputred/captured Done. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:236: virtual void OnIncomingCapturedData(const uint8* data, On 2014/09/17 11:29:42, Pawel Osciak wrote: > Let's call this differently, in case one class wants to subclass both Clients. It is unlikely to happen. Both function "AllocateAndStart" and "InitializeImageCapture" will take the ownership of its client (It is passed as a scoped_ptr). So we cannot pass the same client to those two functions. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:239: int rotation, On 2014/09/17 11:29:42, Pawel Osciak wrote: > Please specify the parameters in the doc above. Done. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; On 2014/09/17 11:29:42, Pawel Osciak wrote: > I'm wondering if we need to have a separate OnError? The difference between this > and the regular client is that we have many OnIncomingCapturedData() there and > checking for error in the impl of it would be ugly and slightly inefficient. In > this case we only have one OnIncomingCapturedData, which can either be a success > or failure, so maybe checking for error or success would be ok in its > implementation? > Just wondering what others think about this... Good point. I was thinking that. The main reason I do this way is to handle the error not in the scope of taking a picture (e.g., allocation fail in InitializeImageCapture). Now, I am thinking maybe we can put the device into an error state and report the error when CaputreImage is called. In that case, we can remove the entire ImageClient and use callbacks in these functions. How do you think? https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:267: static void GetDeviceSupportedImageFormats( On 2014/09/17 11:29:42, Pawel Osciak wrote: > StillImageFormats? Let's keep using Image for consistency. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:269: ImageCaptureFormats* supported_formats); On 2014/09/17 11:29:42, Pawel Osciak wrote: > What is the expectation for lifetime/ownership of this pointer? Please document > how this works. ImageCaputreFormats is actullay a vector<ImageCaptureFormat>. Here, it's an output. Caller should provide it. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.cc:22: ImageCaptureFormat::ImageCaptureFormat() : pixel_format(PIXEL_FORMAT_UNKNOWN) { On 2014/09/17 11:25:59, perkj wrote: > Please move ImageCaptureFormat to after all VideoCaptureFormat methods in this > file. Done. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.h:61: // This class is used by the video capture device to specify the format of the On 2014/09/17 11:29:42, Pawel Osciak wrote: > s/the format of the/the format of a/ Done. https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_types.h:62: // still image captured and returned to a client. It is also used to specify a On 2014/09/17 11:29:42, Pawel Osciak wrote: > A list of these is also provided when client queries supported formats for still > image capture. Done.
https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/80001/media/video/capture/vide... media/video/capture/video_capture_device.h:245: virtual void OnError(const std::string& reason) = 0; On 2014/09/18 06:04:15, Owen Lin wrote: > On 2014/09/17 11:29:42, Pawel Osciak wrote: > > I'm wondering if we need to have a separate OnError? The difference between > this > > and the regular client is that we have many OnIncomingCapturedData() there and > > checking for error in the impl of it would be ugly and slightly inefficient. > In > > this case we only have one OnIncomingCapturedData, which can either be a > success > > or failure, so maybe checking for error or success would be ok in its > > implementation? > > Just wondering what others think about this... > > Good point. I was thinking that. The main reason I do this way is to handle the > error not in the scope of taking a picture (e.g., allocation fail in > InitializeImageCapture). > > Now, I am thinking maybe we can put the device into an error state and report > the error when CaputreImage is called. > > In that case, we can remove the entire ImageClient and use callbacks in these > functions. > > How do you think? I think having ImageClient interface is more consistent with VideoCaptureDevice::Client. Both callbacks and ImageClient work. We should choose one that is more consistent. As I said, we'll add autofocus and parameters setting, which will require lots of callbacks.
LGTM. Please remember to fix the typo. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| remove extra / after tightly
https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client s/has called/has been called/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client s/captured/is captured/ So the design is that we can't capture a still image without having the device streaming? Why this limitation? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:230: class MEDIA_EXPORT ImageClient { Please add documentation what this class is for. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:234: // Callback function to notify the client the captured image is available. s/the/a/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:236: // The captured still image is stored at address |data| and of |length| s/of/is of/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| s/should/is/ I think? s/for// https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:241: // Note that the content in |data| would be freed after this callback. s/would be freed/will not be valid/ s/callback/callback returns/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:244: int length, s/int/size_t/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:246: int rotation, // Clockwise Perhaps remove the comment, it's already in the doc above. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:249: // Callback function to notify client the failure of the image capture. s/client/the client/ s/the failure/about a failure/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:250: // The VideoCaptureDevice must be StopAndDeAllocate()-ed. |reason| is a s/is/contains/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:272: // Gets the supported formats for still image of a particular device attached Gets formats for still image capture supported by a particular... s/device/|device|/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:304: // StopAndDeAllocate(). Why do we have this requirement? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:305: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, I think we need to have a bool return in case this fails? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:310: // The ImageClient passed from InitializeImageCapture would be freed. This s/would/will/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:315: // Gets one image from the device asynchronously. Asynchronously to what? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:317: // It will return the image by the ImageClient::OnIncomingCapturedData() s/It will return the image by/The image will be returned via/ https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:321: // This function must be called between InitializeImageCapture() and Why this requirement?
https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/captured/is captured/ > > So the design is that we can't capture a still image without having the device > streaming? Why this limitation? 1. Same as the JS interface. 2. We need preview running before capturing an still image to adjust white balance and exposure. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:230: class MEDIA_EXPORT ImageClient { On 2014/09/23 08:15:57, Pawel Osciak wrote: > Please add documentation what this class is for. Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:234: // Callback function to notify the client the captured image is available. On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/the/a/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:236: // The captured still image is stored at address |data| and of |length| On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/of/is of/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| On 2014/09/23 07:09:22, wuchengli wrote: > remove extra / after tightly Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/should/is/ I think? > s/for// 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. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:241: // Note that the content in |data| would be freed after this callback. On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/would be freed/will not be valid/ > s/callback/callback returns/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:244: int length, On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/int/size_t/ For consistency with Client::OnIncomingCaptureData. I can fix it later. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:246: int rotation, // Clockwise On 2014/09/23 08:15:57, Pawel Osciak wrote: > Perhaps remove the comment, it's already in the doc above. Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:249: // Callback function to notify client the failure of the image capture. On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/client/the client/ > s/the failure/about a failure/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:250: // The VideoCaptureDevice must be StopAndDeAllocate()-ed. |reason| is a On 2014/09/23 08:15:57, Pawel Osciak wrote: > s/is/contains/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:272: // Gets the supported formats for still image of a particular device attached On 2014/09/23 08:15:56, Pawel Osciak wrote: > Gets formats for still image capture supported by a particular... > > s/device/|device|/ Done. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:304: // StopAndDeAllocate(). On 2014/09/23 08:15:57, Pawel Osciak wrote: > Why do we have this requirement? We need the preview on before taking a picture to adjust white balance and exposure. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:305: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, On 2014/09/23 08:15:57, Pawel Osciak wrote: > I think we need to have a bool return in case this fails? Fail will be reported by ImageClient::onError(). https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:315: // Gets one image from the device asynchronously. On 2014/09/23 08:15:57, Pawel Osciak wrote: > Asynchronously to what? Did I misuse it ? Asynchronously to this function. Client will get the image after this function returns. https://codereview.chromium.org/545053002/diff/120001/media/video/capture/vid... media/video/capture/video_capture_device.h:321: // This function must be called between InitializeImageCapture() and On 2014/09/23 08:15:57, Pawel Osciak wrote: > Why this requirement? It's the life time of the image_client instance.
https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client On 2014/09/24 05:20:03, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > s/captured/is captured/ > > > > So the design is that we can't capture a still image without having the device > > streaming? Why this limitation? > > 1. Same as the JS interface. 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. > 2. We need preview running before capturing an still image to adjust white > balance and exposure. 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. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| On 2014/09/24 05:20:03, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > s/should/is/ I think? > > s/for// > > 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. > 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? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:244: int length, On 2014/09/24 05:20:02, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > s/int/size_t/ > > For consistency with Client::OnIncomingCaptureData. I can fix it later. 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. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:305: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, On 2014/09/24 05:20:03, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > I think we need to have a bool return in case this fails? > Fail will be reported by ImageClient::onError(). 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? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:315: // Gets one image from the device asynchronously. On 2014/09/24 05:20:03, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > Asynchronously to what? > Did I misuse it ? Asynchronously to this function. Client will get the image > after this function returns. You should probably say Request instead of Get.
https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client 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: > On 2014/09/24 05:20:03, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > s/captured/is captured/ > > > > > > So the design is that we can't capture a still image without having the > device > > > streaming? Why this limitation? > > > > 1. Same as the JS interface. > > 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. 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. > > 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. 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. > > And VideoCaptureDevice is also not a JS-specific interface and JS is not the > only client. > > > 2. We need preview running before capturing an still image to adjust white > > balance and exposure. > > 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. 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. > > 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. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| On 2014/09/25 02:16:20, Pawel Osciak wrote: > On 2014/09/24 05:20:03, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > s/should/is/ I think? > > > s/for// > > > > 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. > > > > 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? 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. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:244: int length, On 2014/09/25 02:16:21, Pawel Osciak wrote: > On 2014/09/24 05:20:02, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > s/int/size_t/ > > > > For consistency with Client::OnIncomingCaptureData. I can fix it later. > > 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. Done. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:305: virtual void InitializeImageCapture(const ImageCaptureFormat& image_format, On 2014/09/25 02:16:20, Pawel Osciak wrote: > On 2014/09/24 05:20:03, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > I think we need to have a bool return in case this fails? > > Fail will be reported by ImageClient::onError(). > > 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? Good point. Let me make this synchronous. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:315: // Gets one image from the device asynchronously. On 2014/09/25 02:16:20, Pawel Osciak wrote: > On 2014/09/24 05:20:03, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > Asynchronously to what? > > Did I misuse it ? Asynchronously to this function. Client will get the image > > after this function returns. > > You should probably say Request instead of Get. Thanks. https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:317: // It will return the image by the ImageClient::OnIncomingCapturedData() On 2014/09/23 08:15:56, Pawel Osciak wrote: > s/It will return the image by/The image will be returned via/ Done.
https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:222: // CaptureImage() has called. After the still image captured, the client On 2014/09/25 05:53:53, Owen Lin wrote: > 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: > > On 2014/09/24 05:20:03, Owen Lin wrote: > > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > > s/captured/is captured/ > > > > > > > > So the design is that we can't capture a still image without having the > > device > > > > streaming? Why this limitation? > > > > > > 1. Same as the JS interface. > > > > 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. > > 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. > My suggestion would not be adding new features. The current one however is adding additional constraints, making the API more complicated to use. > > > > 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. > > 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. 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.). > > > > And VideoCaptureDevice is also not a JS-specific interface and JS is not the > > only client. > > > > > 2. We need preview running before capturing an still image to adjust white > > > balance and exposure. > > > > 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. > > Before I design this API, I have asked kcwu, he said the first few frames from > camera are not looking good. 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? > Besides, when user click on the button to take a picture. They want to take it > ASAP. Well, requiring them to run streaming first for an unspecified amount of time and then capture is not ASAP... https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The still image should be rotated for |rotation| On 2014/09/25 05:53:53, Owen Lin wrote: > On 2014/09/25 02:16:20, Pawel Osciak wrote: > > On 2014/09/24 05:20:03, Owen Lin wrote: > > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > > s/should/is/ I think? > > > > s/for// > > > > > > 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. > > > > > > > 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? > > 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. Sorry, what should we do? If there is no rotation info, what is this argument for? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:304: // StopAndDeAllocate(). On 2014/09/24 05:20:02, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > Why do we have this requirement? > > We need the preview on before taking a picture to adjust white balance and > exposure. What will happen if it's called at a wrong time (in terms of API), error, callback, etc.? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:311: // method must be called between InitializeImageCapture() and What will happen if it's called at a wrong time (in terms of API), error, callback, etc.? https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... media/video/capture/video_capture_device.h:321: // This function must be called between InitializeImageCapture() and On 2014/09/24 05:20:03, Owen Lin wrote: > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > Why this requirement? > > It's the life time of the image_client instance. What will happen if that's not the case (in terms of API), error, callback, etc.? https://chromiumcodereview.appspot.com/545053002/diff/140001/media/video/capt... File media/video/capture/video_capture_device.h (right): https://chromiumcodereview.appspot.com/545053002/diff/140001/media/video/capt... media/video/capture/video_capture_device.h:268: // This method should be called before allocating or starting a device. In What will happen if it's called at a wrong time (in terms of API), error, callback, etc.?
On 2014/09/29 00:13:37, Pawel Osciak wrote: > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > File media/video/capture/video_capture_device.h (right): > > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > media/video/capture/video_capture_device.h:222: // CaptureImage() has called. > After the still image captured, the client > On 2014/09/25 05:53:53, Owen Lin wrote: > > 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: > > > On 2014/09/24 05:20:03, Owen Lin wrote: > > > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > > > s/captured/is captured/ > > > > > > > > > > So the design is that we can't capture a still image without having the > > > device > > > > > streaming? Why this limitation? > > > > > > > > 1. Same as the JS interface. > > > > > > 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. > > > > 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. > > > > My suggestion would not be adding new features. The current one however is > adding additional constraints, making the API more complicated to use. > > > > > > > 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. > > > > 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. > > 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.). > > > > > > > And VideoCaptureDevice is also not a JS-specific interface and JS is not the > > > only client. > > > > > > > 2. We need preview running before capturing an still image to adjust white > > > > balance and exposure. > > > > > > 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. > > > > Before I design this API, I have asked kcwu, he said the first few frames from > > camera are not looking good. > > 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? > > > Besides, when user click on the button to take a picture. They want to take it > > ASAP. > > Well, requiring them to run streaming first for an unspecified amount of time > and then capture is not ASAP... > > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > media/video/capture/video_capture_device.h:238: // to be tightly/ packed. The > still image should be rotated for |rotation| > On 2014/09/25 05:53:53, Owen Lin wrote: > > On 2014/09/25 02:16:20, Pawel Osciak wrote: > > > On 2014/09/24 05:20:03, Owen Lin wrote: > > > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > > > s/should/is/ I think? > > > > > s/for// > > > > > > > > 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. > > > > > > > > > > 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? > > > > 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. > > Sorry, what should we do? If there is no rotation info, what is this argument > for? > > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > media/video/capture/video_capture_device.h:304: // StopAndDeAllocate(). > On 2014/09/24 05:20:02, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > Why do we have this requirement? > > > > We need the preview on before taking a picture to adjust white balance and > > exposure. > > What will happen if it's called at a wrong time (in terms of API), error, > callback, etc.? > > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > media/video/capture/video_capture_device.h:311: // method must be called between > InitializeImageCapture() and > What will happen if it's called at a wrong time (in terms of API), error, > callback, etc.? > > https://chromiumcodereview.appspot.com/545053002/diff/120001/media/video/capt... > media/video/capture/video_capture_device.h:321: // This function must be called > between InitializeImageCapture() and > On 2014/09/24 05:20:03, Owen Lin wrote: > > On 2014/09/23 08:15:57, Pawel Osciak wrote: > > > Why this requirement? > > > > It's the life time of the image_client instance. > > What will happen if that's not the case (in terms of API), error, callback, > etc.? > > https://chromiumcodereview.appspot.com/545053002/diff/140001/media/video/capt... > File media/video/capture/video_capture_device.h (right): > > https://chromiumcodereview.appspot.com/545053002/diff/140001/media/video/capt... > media/video/capture/video_capture_device.h:268: // This method should be called > before allocating or starting a device. In > What will happen if it's called at a wrong time (in terms of API), error, > callback, etc.? I have discussed this CL with Pawel off line. I explained how the rotation information is being used. We also agree to DCHECK if the API get called in incorrect order.
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545053002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545053002/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as ae270c7c29c23f5ab52bd081119ffc05dd0491e3
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/fd5bdd2688768683e30b27941f181597a849656d Cr-Commit-Position: refs/heads/master@{#298009} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
