Camera - expose thumbnail size capability and control property

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

({dev-doc-needed})

unspecified
ARM
Gonk (Firefox OS)
dev-doc-needed

Firefox Tracking Flags

(blocking-b2g:hd+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g18 wontfix, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

Attachments

(3 attachments, 9 obsolete attachments)

3.40 KB, patch
Details | Diff | Splinter Review
38.52 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
33.35 KB, patch
Details | Diff | Splinter Review
Add .thumbnailSizes to CameraCapabilities, and .thumbnailSize to either CameraControl or the takePicture() options--haven't decided which, yet.
Created attachment 676792 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option

Sample patch for camera.js forthcoming.
Attachment #676792 - Flags: feedback?(dflanagan)
Created attachment 676793 [details] [diff] [review]
Sample camera.js patch to illustrate use of property
Attachment #676793 - Flags: feedback?(dflanagan)
Attachment #676793 - Flags: feedback?(dale)
Comment on attachment 676792 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option

This was simple enough to implement, and all of the code has been used/tested/exercised already.
Attachment #676792 - Flags: feedback?(dale)
Comment on attachment 676793 [details] [diff] [review]
Sample camera.js patch to illustrate use of property

too bad the driver doesn't honor it! Thanks for coding this up, Mike!
Attachment #676793 - Flags: feedback?(dflanagan) → feedback+
Comment on attachment 676792 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option

I'm not a gecko hacker, but this looks good to me.
Attachment #676792 - Flags: feedback?(dflanagan) → feedback+
Attachment #676793 - Flags: feedback?(dale) → feedback+
Attachment #676792 - Flags: feedback?(dale) → feedback+
Blocks: 866790
I don't see how this blocked bug 866790; that bug has to do with a disconnect between the range of supported preview sizes (usually 3:2) and the range of supported picture sizes (usually 4:3).

Exposing the thumbnail size should only affect the size of the thumbnail embedded in the EXIF header.
Flags: needinfo?(dale)
ah, I will update the other bug then, matching the preview aspect ratio didnt help
Flags: needinfo?(dale)
Yeh was wrong about this one, not blocking
No longer blocks: 866790
Here are the thumbnail sizes supported by Hamachi:

camera.capabilities.thumbnailSizes=[{width:1280, height:720}, {width:864, height:480}, {width:800, height:480}, {width:768, height:432}, {width:720, height:480}, {width:640, height:480}, {width:576, height:432}, {width:512, height:384}, {width:480, height:320}, {width:432, height:240}, {width:384, height:288}, {width:352, height:288}, {width:320, height:240}, {width:240, height:160}, {width:176, height:144}, {width:0, height:0}]
Created attachment 821935 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v2

Updated for v1.2 (lands against aurora).
Attachment #676792 - Attachment is obsolete: true
Hamachi's default thumbnail size is 512x384.
Setting the thumbnail size to the maximum reported by hamachi {1280x720} causes the camera driver to implicitly change the thumbnail size to something with the same aspect ratio as the picture to be captured--in this case, 960x720:

  QCameraHWI_Still: android::status_t android::QCameraStream_Snapshot::configSnapshotDimension(cam_ctrl_dimension_t*): Image Sizes before set parm call: main: 2048x1536 thumbnail: 1280x720
    ...
  QCameraHWI_Still: android::status_t android::QCameraStream_Snapshot::configSnapshotDimension(cam_ctrl_dimension_t*): Image Sizes: main: 2048x1536 thumbnail: 960x720

This is not one of the reported supported thumbnail sizes in comment 9. When a picture is taken, encoding fails with the following error:

  QCameraHWI_Still: void android::snapshot_jpeg_cb(jpeg_event_t, void*): Error in thumbnail encoding (event: 4) : X !!!

Not sure if this is related, but |adb shell cat /proc/kmsg| shows:

<3>[ 1611.146938] mpd_ppp: error scaling when size is 1!
<3>[ 1611.706641] 
<3>[ 1611.706648] mdp_ppp_verify_req(): Error in Line 1212
<3>[ 1611.706661] mdp_ppp: invalid image!
<3>[ 1611.706838] 
<3>[ 1611.706841] mdp_ppp_verify_req(): Error in Line 1205
<3>[ 1611.706851] mdp_ppp: invalid image!
<3>[ 1611.706959] 
<3>[ 1611.706963] mdp_ppp_verify_req(): Error in Line 1205
<3>[ 1611.706971] mdp_ppp: invalid image!
<3>[ 1611.707083] 
<3>[ 1611.707086] mdp_ppp_verify_req(): Error in Line 1212
<3>[ 1611.707094] mdp_ppp: invalid image!
Manually selecting a thumbnail size of 640x480 results in a proper JPEG encoded with a proper thumbnail.

hamachi, at least, seems to require that the chosen thumbnail size be of the same aspect ratio as the picture to be captured.
To ensure that Camera and Gallery apps don't OOM frequently, it is very important that our photos have thumbnails that are larger than the device's screen size (in physical pixels, not CSS pixels).  

This seems to be the case for all of our 320x480 devices, but we should check this on the hd device.  If the default thumbnail is not big enough on that device, then this bug will block 928614.
These are the logs that my hamachi is producing when taking pictures:

E/QCameraHWI_Parm(  144): requested picture size 2048 x 1536
E/QCameraHWI_Parm(  144): requested jpeg thumbnail size 320 x 240

Firmware issue? How can I tell the version of my firmware?
Flags: needinfo?(mhabicher)
This is going to feed into bug 928614.
blocking-b2g: --- → hd?
Flags: needinfo?(mhabicher)
Blocks: 931093
Created attachment 822486 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v3
Attachment #821935 - Attachment is obsolete: true
Created attachment 822608 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v4

Builds, works--tested with camera.js to be attached next.
Attachment #822486 - Attachment is obsolete: true
Created attachment 822615 [details] [diff] [review]
Sample camera.js patch to illustrate/text use of new properties

Diego, if you feel comfortable building/flashing your own gecko, you can try attachment 822608 [details] [diff] [review] with this patch. It will select the 3MP image size and choose an appropriate thumbnail.
Attachment #676793 - Attachment is obsolete: true
Attachment #822615 - Flags: feedback?(dmarcos)
Yes I can flash my own gecko :). I will give it a shot probably tomorrow. I've been thinking about the API design (I really care about these things). What about having also a convenience method 'configure'? The method could return feedback to the user about invalid values or options. Below an example on two ways of configuring the camera:

