Closed Bug 807058 Opened 12 years ago Closed 11 years ago

Camera - expose thumbnail size capability and control property

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
blocking-b2g hd+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g18 --- wontfix
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 9 obsolete files)

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.
Sample patch for camera.js forthcoming.
Attachment #676792 - Flags: feedback?(dflanagan)
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}]
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)
Builds, works--tested with camera.js to be attached next.
Attachment #822486 - Attachment is obsolete: true
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)
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 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+
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+
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+
Attachment #823637 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d7322c4e291b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #822615 - Flags: feedback?(dmarcos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: