980290 - [Camera] Bytes and pixels shouldn't be used interchangeably

Status: RESOLVED FIXED

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

Description

[Wilson Page [:wilsonpage]]

Throughout the app we are comparing bytes and pixels like for like. We need to make clear conversions between the two.

Comment 1

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (camera-new-features)

Please note, this patch is dependent upon 980599 and will need to land in camera-new-features *after* 980599 has landed.

Comment 2

[David Flanagan [:djf]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

r- because the function returns the estimated # of bits in the jpeg file, not the number of bytes!  So your numbers are 8 times too big.

See also my comments on github.

Comment 3

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

David: Updated to address your comments. I moved `CONFIG_AVG_JPEG_COMPRESSION_RATIO` to be a build-time configurable setting *now* instead of waiting until `config/app.js` is build-time configurable. We can move it out later if we want to. Please note, I am waiting until mega-patch (980599) lands before rebasing against camera-new-features.

Comment 4

[Justin D'Arcangelo [:justindarc]]

David: I'm not sure I understand why this is R- from the comments on the PR? I replied back on one of them to indicate that the default compression ratio of 24 was what I had gathered as a result of taking *actual* photos with the device. Would a comment explaining that suffice?

Comment 5

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

David: Updated PR once more to adjust compression ratio for a *WORST CASE SCENARIO* of my findings against actual photos taken with the camera app. I also clarified these findings in the comments so there would be no confusion as to their validity. The previous compression ratio of 24 was an average of 10 *random* photos from my camera. This has now be lowered to 16 if I only take into account the 5 *largest* photo files on my SD card, which should hopefully provide a better worst case prediction as to the resulting file size. Although, from my research on this, it appears there is no algorithm that can accurately assume the anticipated size of a JPEG image without first knowing the contents of the image. Even still, this value is a build-time configurable option and can be tweaked as necessary.

Comment 6

[David Flanagan [:djf]]

Looking at some jpegs I have on my laptop right now, I see one with pixel size: 3456 x 2304 for a total number of pixels: 7962624.  It has a file size of 2995821.  So the conversion factor is .376 bytes/pixel.

Looking at another one: 480x640 = 307200 pixels with a file size of 91049.  That is a factor of .296

If I pass 480,640 those numbers into your estimateJpegFileSize() function, I get 480*640*24/8/16, or 57,600.  That is smaller than the actual file size of 91049 by a substantial margin, and that was your worst case number, something is clearly wrong.

I just want a function that makes plausible guesses for a file size given an image size.  I would have just multiplied width times height times a bytes/pixel conversion factor. If you want to expression the conversion factor as a compression ratio or a bits/pixel value or something that is fine, but just ensure that the return value of your function returns a realisitic file size.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

r- even with the conversion factor reduced to 16. Your function is still returning file sizes that are too small.  See the comment above.

Comment 8

[David Flanagan [:djf]]

To be fair, my test photos are not from a FxOS phone, so perhaps they are using some unrealistically high compression ratio.

Comment 9

[Justin D'Arcangelo [:justindarc]]

Ok, I guess the photos I've been taking maybe don't have as much complexity as yours do, so yours makes a better worst-case scenario. Based on your sample data in Comment 6, I'm getting a compression ratio of 8.06 (3496 * 2304 * 24 / 8 = 24,164,352 bytes raw bitmap, divided by your actual JPEG size of 2,995,821 gives me a compression ratio of around 8.06). So, if I change the default JPEG compression ratio to 8, would that pass review?

FYI: The photos I was using came from my Helix.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Also, 24(bpp) / 8(bits) / 8(compression ratio) = 0.375 which is comparable to the 0.376bytes/px you're seeing in your sample photo. So, I think a compression ratio of 8 fits the samples you've provided. However, maybe it is possible that the Helix running FxOS is compressing JPEG's from the camera much higher than we thought? I was surprised at how incredibly small the JPEGs were on the SD card when I looked at them.

Comment 11

[David Flanagan [:djf]]

Yes, r+ if you change the 16 to an 8. 

My 8mp sample photo described above was from someone else's digital camera. I have no idea what type it was, but it must have been using higher jpeg quality than we do in our camera.  When I tried 5mp photos on the nexus 4, I got .22 bytes per pixel (for a photograph of printed text) instead of .376. Your value of 16 would work out to less than .2 bytes per pixel and would have failed in that case. 8 seems very safe. We could probably even go with 10, but let's do 8 for now.  

(And let's ask Mike how the default JPEG quality ratio was chosen, since I agree that it seems a little lower than I would expect. Maybe that is something we should adjust...)

Comment 12

[Justin D'Arcangelo [:justindarc]]

Will do! Thank you for your patience in helping to get this right! I will ping Mike about the JPEG quality setting. He seemed to indicate to me the other day that it could be easily bumped up if necessary. We might want to open a new bug for it.

Comment 13

[David Flanagan [:djf]]

When I take a 5mp photo of the same printed page using my Android phone (Galaxy Nexus), I get .36 bytes per pixel.  So I do think that maybe our quality setting is lower than the competition. Maybe something we should make configurable in the future.  I've filed bug 981318 to investigate.  Not a 1.4 blocker, though.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

Changed `CONFIG_AVG_JPEG_COMPRESSION_RATIO` to `8` as requested.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8387192 [details] [review]
pull-request (camera-new-features)

Looks good.

You actually had my r+ for this in comment 11 and did not actually have to ask for it again.

Comment 16

[Diego Marcos [:dmarcos]]

Landed in camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/2ec7aadf4f26e5d1621441812e48b2edd48035ce

Comment 17

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530