Option A:

camera.flash = "auto";
camera.pictureSize.width = 1280;
camera.pictureSize.height = 720;
camera.thumbnailSize.width = 640;
camera.thumbnailSize.height = 360;

camera.takePicture();

Option B:

var error = camera.configure({ 
  pictureSize : {
    width: 1280,
    height: 720
  },
  thumbnailSize : {
    width: 640,
    height: 360
  },
  flash: "auto"
});

camera.takePicture();
I'm looking at the code of the gallery and I see calls to:

camera.capabilities.previewSizes

How is this call different than camera.capabilities.thumbnailSizes?
Flags: needinfo?(mhabicher)
I applied your patch and it works as expected. 

I also attached a patch for 931093 based on yours. Are you sending yours for review?
(In reply to Diego Marcos from comment #20)
> Yes I can flash my own gecko :). I will give it a shot probably tomorrow.
> I've been thinking about the API design (I really care about these things).
> What about having also a convenience method 'configure'? The method could
> return feedback to the user about invalid values or options. Below an
> example on two ways of configuring the camera:
> 
> Option A:
> 
> camera.flash = "auto";
> camera.pictureSize.width = 1280;
> camera.pictureSize.height = 720;
> camera.thumbnailSize.width = 640;
> camera.thumbnailSize.height = 360;
> 
> camera.takePicture();
> 
> Option B:
> 
> var error = camera.configure({ 
>   pictureSize : {
>     width: 1280,
>     height: 720
>   },
>   thumbnailSize : {
>     width: 640,
>     height: 360
>   },
>   flash: "auto"
> });
> 
> camera.takePicture();

