Closed Bug 981318 Opened 6 years ago Closed 5 years ago

[Camera][Gecko] Check JPEG quality setting of photos

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: djf, Assigned: mikeh)

References

Details

Attachments

(2 files, 2 obsolete files)

Justin and I have noticed that the FirefoxOS camera creates files that are significantly smaller than those created by the Android camera app, and that makes me wonder if we are compressing our JPEG files more than we should.

Mike: could you let us know what JPEG quality setting the camera is using by default, how that default was chosen, and whether there is an API we can use to change the default?

Peter: are you satisfied with the quality of the images the camera is taking? Do we have a good balance between conserving space on the user's sdcard and maximizing image quality?
Flags: needinfo?(pla)
Flags: needinfo?(mhabicher)
Justin: do you see anything in the AOSP camera source code to indicate what jpeg quality setting is used in the Android camera?
Flags: needinfo?(jdarcangelo)
(In reply to David Flanagan [:djf] from comment #0)
> 
> Mike: could you let us know what JPEG quality setting the camera is using by
> default, how that default was chosen, and whether there is an API we can use
> to change the default?

I think the last time I looked it was "70%"--whatever that means--but I could be wrong, and it may very well be different for different devices.

If we think we need this, I'm happy to expose it.
Flags: needinfo?(mhabicher)
Here's a patch you can apply to Gecko to expose the read/write JPEG image quality setting. It's call pictureQuality and it hangs off of CameraControl. It takes fractional values, 0..1.0 -- the lower the setting value, the more compression/less image quality.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=e0d7ccb0e512&showall=1
Attachment #8388117 - Flags: feedback?(jdarcangelo)
Attachment #8388117 - Flags: feedback?(dflanagan)
Thanks Mike! I'm assuming this value correlates with the JPEG "quality" setting in most image editing applications where it usually ranges from 0-100. I'll give it a try.
(In reply to Justin D'Arcangelo from comment #4)
>
> Thanks Mike! I'm assuming this value correlates with the JPEG "quality"
> setting in most image editing applications where it usually ranges from
> 0-100. I'll give it a try.

That's exactly it, but I figured there was no need to tie it to JPEG, should we ever wind up supporting other lossy formats.
Thanks, Mike!

Let's not make this a high priority unless someone actually tells us that what we have now is not acceptable.  But good to know we have options here.

Justin: obviously your work on zooming takes priority!
Flags: needinfo?(jdarcangelo)
Comment on attachment 8388117 [details] [diff] [review]
WIP - Expose lossy image quality setting, v1

Belatedly setting feedback+ on this.  Seems like something worth landing in 1.5, once we have build-time configuration set up so that partners can tweak it easily.
Attachment #8388117 - Flags: feedback?(dflanagan) → feedback+
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Summary: [camera] Check JPEG quality setting of photos → [Camera][Gecko] Check JPEG quality setting of photos
David, my sincere apologies for the absurdly long delay in replying to this. :(

I have been noticing a lot that our photos are generally not of a very high quality, so this is definitely something to address.  I'm going to needinfo Amy to do a more thorough analysis of picture quality.
Flags: needinfo?(pla) → needinfo?(amlee)
[Blocking Requested - why for this release]: not really blocking, but nom-ing for 2.1 because this patch is ready to go, including automated tests. The ability to control JPEG image quality is nice to have.
blocking-b2g: --- → 2.1?
Hi, 

I've taken a few photos with the Flame (in HDR On/Off modes) and also looked at the file size in comparison to a Nexus 5.

Overall I think the picture quality is lacking and often images are over exposed. I also compared taking photos with HDR on/off and noticed that the HDR photos IMO look worse. 

Image file size on the flame ranges from 600KB to 1MB in comparison to the Nexus 5 where in default image settings the file sizes ranged from 1MB-2MB. HDR On seemed to create file sizes slightly smaller than having HDR Off except in one instance.

I think we can afford to have less compression in our image quality but I'm not sure if that alone would fix the over exposure issue and HDR On quality. 

I've attached the test shots I took and labeled the ones that were taken in HDR mode.
Flags: needinfo?(amlee)
Mike, if the patch is ready to go let's get it in
Flags: needinfo?(mhabicher)
dhylands and khuey, do you have time to review this? It's pretty boiler-plate. The tl;dr is this exposes a new 0.0..1.0 attribute to JS that allows the Camera app to specify the quality of JPEGs created by the camera driver.

Patch includes tests.
Attachment #8476304 - Flags: review?(khuey)
Attachment #8476304 - Flags: review?(dhylands)
Flags: needinfo?(mhabicher)
Attachment #8388117 - Attachment is obsolete: true
Attachment #8388117 - Flags: feedback?(jdarcangelo)
Assignee: nobody → mhabicher
Comment on attachment 8476304 [details] [diff] [review]
Expose lossy image quality setting, v1.1

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

r=me with comments addressed.

::: dom/camera/DOMCameraControl.cpp
@@ +403,5 @@
> +  MOZ_ASSERT(mCameraControl);
> +
> +  double quality;
> +  aRv = mCameraControl->Get(CAMERA_PARAM_PICTURE_QUALITY, quality);
> +  return quality;

Shouldn't quality be initialized? If the call to Get fails, then you'll wind up returning random data from the stack (which might actually be considered to be a security issue).

::: dom/camera/GonkCameraParameters.cpp
@@ +666,5 @@
> +
> +    case CAMERA_PARAM_PICTURE_QUALITY:
> +      {
> +        // Convert aValue to nearest integer percentage in the range [0..100].
> +        if (aValue < 0) {

nit: This should probably be 0.0 to match the fact that aValue is a double
Attachment #8476304 - Flags: review?(dhylands) → review+
Comment on attachment 8476304 [details] [diff] [review]
Expose lossy image quality setting, v1.1

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

I assume you just want me to sign off on the webidl.  r=me.
Attachment #8476304 - Flags: review?(khuey) → review+
Slight tweak to adjust for a Gonk attribute that is really only 1..100. All tests pass, carrying r+ forward.
Attachment #8476304 - Attachment is obsolete: true
Attachment #8478481 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/10412cc35ca4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
Landed on master prior to 2.1-branching; clearning 2.1-nom as it is not required.
blocking-b2g: 2.1? → ---
You need to log in before you can comment on or make changes to this bug.