Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
391323002:
Pepper: add Image Capture interfaces. (Closed)
|
Created:
6 years, 5 months ago by wuchengli Modified:
6 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionPepper: add Image Capture interfaces.
API proposal:
https://docs.google.com/a/chromium.org/document/d/1deeCrsvT9xA77UP4yWMhqbHgS3FvYCTF_8meIMQRrDo/edit#
BUG=387547
TEST=Build chrome, run PPAPITest, and run an app that uses ppapi.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290186
Patch Set 1 : #
Total comments: 8
Patch Set 2 : add PP_ImageCaptureConfig and PP_ImageCaptureCapabilities #
Total comments: 17
Patch Set 3 : move to private, add PPP_ImageCapture, and address comments #
Total comments: 38
Patch Set 4 : address comments, generate c and thunk files #Patch Set 5 : rebase #
Total comments: 46
Patch Set 6 : address review comments #
Total comments: 18
Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : add c++ wrappers and remove message loop #
Total comments: 10
Patch Set 9 : update comments #
Total comments: 44
Patch Set 10 : address review comments #
Total comments: 2
Messages
Total messages: 53 (0 generated)
This is a draft. Please take a look before I go deep down the road. I'm adding PP_CameraCapabilities and PP_ImageCaptureConfig.
Are you planning to send an API proposal out for this? Thanks! To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/16 06:56:32, chromium-reviews wrote: > Are you planning to send an API proposal out for this? Yes. I'll send it out for review when it's ready. > > Thanks! > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:42: * Sets the configuration of the photos and the camera device. [optional] "Sets the configuration of the camera device." sounds good enough. https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:53: * <code>SetConfig()</code> or <code>TakePhoto()</code>. What's the definition of the pending call to TakePhoto(). Until the picture_callback is being called? https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:61: * Gets attribute values for given attribute names. Where are the attribute values and attribute names? Inside the config? In that case, you should change the type to [inout]. I was assuming it contains all attributes available. (Not just the given attribute names.) https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:98: * VideoFrame resource used to store the postview. Did you consider to make it a ImageData? Client can use graphics_2d to render it. Do we need to duplicate the vidoe frame from the video track ? https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:101: * @param[in] callback A <code>PP_CompletionCallback</code> to be called when s/callback/shutter_callback/ https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:104: * @param[in] callback A <code>PP_CompletionCallback</code> to be called to s/callback/postview_callback/. Can you move most of your comment to param postview. Since it is actually a completion callback and returns nothing. https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:107: * @param[in] callback A <code>PP_CompletionCallback</code> to be called upon photo_callback https://codereview.chromium.org/391323002/diff/40001/ppapi/api/ppb_image_capt... ppapi/api/ppb_image_capture.idl:118: [in] PP_CompletionCallback picture_callback); why not using the name "photo_callback".
The extra resources for Capabilities and Config might be overkill. You could consider just PP_ structs for those, maybe... https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:28: PP_Resource Create([in] PP_Resource media_stream_video_track); I'm a little unsure about this. I like that it neatly takes care of permissions, but it's a little strange. There's not an analogous JavaScript API exactly. This is kind of a lower-level API, wanting to control stuff like AutoFocus. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:121: [in] PP_CompletionCallback photo_callback); We can't use multiple completion callbacks for 1 function. It's supposed to be possible to pass a blocking callback. It's not clear what would happen if one callback was passed as blocking and two others weren't for instance. And the return code is sometimes an indication of whether the callback will be run. So it's unclear what PP_OK_COMPLETIONPENDING or PP_OK would mean... which callback(s) will still be run? Two alternatives: 1) Split it in to multiple calls, each with its own completion callbakc. 2) Use a PPP-style interface (See PPB_Messaging::RegisterMessageHandler and PPP_MessageHandler) 1 might be best, since it would allow you to block at each stage. Either lets you get your result on a background thread. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture_capabilities.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture_capabilities.idl:49: [out] PP_Size[] photo_sizes); This would also have to have the array size as an inout, I think?
On 2014/07/16 23:10:54, dmichael wrote: > The extra resources for Capabilities and Config might be overkill. You could > consider just PP_ structs for those, maybe... I was thinking to use structs. But Capabilities have dynamic size arrays. Config will be likely to have it soon. It looks like structs can be only fixed size and cannot use pointers. > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > File ppapi/api/ppb_image_capture.idl (right): > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > ppapi/api/ppb_image_capture.idl:28: PP_Resource Create([in] PP_Resource > media_stream_video_track); > I'm a little unsure about this. I like that it neatly takes care of permissions, > but it's a little strange. There's not an analogous JavaScript API exactly. This > is kind of a lower-level API, wanting to control stuff like AutoFocus. > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > ppapi/api/ppb_image_capture.idl:121: [in] PP_CompletionCallback photo_callback); > We can't use multiple completion callbacks for 1 function. It's supposed to be > possible to pass a blocking callback. It's not clear what would happen if one > callback was passed as blocking and two others weren't for instance. And the > return code is sometimes an indication of whether the callback will be run. > > So it's unclear what PP_OK_COMPLETIONPENDING or PP_OK would mean... which > callback(s) will still be run? > > Two alternatives: > 1) Split it in to multiple calls, each with its own completion callbakc. > 2) Use a PPP-style interface (See PPB_Messaging::RegisterMessageHandler and > PPP_MessageHandler) > > 1 might be best, since it would allow you to block at each stage. Either lets > you get your result on a background thread. > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > File ppapi/api/ppb_image_capture_capabilities.idl (right): > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > ppapi/api/ppb_image_capture_capabilities.idl:49: [out] PP_Size[] photo_sizes); > This would also have to have the array size as an inout, I think?
https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:28: PP_Resource Create([in] PP_Resource media_stream_video_track); On 2014/07/16 23:10:54, dmichael wrote: > I'm a little unsure about this. I like that it neatly takes care of permissions, > but it's a little strange. There's not an analogous JavaScript API exactly. This > is kind of a lower-level API, wanting to control stuff like AutoFocus. I followed the design in Mediastream Image Capture draft. The idea is MediaStreamVideoTrack is for preview and we want to use the same camera to take a picture. Or we can take a video sourceId or MediaStreamTrack.id in the constructor. http://www.w3.org/TR/image-capture/#taking-a-picture-if-red-eye-reduction-is-... https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:121: [in] PP_CompletionCallback photo_callback); On 2014/07/16 23:10:54, dmichael wrote: > We can't use multiple completion callbacks for 1 function. It's supposed to be > possible to pass a blocking callback. It's not clear what would happen if one > callback was passed as blocking and two others weren't for instance. And the > return code is sometimes an indication of whether the callback will be run. > > So it's unclear what PP_OK_COMPLETIONPENDING or PP_OK would mean... which > callback(s) will still be run? > > Two alternatives: > 1) Split it in to multiple calls, each with its own completion callbakc. > 2) Use a PPP-style interface (See PPB_Messaging::RegisterMessageHandler and > PPP_MessageHandler) > > 1 might be best, since it would allow you to block at each stage. Either lets > you get your result on a background thread. You are right. I read the documentation of completion callback and it doesn't make sense to have three of them for one function. TakePhoto should be one function call and three callbacks. Making multiple calls is not efficient. I'll try option 2.
I wanted to let you know that I'm starting to talk to some people about whether we should start making this kind of API a completely different way (using mojo). It's unclear to me if we're ready for such a step, or if we should stick to Pepper for the short-medium term. We'll try to figure this out quickly so we don't block you too long. I'll spend some more time tomorrow looking at your proposal and the spec.
On 2014/07/18 03:05:06, dmichael wrote: > I wanted to let you know that I'm starting to talk to some people about whether > we should start making this kind of API a completely different way (using mojo). > It's unclear to me if we're ready for such a step, or if we should stick to > Pepper for the short-medium term. We'll try to figure this out quickly so we > don't block you too long. > > I'll spend some more time tomorrow looking at your proposal and the spec. Thanks. I just read http://dev.chromium.org/developers/design-documents/mojo. What is the relationship between this API and mojo?
On 2014/07/18 03:21:06, wuchengli wrote: > On 2014/07/18 03:05:06, dmichael wrote: > > I wanted to let you know that I'm starting to talk to some people about > whether > > we should start making this kind of API a completely different way (using > mojo). > > It's unclear to me if we're ready for such a step, or if we should stick to > > Pepper for the short-medium term. We'll try to figure this out quickly so we > > don't block you too long. > > > > I'll spend some more time tomorrow looking at your proposal and the spec. > Thanks. I just read http://dev.chromium.org/developers/design-documents/mojo. > What is the relationship between this API and mojo? Mojo is a new way to provide services to other parts of Chrome. We might also be able to use it for providing services that ARC needs. I added some comments to your feature bug.
https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:24: * What happens if the video track is Close()-ed? https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:96: * Add comments that it may mute()/unmute() the paired video track object? It seems important. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:104: * resource used to store the encoded picture. The default format is JPEG. I suspect that we need to add a new PP_Resource. I'm not sure if PPB_ImageData fits here, which seems to be designed for alpha-blending immediately (only premultiplied ARGB format). I don't know if extending PPB_ImageData to support JPG will break the overall design. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:112: * encoded picture. A general question: do we want to do something like PPB_MediaStreamVideoTrack.RecycleFrame()? I'm worried about the internal memory fragmentation, esp when the image size can go up to 10MB. I've never done real camera handling before, just a feeling that a frame ring can be also helpful when supporting burst mode in the future.
Message was sent while issue was closed.
PTAL https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:24: * On 2014/07/21 17:34:56, Justin Chuang wrote: > What happens if the video track is Close()-ed? Now I think video track and image capture should be decoupled more. We should not force the applications to have an active video track when using image capture. The constructor simply provides a way to use the same camera that video tracks associates with. So the plugin can continue to use ImageCapture object if the video track is closed. Pawel. Does V4L2 require that some settings can only be controlled when streaming is on? For example, can we control autofocus when streaming is off? https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:28: PP_Resource Create([in] PP_Resource media_stream_video_track); On 2014/07/17 14:05:47, wuchengli wrote: > On 2014/07/16 23:10:54, dmichael wrote: > > I'm a little unsure about this. I like that it neatly takes care of > permissions, > > but it's a little strange. There's not an analogous JavaScript API exactly. > This > > is kind of a lower-level API, wanting to control stuff like AutoFocus. > I followed the design in Mediastream Image Capture draft. The idea is > MediaStreamVideoTrack is for preview and we want to use the same camera to take > a picture. Or we can take a video sourceId or MediaStreamTrack.id in the > constructor. > > http://www.w3.org/TR/image-capture/#taking-a-picture-if-red-eye-reduction-is-... I've changed the constructor argument to source ID. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:96: * On 2014/07/21 17:34:56, Justin Chuang wrote: > Add comments that it may mute()/unmute() the paired video track object? It seems > important. Done. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:104: * resource used to store the encoded picture. The default format is JPEG. On 2014/07/21 17:34:56, Justin Chuang wrote: > I suspect that we need to add a new PP_Resource. > > I'm not sure if PPB_ImageData fits here, which seems to be designed for > alpha-blending immediately (only premultiplied ARGB format). I don't know if > extending PPB_ImageData to support JPG will break the overall design. PPB_ImageData applies to only ARGB so we should not use it. It looks like we can add Jpeg to ppapi/api/ppb_video_frame.idl (not ppapi/api/private/pp_video_frame_private.idl). So I use VideoFrame PP_Resource for both postview and jpeg pictures. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:112: * encoded picture. On 2014/07/21 17:34:56, Justin Chuang wrote: > A general question: do we want to do something like > PPB_MediaStreamVideoTrack.RecycleFrame()? I'm worried about the internal memory > fragmentation, esp when the image size can go up to 10MB. We may need it in the future. When I was on Android, taking pictures allocated memory every time, caused lots of GC, and slowed down the image capture. But the size of Jpeg is not fixed like YUV. So the buffer size has to be provided by the driver. Let's make image capture work first and come back to optimization later. > > I've never done real camera handling before, just a feeling that a frame ring > can be also helpful when supporting burst mode in the future. We should only expose the minimum API that are required today. Let's worry about this in the future. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:121: [in] PP_CompletionCallback photo_callback); On 2014/07/17 14:05:47, wuchengli wrote: > On 2014/07/16 23:10:54, dmichael wrote: > > We can't use multiple completion callbacks for 1 function. It's supposed to be > > possible to pass a blocking callback. It's not clear what would happen if one > > callback was passed as blocking and two others weren't for instance. And the > > return code is sometimes an indication of whether the callback will be run. > > > > So it's unclear what PP_OK_COMPLETIONPENDING or PP_OK would mean... which > > callback(s) will still be run? > > > > Two alternatives: > > 1) Split it in to multiple calls, each with its own completion callbakc. > > 2) Use a PPP-style interface (See PPB_Messaging::RegisterMessageHandler and > > PPP_MessageHandler) > > > > 1 might be best, since it would allow you to block at each stage. Either lets > > you get your result on a background thread. > You are right. I read the documentation of completion callback and it doesn't > make sense to have three of them for one function. > > TakePhoto should be one function call and three callbacks. Making multiple calls > is not efficient. I'll try option 2. I moved the callbacks to PPP_ImageCapture_Private. This is how all other pepper APIs do like PPB_VideoDecoder/PPP_VideoDecoder. https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture_capabilities.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture_capabilities.idl:49: [out] PP_Size[] photo_sizes); On 2014/07/16 23:10:54, dmichael wrote: > This would also have to have the array size as an inout, I think? Done.
https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... File ppapi/api/ppb_image_capture.idl (right): https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... ppapi/api/ppb_image_capture.idl:24: * I think we need stream on to take a photo. It need continuously to adjust the 3A settings.
https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] PP_Size[] jpeg_sizes); dmichael@ When I ran generator.py, there was an error message -- TGenError: 'There is no default value for rtype struct PP_Size*. Maybe you should provide an on_failure attribute in the IDL file.'. Do you know why? Is adding on_failure the right solution?
https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] PP_Size[] jpeg_sizes); On 2014/07/29 05:34:27, wuchengli wrote: > dmichael@ When I ran generator.py, there was an error message -- TGenError: > 'There is no default value for rtype struct PP_Size*. Maybe you should provide > an on_failure attribute in the IDL file.'. Do you know why? Is adding on_failure > the right solution? That doesn't seem right. We should not touch out-params in case of error. You can probably add [on_failure=NULL] right before the method to shut up the IDL generator. If not, we may need to fix it...
https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedJpegSize() returns the supported Jpeg sizes for the given Should this be Jpeg-specific? Does this have a corresponding camera API call? Perhaps we should rather have a list of supported formats returned by the call? https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedJpegSize() returns the supported Jpeg sizes for the given s/Jpeg/JPEG/ https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_camera_capabilities_private.idl:45: * is 0, the camera has no support of generating Jpeg pictures. s/of/for/ https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_camera_capabilities_private.idl:46: * @param[out] jpeg_sizes An array of <code>PP_Size</code> corresonding to s/corresonding/corresponding/ https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_camera_capabilities_private.idl:47: * the supported Jpeg photo sizes in pixels of the camera. s/ of the camera// I think we should stick to one of {picture, photo, image} in general in this file and everywhere else. https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_image_capture_config_private.idl:19: * several functions for establishing your image capture configuration within s/your// https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_image_capture_config_private.idl:21: * <code>PPB_ImageCapture.SetConfig</code> is called. s/PPB_ImageCapture/PPB_ImageCapture_Private/ ? https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... File ppapi/api/private/ppb_image_capture_private.idl (right): https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_image_capture_private.idl:29: * is closed, it will not affect this PPB_ImageCapture_Private object. What does it mean it will not affect? If we close it we can still capture? https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppb_image_capture_private.idl:120: * image capture or autofocus is submitted. Why is it inout? Can implementation change it? If so, when? https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... File ppapi/api/private/ppp_image_capture_private.idl (right): https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppp_image_capture_private.idl:28: * feedback of camera operation. This may be some time after the photo was s/may be some time/will occur/ https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppp_image_capture_private.idl:29: * triggered, but some time before the actual data is available. s/triggered/captured/ I assume? https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppp_image_capture_private.idl:29: * triggered, but some time before the actual data is available. s/some time// https://chromiumcodereview.appspot.com/391323002/diff/140001/ppapi/api/privat... ppapi/api/private/ppp_image_capture_private.idl:60: [in] PP_Resource postview, What's the format? How is this returned after use?
https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:60: */ Can this method returns fail on wrong photo_size? https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:32: * PPB_ImageCapture_Private resource if successful, 0 if failed. Seems all other PPAPI has a [in] PP_Instance instance argument here
Pawel, please check old Patch Set 2, ppb_image_capture.idl: Wucheng and Owen were discussing if we need stream on to take a photo. I'm not very familiar with V4L2. In Patch Set 2, it's more close to W3C ImageCapture Draft API (Image Capture is associated with a MediaStream Video Track). In Patch Set 3, Wucheng's new design is more independent from Media Stream Video Track. It just uses the camera source id. Any comment which approach is better? https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:32: * PPB_ImageCapture_Private resource if successful, 0 if failed. On 2014/08/03 09:45:45, Justin Chuang wrote: > Seems all other PPAPI has a [in] PP_Instance instance argument here Can see PPB_AudioConfig as an example. Its CreateXXX method also has parameters like here. It still requires a PP_Instance. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... File ppapi/api/private/ppp_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:19: * implementation. I am not familiar with PPAPI, but I suspect it should be done in a way similar to ppp_image_capture_private.idl => GetPermissionSettings(). In PPP_Flash_BrowserOperations::GetPermissionSettings(), ppp just provides the prototype of callback, and provides API to *register* the callback function pointer implemented elsewhere. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:60: [in] PP_Resource postview, On 2014/08/03 08:34:38, Pawel Osciak wrote: > What's the format? How is this returned after use? The format should be similar to PPB_MediaStreamVideoTrack::GetFrame() to the associated camera source. So it probably needs extra comments to describe that the image resolution of Postview is the same as associated camera_source_id in PPB_ImageCapture_Private::Create().
https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:127: [inout] int64_t sequence_id); Need a Close() method here.
All done. PTAL. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedJpegSize() returns the supported Jpeg sizes for the given On 2014/08/03 08:34:38, Pawel Osciak wrote: > Should this be Jpeg-specific? Does this have a corresponding camera API call? > Perhaps we should rather have a list of supported formats returned by the call? This has a corresponding camera API call and should be Jpeg-specific. Also, cameras may have different supported sizes for different output formats. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedJpegSize() returns the supported Jpeg sizes for the given On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/Jpeg/JPEG/ Replaced all Jpeg with JPEG. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:45: * is 0, the camera has no support of generating Jpeg pictures. On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/of/for/ Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:46: * @param[out] jpeg_sizes An array of <code>PP_Size</code> corresonding to On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/corresonding/corresponding/ Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:47: * the supported Jpeg photo sizes in pixels of the camera. On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/ of the camera// Done. > > I think we should stick to one of {picture, photo, image} in general in this > file and everywhere else. Changed everything to image. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:19: * several functions for establishing your image capture configuration within On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/your// Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:21: * <code>PPB_ImageCapture.SetConfig</code> is called. On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/PPB_ImageCapture/PPB_ImageCapture_Private/ ? Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:60: */ On 2014/08/03 09:45:45, Justin Chuang wrote: > Can this method returns fail on wrong photo_size? No. This method only stores the size in this object. The setting is applied by PPP_ImageCapture_Private.SetConfig. SetConfig can return failure. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:29: * is closed, it will not affect this PPB_ImageCapture_Private object. On 2014/08/03 08:34:38, Pawel Osciak wrote: > What does it mean it will not affect? If we close it we can still capture? Yes. I updated the comments. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:32: * PPB_ImageCapture_Private resource if successful, 0 if failed. On 2014/08/03 09:45:45, Justin Chuang wrote: > Seems all other PPAPI has a [in] PP_Instance instance argument here You are right. Added PP_Instance. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:120: * image capture or autofocus is submitted. On 2014/08/03 08:34:38, Pawel Osciak wrote: > Why is it inout? Can implementation change it? If so, when? Fixed. This should be out. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... File ppapi/api/private/ppp_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:19: * implementation. On 2014/08/03 10:46:45, Justin Chuang wrote: > I am not familiar with PPAPI, but I suspect it should be done in a way similar > to ppp_image_capture_private.idl => GetPermissionSettings(). > > In PPP_Flash_BrowserOperations::GetPermissionSettings(), ppp just provides the > prototype of callback, and provides API to *register* the callback function > pointer implemented elsewhere. Good idea. This patchset is similar to PPP_VideoDecoder, which requires the implementations of all callbacks to work. Image capture callbacks are optional and should follow your suggestion. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:28: * feedback of camera operation. This may be some time after the photo was On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/may be some time/will occur/ Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:29: * triggered, but some time before the actual data is available. On 2014/08/03 08:34:39, Pawel Osciak wrote: > s/triggered/captured/ I assume? Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:29: * triggered, but some time before the actual data is available. On 2014/08/03 08:34:38, Pawel Osciak wrote: > s/some time// Done. https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:60: [in] PP_Resource postview, On 2014/08/03 10:46:45, Justin Chuang wrote: > On 2014/08/03 08:34:38, Pawel Osciak wrote: > > What's the format? How is this returned after use? > > The format should be similar to PPB_MediaStreamVideoTrack::GetFrame() to the > associated camera source. > > So it probably needs extra comments to describe that the image resolution of > Postview is the same as associated camera_source_id in > PPB_ImageCapture_Private::Create(). Several media stream video tracks can be associated with the same camera. I added SetPreviewSize, GetPreviewSize, and GetSupportedPreviewSizes. Postview is renamed to Preview. The format is the original format from the camera. It can be read from PPB_VideoFrame.GetFormat. If there is a need to specify the format, we can add it at that time. Destroying the VideoFrame object will release the buffer. CaptureStillImage is not called very often like video capture. If the performance is a problem in the future, we can add something like PPB_MediaStreamVideoTrack.RecycleFrame and GetFrame. I added more comments in PPB_ImageCapture_Private.CaptureStillImage.
Hi Dave. PTAL.
https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... File ppapi/api/private/ppp_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... ppapi/api/private/ppp_image_capture_private.idl:60: [in] PP_Resource postview, On 2014/08/06 08:14:58, wuchengli wrote: > On 2014/08/03 10:46:45, Justin Chuang wrote: > > On 2014/08/03 08:34:38, Pawel Osciak wrote: > > > What's the format? How is this returned after use? > > > > The format should be similar to PPB_MediaStreamVideoTrack::GetFrame() to the > > associated camera source. > > > > So it probably needs extra comments to describe that the image resolution of > > Postview is the same as associated camera_source_id in > > PPB_ImageCapture_Private::Create(). > Several media stream video tracks can be associated with the same camera. I > added SetPreviewSize, GetPreviewSize, and GetSupportedPreviewSizes. Postview is > renamed to Preview. The format is the original format from the camera. It can be > read from PPB_VideoFrame.GetFormat. If there is a need to specify the format, we > can add it at that time. > Can you elaborate the relationship between the *Preview API and the always-on preview window in camera app (which supposedly is implemented with MediaStreamVideoTrack)? Will the Preview image you get here be grabbed by the live streaming path? Or will it be obtained by different data path, such as resizing the captured still image? If it's the former case, then there can be only one fixed resolution per device for the streaming even when multiple Media Stream are attached to one device. I suppose the master client will be the MediaStreamVideoTrack used by the always-on camera preview window. Is it correct? Thanks
https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:46: * @return An array of <code>PP_Size</code> corresondping to typo https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] int32_t count); When to free the output PPSize[]? A similar example is PPP_Flash_BrowserOperations void GetSitesWithData([in] str_t plugin_data_path, [out] fstr_t[] sites); => void (*GetSitesWithData)(const char* plugin_data_path, char** sites[]); It requires void (*FreeSiteList)(char* sites[]); to release the memory. Another method is to return a PP_Var, which contains an array. Don't know which method is better. https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_ima... File ppapi/c/private/ppb_image_capture_private.h (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_ima... ppapi/c/private/ppb_image_capture_private.h:209: * preview of the captured imgage. typo of image
https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:65: struct PP_Size (*GetSupportedPreviewSizes[])(PP_Resource capabilities, The prototype is not generated correctly.
On 2014/07/29 03:34:43, Owen Lin wrote: > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > File ppapi/api/ppb_image_capture.idl (right): > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > ppapi/api/ppb_image_capture.idl:24: * > I think we need stream on to take a photo. It need continuously to adjust the 3A > settings. Yes, we need to streamon.
On 2014/07/29 03:24:50, wuchengli wrote: > PTAL > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > File ppapi/api/ppb_image_capture.idl (right): > > https://codereview.chromium.org/391323002/diff/100001/ppapi/api/ppb_image_cap... > ppapi/api/ppb_image_capture.idl:24: * > On 2014/07/21 17:34:56, Justin Chuang wrote: > > What happens if the video track is Close()-ed? > Now I think video track and image capture should be decoupled more. We should > not force the applications to have an active video track when using image > capture. The constructor simply provides a way to use the same camera that video > tracks associates with. So the plugin can continue to use ImageCapture object if > the video track is closed. > > Pawel. Does V4L2 require that some settings can only be controlled when > streaming is on? For example, can we control autofocus when streaming is off? The API does not specify it. It's up to the driver to return success/error returns depending on the hardware, etc. I don't think you need to require an active video track. Perhaps you could just enable it just before taking the picture if it wasn't enabled?
https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedPreviewSize() returns the supported preview sizes for the given s/Size/Sizes/ https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:44: * @param[out] count The count of preview size array. s/count/size/ https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:71: * @param[out] jpeg_size A <code>PP_Size</code> that indicates the requested s/requested/current/ https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:7: * Defines the <code>PPB_ImageCapture_Private</code> interface. Used for taking s/taking an/acquiring a single/ https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:35: * </code>. Called for the camera to deliver a preview image. The client s/for the camera// And below too. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:39: * s/know/get/ https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:122: * If an error is returned, all configuration will not be changed. s/all// https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:168: * callback,and JPEG callback. The shutter callback occurs after the image is s/,/, / https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:179: * The camera may need to stop and re-start the streaming during image s/the// https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:76: * @return An array of <code>PP_Size</code> corresondping to the supported s/corresondping/corresponding/
https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:18: * The <code>PPB_CameraCapabilities_Private</code> interface contains pointers to line too long. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:67: PP_Size[] GetSupportedJPEGSizes( In func name, I believe the naming convention suggests using "Jpeg". http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names where it use Url not URL. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:68: [in] PP_Resource capabilities, It's kind of strange to return the array and size differently. FYR: "private/finish_writing_these/ppb_pdf.idl" https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:18: * The <code>PPB_ImageCaptureConfig_Private</code> interface contains pointers to line too long https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:45: * @param[out] preview_size A <code>PP_Size</code> that indicates the requested line too long. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:74: void GetJPEGSize( GetJpegSize https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:79: * SetJPEGSize() sets the JPEG image size for the given SetJpegSize https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:133: * @param[out] config A <code>PP_ImageCaptureConfig_Private</code> for storing the line too long https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:198: * @return An int32_t containing a result code from <code>pp_errors.h</code>. A more general way is passing a user_data: You can find one example in "dev/ppb_audio_input_dev.idl"
All done. PTAL. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:18: * The <code>PPB_CameraCapabilities_Private</code> interface contains pointers to On 2014/08/07 07:45:04, Owen Lin wrote: > line too long. Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:39: * GetSupportedPreviewSize() returns the supported preview sizes for the given On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/Size/Sizes/ Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:44: * @param[out] count The count of preview size array. On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/count/size/ Done. I used count because several other ppapi used count. I also prefer size. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:46: * @return An array of <code>PP_Size</code> corresondping to On 2014/08/07 04:27:28, Justin Chuang wrote: > typo Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] int32_t count); On 2014/08/07 04:27:27, Justin Chuang wrote: > When to free the output PPSize[]? > > A similar example is PPP_Flash_BrowserOperations > > void GetSitesWithData([in] str_t plugin_data_path, > [out] fstr_t[] sites); > => > void (*GetSitesWithData)(const char* plugin_data_path, char** sites[]); > > It requires > void (*FreeSiteList)(char* sites[]); > to release the memory. > > Another method is to return a PP_Var, which contains an array. > Don't know which method is better. Good point. If the ownership belongs to the caller, we can use PPB_Memory_Dev.MemFree. I think it's more convenient if the ownership belongs to this class. The caller will own a PPB_CameraCapabilities_Private object anyway, which contains the memory of preview sizes and picture sizes. The caller doesn't need another copy when calling this function. If there is a need, the caller can allocate a new array and copy out. I added some comments. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:67: PP_Size[] GetSupportedJPEGSizes( On 2014/08/07 07:45:04, Owen Lin wrote: > In func name, I believe the naming convention suggests using "Jpeg". > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names > > where it use Url not URL. Pawel suggested JPEG. I just searched the code base. The only function not in third_party that has JPEG is JpegLibraryVariant. I searched for URL and people use both URL and Url. I personally prefer Jpeg. Pawel. I'm changing this back to Jpeg. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:68: [in] PP_Resource capabilities, On 2014/08/07 07:45:04, Owen Lin wrote: > It's kind of strange to return the array and size differently. > > FYR: "private/finish_writing_these/ppb_pdf.idl" Done. I thought this generated failure when running generator.py. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:18: * The <code>PPB_ImageCaptureConfig_Private</code> interface contains pointers to On 2014/08/07 07:45:04, Owen Lin wrote: > line too long Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:45: * @param[out] preview_size A <code>PP_Size</code> that indicates the requested On 2014/08/07 07:45:04, Owen Lin wrote: > line too long. Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:71: * @param[out] jpeg_size A <code>PP_Size</code> that indicates the requested On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/requested/current/ Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:74: void GetJPEGSize( On 2014/08/07 07:45:04, Owen Lin wrote: > GetJpegSize Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:79: * SetJPEGSize() sets the JPEG image size for the given On 2014/08/07 07:45:04, Owen Lin wrote: > SetJpegSize Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:7: * Defines the <code>PPB_ImageCapture_Private</code> interface. Used for taking On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/taking an/acquiring a single/ Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:35: * </code>. Called for the camera to deliver a preview image. The client On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/for the camera// > > And below too. Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:39: * On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/know/get/ Removed this. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:122: * If an error is returned, all configuration will not be changed. On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/all// Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:133: * @param[out] config A <code>PP_ImageCaptureConfig_Private</code> for storing the On 2014/08/07 07:45:04, Owen Lin wrote: > line too long Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:168: * callback,and JPEG callback. The shutter callback occurs after the image is On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/,/, / Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:179: * The camera may need to stop and re-start the streaming during image On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/the// Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:198: * @return An int32_t containing a result code from <code>pp_errors.h</code>. On 2014/08/07 07:45:05, Owen Lin wrote: > A more general way is passing a user_data: > You can find one example in "dev/ppb_audio_input_dev.idl" Yeah. I feel using an integer is easier to implement. If we use user_data, the implementation may still need to use a sequence ID. https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:65: struct PP_Size (*GetSupportedPreviewSizes[])(PP_Resource capabilities, On 2014/08/07 04:30:28, Justin Chuang wrote: > The prototype is not generated correctly. Done. Moved the return value to a parameter. https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:76: * @return An array of <code>PP_Size</code> corresondping to the supported On 2014/08/07 06:22:46, Pawel Osciak wrote: > s/corresondping/corresponding/ Done. https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_ima... File ppapi/c/private/ppb_image_capture_private.h (right): https://codereview.chromium.org/391323002/diff/260001/ppapi/c/private/ppb_ima... ppapi/c/private/ppb_image_capture_private.h:209: * preview of the captured imgage. On 2014/08/07 04:27:28, Justin Chuang wrote: > typo of image Done.
On 2014/08/07 03:34:46, Justin Chuang wrote: > https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... > File ppapi/api/private/ppp_image_capture_private.idl (right): > > https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... > ppapi/api/private/ppp_image_capture_private.idl:60: [in] PP_Resource postview, > On 2014/08/06 08:14:58, wuchengli wrote: > > On 2014/08/03 10:46:45, Justin Chuang wrote: > > > On 2014/08/03 08:34:38, Pawel Osciak wrote: > > > > What's the format? How is this returned after use? > > > > > > The format should be similar to PPB_MediaStreamVideoTrack::GetFrame() to the > > > associated camera source. > > > > > > So it probably needs extra comments to describe that the image resolution of > > > Postview is the same as associated camera_source_id in > > > PPB_ImageCapture_Private::Create(). > > Several media stream video tracks can be associated with the same camera. I > > added SetPreviewSize, GetPreviewSize, and GetSupportedPreviewSizes. Postview > is > > renamed to Preview. The format is the original format from the camera. It can > be > > read from PPB_VideoFrame.GetFormat. If there is a need to specify the format, > we > > can add it at that time. > > > > Can you elaborate the relationship between the *Preview API and the always-on > preview window in camera app (which supposedly is implemented with > MediaStreamVideoTrack)? > > Will the Preview image you get here be grabbed by the live streaming path? Or > will it be obtained by different data path, such as resizing the captured still > image? > > If it's the former case, then there can be only one fixed resolution per device > for the streaming even when multiple Media Stream are attached to one device. I > suppose the master client will be the MediaStreamVideoTrack used by the > always-on camera preview window. Is it correct? > > Thanks The preview image cannot be sent to those media streams because there is no way for the application to know which frame is the captured image (unless we extend media stream API). So the preview image has to be sent to the callback here. When there are multiple media streams attached to one camera source, I don't know how chrome decides the output resolution of the camera source. To have the optimal performance, the camera app should only use one MediaStreamVideoTrack and the resolution should be one of the sizes from getSupportedPreviewSizes. Usually the camera ISP can have two simultaneous outputs - one smaller image for preview and one full size for JPEG.
Dave. Please take a look. Since there's no c++ wrappers generator, I'll address most comments before writing c++ wrappers.
lgtm: I think this is good enough to start implementing to. I did have a couple of comments. Thanks for your patience. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:47: * PPB_CameraCapabilities_Private</code> and the caller should not free it. Ah, I see! That sounds like a good approach in this case. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:66: void GetSupportedJpegSizes( I assume you expect this resource to have its data completely on the plugin-side, so these Get functions don't have to do IPC or anything. Is that right? Sounds like a good approach to me... (otherwise these should have completion callbacks and be async; but I think we can avoid that) https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:224: [in] PPB_ImageCapture_Private_JpegCallback jpeg_callback, You might want to consider modeling this after PPB_Messaging::RegisterMessageHandler and PPP_MessageHandler... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_mess... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppp_mess... ...the main advantage being consistency. That also makes it easy to specify a different thread to receive events, but you probably don't care about that. I also think it's helpful that it has a void* user_data pointer passed in and out, to make it easy for the plugin to have something like an object that handles the callbacks without needing to map from PP_Resource. But what you have may be good enough for a Private API. Speaking of threads... I'm guessing your intent is that these callbacks are invoked on the same thread that CaptureStillImage is called on? It would be good to document that.
By the way... what's the plan for displaying a continuous preview... i.e., so that the person taking the picture has a near-real-time video display of what the camera sees? I guess that's just using the existing MediaStreamVideoTrack stuff? https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out, size_is(array_size)] PP_Size[] preview_sizes); By the way, in case it isn't obvious... you'll want to just allocate that array (really probably a vector) when the internal proxy object is created, and never delete it. You might note that when a PPB_CameraCapabilities_Private is deleted, the arrays resulting from GetSupported*Sizes are no longer valid. You don't want to get in to wacky thread race conditions by being too clever about the array lifetime.
Looking good. Please check a bug in C interface. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:1: /* Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: copyright disclaimer no longer contains "(c)" in latest coding convention. http://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:1: /* Copyright (c) 2014 The Chromium Authors. All rights reserved. dittos https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:67: We do not have *_ErrorCallback when HW is broken or other unexpected case (like OOM). The API may not know the error immediately when CaptureStillImage returns. Do we need the error callback? Can we merge it with one of the existing 3 callbacks? https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:54: * GetSupportedPreviewSizes( nit: spurious newline? https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:67: struct PP_Size* preview_sizes[]); From the comments, the returned array should belong to PPB_CameraCapabilities_Private, but from the generated prototype, it actually means the caller owns and prepares "an array of pointers" (caller prepares a PP_Size* []), then pass the array of pointer to this API.) It's not what we want. I guess you want to generate something like: const struct PP_Size **preview_sizes Then the caller only needs to prepare a pointer. (Otherwise, C++ can pass reference to an array in return value, but I doubt the idl generator can do that.)
On 2014/08/11 08:32:36, Justin Chuang wrote: > I guess you want to generate something like: > > const struct PP_Size **preview_sizes > > Then the caller only needs to prepare a pointer. > Or it's easier to return struct PP_Size *. One drawback like Owen mentioned is that output values are not all in parameters.
All done. PTAL. FYI. Autofocus will be added in a future CL. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:1: /* Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/11 08:32:35, Justin Chuang wrote: > nit: copyright disclaimer no longer contains "(c)" in latest coding convention. > > http://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:47: * PPB_CameraCapabilities_Private</code> and the caller should not free it. On 2014/08/08 21:13:05, dmichael (OOO 8.9-8.17) wrote: > Ah, I see! That sounds like a good approach in this case. Acknowledged. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out, size_is(array_size)] PP_Size[] preview_sizes); On 2014/08/08 21:23:07, dmichael (OOO 8.9-8.17) wrote: > By the way, in case it isn't obvious... you'll want to just allocate that array > (really probably a vector) when the internal proxy object is created, and never > delete it. You might note that when a PPB_CameraCapabilities_Private is deleted, > the arrays resulting from GetSupported*Sizes are no longer valid. Done. I ａｄｄｅｄ the comments. > > You don't want to get in to wacky thread race conditions by being too clever > about the array lifetime. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:66: void GetSupportedJpegSizes( On 2014/08/08 21:13:05, dmichael (OOO 8.9-8.17) wrote: > I assume you expect this resource to have its data completely on the > plugin-side, so these Get functions don't have to do IPC or anything. Is that > right? Sounds like a good approach to me... Yes. The data is completely on the plugin-side and IPC is not required. > > (otherwise these should have completion callbacks and be async; but I think we > can avoid that) https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:1: /* Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/11 08:32:35, Justin Chuang wrote: > dittos Done. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_private.idl (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:67: On 2014/08/11 08:32:35, Justin Chuang wrote: > We do not have *_ErrorCallback when HW is broken or other unexpected case (like > OOM). > The API may not know the error immediately when CaptureStillImage returns. > > Do we need the error callback? Can we merge it with one of the existing 3 > callbacks? I added an error callback to Create for image capture. For the error callback notifying camera is disconnected or disabled, I'll add it in the future patchset. https://codereview.chromium.org/391323002/diff/280001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_private.idl:224: [in] PPB_ImageCapture_Private_JpegCallback jpeg_callback, On 2014/08/08 21:13:05, dmichael (OOO 8.9-8.17) wrote: > You might want to consider modeling this after > PPB_Messaging::RegisterMessageHandler and PPP_MessageHandler... > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_mess... > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppp_mess... Do you mean something like my patchset #3? https://codereview.chromium.org/391323002/diff/140001/ppapi/api/private/ppp_i... Or do you mean I should have a single HandleMessage to handle all different kinds of callbacks? I assume you meant the former. I changed to the current code so the plugin can just implement the callbacks they need. Also, several other classes use the pattern like this. Ex: PPB_Audio_Callback. I'll submit this first and discuss with you after you are back from OOO. > ...the main advantage being consistency. That also makes it easy to specify a > different thread to receive events, but you probably don't care about that. I > also think it's helpful that it has a void* user_data pointer passed in and out, > to make it easy for the plugin to have something like an object that handles the > callbacks without needing to map from PP_Resource. > > But what you have may be good enough for a Private API. > > Speaking of threads... I'm guessing your intent is that these callbacks are > invoked on the same thread that CaptureStillImage is called on? It would be good > to document that. All good points. I added user_data and message_loop. CaptureStillImage callbacks will be called on the thread of message_loop. I also added comments about threading. https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:54: * GetSupportedPreviewSizes( On 2014/08/11 08:32:35, Justin Chuang wrote: > nit: spurious newline? This is auto-generated. We should not change the file manually. I don't want to fix IDL generator. :) https://codereview.chromium.org/391323002/diff/280001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:67: struct PP_Size* preview_sizes[]); On 2014/08/11 08:32:35, Justin Chuang wrote: > From the comments, the returned array should belong to > PPB_CameraCapabilities_Private, but from the generated prototype, it actually > means the caller owns and prepares "an array of pointers" (caller prepares a > PP_Size* []), then pass the array of pointer to this API.) It's not what we > want. > > I guess you want to generate something like: > > const struct PP_Size **preview_sizes > > Then the caller only needs to prepare a pointer. > > (Otherwise, C++ can pass reference to an array in return value, but I doubt the > idl generator can do that.) I found idl generator did not support this. I've changed to a null-terminated array of pointer to PP_Size. It's similar to PPP_Flash_BrowserOperations.GetSitesWithData(). https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/private/...
If everything looks good, I'll add C++ wrapper in the next patchset. There's no C++ wrapper generator. I want to add it after other files are approved by the reviewers.
https://codereview.chromium.org/391323002/diff/300001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/300001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] PP_Size[] preview_sizes); As we discussed, [out] PP_Size[] (that is, PP_Size* preview_sizes[]) can be used. I'll use it for this CL and add array_size back. I'll also upload another CL to make IDL generator generate PP_Size**. http://www.eskimo.com/~scs/cclass/notes/sx10f.html
I have no more comments from my side. (Leave l_g_t_m to others because I don't really understand the MessageLoop thing) go go go https://codereview.chromium.org/391323002/diff/300001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/300001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:52: [out] PP_Size[] preview_sizes); On 2014/08/12 10:16:42, wuchengli wrote: > As we discussed, [out] PP_Size[] (that is, PP_Size* preview_sizes[]) can be > used. I'll use it for this CL and add array_size back. I'll also upload another > CL to make IDL generator generate PP_Size**. > > http://www.eskimo.com/~scs/cclass/notes/sx10f.html Lesson learned. Generating double pointers can be more readable. Besides, I think it's good enough to return NULL-terminated array.
PTAL. I added c++ wrappers and removed message loop. Message loop looks complicated to use. Let's add it when we really need to use another thread to invoke the callbacks.
Haven't finished C++ review. Some nits for C parts https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:34: Paired Close()? https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:56: * @param[out] A NULL-terminated array of pointer to <code>PP_Size</code> Is it still NULL-terminated? https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:75: * @param[out] A NULL-terminated array of pointer to <code>PP_Size</code> dittos https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:34: PP_Resource Create([in] PP_Instance instance); Paired Close()? https://codereview.chromium.org/391323002/diff/340001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:96: struct PP_Size* jpeg_sizes[]); Prefer 'const struct PP_Size*...' Since the array is owned by PP_*Capabilities_Preview
> Since the array is owned by PP_*Capabilities_Preview my typo s/_Preview/_Private/
All done. PTAL. https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... File ppapi/api/private/ppb_camera_capabilities_private.idl (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:34: On 2014/08/14 03:23:46, Justin Chuang wrote: > Paired Close()? This is a plugin-side immutable object. No need to Close. Every pepper resource needs a Create function. https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:56: * @param[out] A NULL-terminated array of pointer to <code>PP_Size</code> On 2014/08/14 03:23:46, Justin Chuang wrote: > Is it still NULL-terminated? Oops. I forgot to update the comments. Done. https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_c... ppapi/api/private/ppb_camera_capabilities_private.idl:75: * @param[out] A NULL-terminated array of pointer to <code>PP_Size</code> On 2014/08/14 03:23:46, Justin Chuang wrote: > dittos Done. https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_i... File ppapi/api/private/ppb_image_capture_config_private.idl (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/api/private/ppb_i... ppapi/api/private/ppb_image_capture_config_private.idl:34: PP_Resource Create([in] PP_Instance instance); On 2014/08/14 03:23:46, Justin Chuang wrote: > Paired Close()? No need to close. This object is just the settings and can be directly destructed. https://codereview.chromium.org/391323002/diff/340001/ppapi/c/private/ppb_cam... File ppapi/c/private/ppb_camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/340001/ppapi/c/private/ppb_cam... ppapi/c/private/ppb_camera_capabilities_private.h:96: struct PP_Size* jpeg_sizes[]); On 2014/08/14 03:23:46, Justin Chuang wrote: > Prefer 'const struct PP_Size*...' > Since the array is owned by PP_*Capabilities_Preview All c headers are automatically generated. We should not change the code manually. I don't want to touch the IDL generator unless it has a bug.
https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... File ppapi/cpp/private/camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:40: /// frames to a new video track, which will be consumed by Javascript. Wrong comments. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:57: /// image capture capabilities resource. remove this @param https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:64: void GetSupportedPreviewSizes(int32_t* array_size, Size** preview_sizes); Not sure if it's better to replace both args with a "std::vector<Size>* preview_sizes"? It's more C++. With the current prototype, you still need to allocate an array of Size, can't directly copy the pointer to PP_Size from C layer. So either way, the costs are the same. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:70: /// image capture capabilities resource. ditto, redundant @param https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:77: void GetSupportedJpegSizes(int32_t* array_size, Size** jpeg_sizes); Ditto https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:86: /// CameraCapabilities_Private</code> resource, otherwise false. s/True/true/ (for C++ keyword) https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... File ppapi/cpp/private/image_capture_config_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:42: /// frames to a new video track, which will be consumed by Javascript. Wrong comments https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:60: void GetPreviewSize(struct PP_Size* preview_size); Why not Size here? https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:67: void SetPreviewSize(const struct PP_Size* preview_size); const Size& preview_size https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:74: void GetJpegSize(struct PP_Size* jpeg_size); ditto https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:81: void SetJpegSize(const struct PP_Size* jpeg_size); ditto https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:90: /// ImageCaptureConfig_Private</code> resource, otherwise false. s/True/true/ https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... File ppapi/cpp/private/image_capture_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:11: #include "ppapi/cpp/private/image_capture_config_private.h" need *capabilities_private.h? https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:21: /// 1. Get a ImageCapture_Private object by Create(). No Create() in C++. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:36: /// Creates a ImageCapture_Private resource. s/a/an/ https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:61: ImageCapture_Private(const ImageCapture_Private& other); nit: Supporting copy constructor for this class may be a headache. Same for the rest constructors below. I don't know how to implement them correctly. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:87: /// image capture resource. redundant @param https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:98: /// image capture resource. redundant @param https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:120: const CompletionCallback& callback); Can replace both arguments with const CompletionCallbackWithOutput<ImageCaptureConfig_Private>& callback More C++-ish, but it will copy the *Config* object. Overheads may be neglectable. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:134: const CompletionCallback& callback); Can replace both arguments with const CompletionCallbackWithOutput<CameraCapabilities_Private>& callback More C++-ish, but it will copy the *Capabilities* object. Overheads may be neglectable. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:180: const PPB_ImageCapture_Private_ShutterCallback& shutter_callback, No need to use 'const &' I think this C++ API can be the same as the C one. It just passes the value of a function pointer of 4/8 bytes, no performance penalty. Another problem is that caller needs to prepare "an instance of *_Callback" first to be able to pass as a reference. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:190: /// otherwise. s/True/true/
All done. PTAL. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... File ppapi/cpp/private/camera_capabilities_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:40: /// frames to a new video track, which will be consumed by Javascript. On 2014/08/14 16:33:44, Justin Chuang wrote: > Wrong comments. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:57: /// image capture capabilities resource. On 2014/08/14 16:33:44, Justin Chuang wrote: > remove this @param Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:64: void GetSupportedPreviewSizes(int32_t* array_size, Size** preview_sizes); On 2014/08/14 16:33:44, Justin Chuang wrote: > Not sure if it's better to replace both args with a "std::vector<Size>* > preview_sizes"? It's more C++. > > With the current prototype, you still need to allocate an array of Size, can't > directly copy the pointer to PP_Size from C layer. So either way, the costs are > the same. Good suggestion. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:70: /// image capture capabilities resource. On 2014/08/14 16:33:44, Justin Chuang wrote: > ditto, redundant @param Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:77: void GetSupportedJpegSizes(int32_t* array_size, Size** jpeg_sizes); On 2014/08/14 16:33:44, Justin Chuang wrote: > Ditto Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/camer... ppapi/cpp/private/camera_capabilities_private.h:86: /// CameraCapabilities_Private</code> resource, otherwise false. On 2014/08/14 16:33:44, Justin Chuang wrote: > s/True/true/ (for C++ keyword) Done. Some ppapi classes use True and some use true. The classes that use true are more. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... File ppapi/cpp/private/image_capture_config_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:42: /// frames to a new video track, which will be consumed by Javascript. On 2014/08/14 16:33:45, Justin Chuang wrote: > Wrong comments Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:60: void GetPreviewSize(struct PP_Size* preview_size); On 2014/08/14 16:33:45, Justin Chuang wrote: > Why not Size here? Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:67: void SetPreviewSize(const struct PP_Size* preview_size); On 2014/08/14 16:33:44, Justin Chuang wrote: > const Size& preview_size Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:74: void GetJpegSize(struct PP_Size* jpeg_size); On 2014/08/14 16:33:44, Justin Chuang wrote: > ditto Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:81: void SetJpegSize(const struct PP_Size* jpeg_size); On 2014/08/14 16:33:45, Justin Chuang wrote: > ditto Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_config_private.h:90: /// ImageCaptureConfig_Private</code> resource, otherwise false. On 2014/08/14 16:33:45, Justin Chuang wrote: > s/True/true/ Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... File ppapi/cpp/private/image_capture_private.h (right): https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:11: #include "ppapi/cpp/private/image_capture_config_private.h" On 2014/08/14 16:33:45, Justin Chuang wrote: > need *capabilities_private.h? Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:21: /// 1. Get a ImageCapture_Private object by Create(). On 2014/08/14 16:33:46, Justin Chuang wrote: > No Create() in C++. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:36: /// Creates a ImageCapture_Private resource. On 2014/08/14 16:33:45, Justin Chuang wrote: > s/a/an/ Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:61: ImageCapture_Private(const ImageCapture_Private& other); On 2014/08/14 16:33:45, Justin Chuang wrote: > nit: Supporting copy constructor for this class may be a headache. Same for the > rest constructors below. > I don't know how to implement them correctly. Removed. I checked and many ppapi cpp classes do not have copy constructor. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:87: /// image capture resource. On 2014/08/14 16:33:45, Justin Chuang wrote: > redundant @param Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:98: /// image capture resource. On 2014/08/14 16:33:45, Justin Chuang wrote: > redundant @param Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:120: const CompletionCallback& callback); On 2014/08/14 16:33:46, Justin Chuang wrote: > Can replace both arguments with > > const CompletionCallbackWithOutput<ImageCaptureConfig_Private>& callback > > More C++-ish, but it will copy the *Config* object. Overheads may be > neglectable. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:134: const CompletionCallback& callback); On 2014/08/14 16:33:45, Justin Chuang wrote: > Can replace both arguments with > > const CompletionCallbackWithOutput<CameraCapabilities_Private>& callback > > More C++-ish, but it will copy the *Capabilities* object. Overheads may be > neglectable. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:180: const PPB_ImageCapture_Private_ShutterCallback& shutter_callback, On 2014/08/14 16:33:45, Justin Chuang wrote: > No need to use 'const &' > I think this C++ API can be the same as the C one. It just passes the value of a > function pointer of 4/8 bytes, no performance penalty. > Another problem is that caller needs to prepare "an instance of *_Callback" > first to be able to pass as a reference. Done. https://codereview.chromium.org/391323002/diff/360001/ppapi/cpp/private/image... ppapi/cpp/private/image_capture_private.h:190: /// otherwise. On 2014/08/14 16:33:46, Justin Chuang wrote: > s/True/true/ Done.
lgtm lgtm May need to *_api.h https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... File ppapi/thunk/ppb_image_capture_config_private_thunk.cc (right): https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... ppapi/thunk/ppb_image_capture_config_private_thunk.cc:13: #include "ppapi/thunk/ppb_image_capture_config_api.h" Missing *_api.h files.
lgtm May need to *_api.h
Submitting. https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... File ppapi/thunk/ppb_image_capture_config_private_thunk.cc (right): https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... ppapi/thunk/ppb_image_capture_config_private_thunk.cc:13: #include "ppapi/thunk/ppb_image_capture_config_api.h" On 2014/08/15 09:39:38, Justin Chuang wrote: > Missing *_api.h files. I'll leave those to you. :)
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/391323002/400001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
On 2014/08/17 07:07:09, wuchengli wrote: > Submitting. > > https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... > File ppapi/thunk/ppb_image_capture_config_private_thunk.cc (right): > > https://codereview.chromium.org/391323002/diff/400001/ppapi/thunk/ppb_image_c... > ppapi/thunk/ppb_image_capture_config_private_thunk.cc:13: #include > "ppapi/thunk/ppb_image_capture_config_api.h" > On 2014/08/15 09:39:38, Justin Chuang wrote: > > Missing *_api.h files. > I'll leave those to you. :) AHA... No problem.
Message was sent while issue was closed.
Committed patchset #10 (400001) as 290186 |