This looks like something that is much easier to write in JS than to expose as a DOM API. :)

(In reply to Diego Marcos from comment #21)
> I'm looking at the code of the gallery and I see calls to:
> 
> camera.capabilities.previewSizes
> 
> How is this call different than camera.capabilities.thumbnailSizes?

.previewSizes = the sizes available for the viewfinder on the screen
.thumbnailSizes = the sizes available for stuffing into the EXIF header
Flags: needinfo?(mhabicher)
Created attachment 823381 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v5 [aurora]
Attachment #822608 - Attachment is obsolete: true
Created attachment 823385 [details] [diff] [review]
Part 1: Camera - Expose thumbnailSizes capability and thumbnailSize picture option
Attachment #823385 - Flags: review?(sotaro.ikeda.g)
Created attachment 823387 [details] [diff] [review]
Part 2: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option
Attachment #823387 - Flags: review?(ehsan)
Attachment #823387 - Attachment description: Part 1: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option → Part 2: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option

Comment 27

4 years ago
Comment on attachment 823387 [details] [diff] [review]
Part 2: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option

Review of attachment 823387 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I'm the right reviewer for this patch.
Attachment #823387 - Flags: review?(ehsan)
Attachment #823381 - Attachment description: Expose thumbnailSizes capability and thumbnailSize picture option, v5 → Expose thumbnailSizes capability and thumbnailSize picture option, v5 [aurora]
Comment on attachment 823385 [details] [diff] [review]
Part 1: Camera - Expose thumbnailSizes capability and thumbnailSize picture option

Looks good.
Attachment #823385 - Flags: review?(sotaro.ikeda.g) → review+
Created attachment 823637 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v5 [b2g18_v1.1.0hd]

b2g18 version of this patch, lands against branch v1.1.0hd
Needed for hd+ bug 928614
blocking-b2g: hd? → hd+
Comment on attachment 823387 [details] [diff] [review]
Part 2: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option

Johnny, time for a quick review? Or nominate someone else to do it?
Attachment #823387 - Flags: review?(jst)
Comment on attachment 823387 [details] [diff] [review]
Part 2: DOM API - Expose thumbnailSizes capability and thumbnailSize picture option

It saddens me that this is still using XPConnect and not the new bindings, but I also don't want to block this work on converting. Also, it's likely that you'd need some features (returning arrays, say) which we don't quite yet have support for in the new bindings.

r=jst
Attachment #823387 - Flags: review?(jst) → review+
Created attachment 824751 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v6 [m-c], r=sotaro,jst

Latest patch update with try-server feedback; carrying r+ forward from attachment 823385 [details] [diff] [review] and attachment 823387 [details] [diff] [review].

https://tbpl.mozilla.org/?tree=Try&rev=682d9dfb2fea

b2g18-ported patch to follow.
Attachment #823381 - Attachment is obsolete: true
Attachment #823385 - Attachment is obsolete: true
Attachment #823387 - Attachment is obsolete: true
Attachment #824751 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/d7322c4e291b
Created attachment 824846 [details] [diff] [review]
Expose thumbnailSizes capability and thumbnailSize picture option, v6 [b2g18 port]

Builds locally, but nowhere to push it to try.
Attachment #823637 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d7322c4e291b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/eebe400c1021
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1d8b95908208
status-b2g18: --- → wontfix
status-b2g-v1.1hd: --- → fixed
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
Keywords: dev-doc-needed
Attachment #822615 - Flags: feedback?(dmarcos)
You need to log in before you can comment on or make changes to this bug.