Closed Bug 807374 Opened 12 years ago Closed 12 years ago

Camera takes extremely small photos

Categories

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

x86
macOS
defect

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
blocking-basecamp +

People

(Reporter: daleharvey, Assigned: daleharvey)

Details

Attachments

(1 file)

Current photo sizes are 640x480, it is supposed to attempt to take the largest picture size, may be a bug in the code
blocking-basecamp: --- → ?
Assignee: nobody → dale
888 in camera_largestPictureSize: mikeh: supported picture sizes:
890 in anonymous:   0. 2592x1944
890 in anonymous:   1. 2048x1536
890 in anonymous:   2. 1920x1080
890 in anonymous:   3. 1600x1200
890 in anonymous:   4. 1280x768
890 in anonymous:   5. 1280x720
890 in anonymous:   6. 1024x768
890 in anonymous:   7. 800x600
890 in anonymous:   8. 800x480
890 in anonymous:   9. 640x480
890 in anonymous:   10. 352x288
890 in anonymous:   11. 320x240
890 in anonymous:   12. 176x144
Thanks guys.

Are we able to profile the size (MBs) of the file for 1-5?  I think djf mentioned starting with 3 to see how big that is.  

We want to optimize for quality, memory card usage and upload ease for attachments within emails.

Marking blocking.
blocking-basecamp: ? → +
Priority: -- → P3
(In reply to Chris Lee [:clee] from comment #2)
> Thanks guys.
> 
> Are we able to profile the size (MBs) of the file for 1-5?  I think djf
> mentioned starting with 3 to see how big that is.  
> 
> We want to optimize for quality, memory card usage and upload ease for
> attachments within emails.
> 

Those are good things to keep in mind, but my concern isn't the size of the photos on the memory card. It is the decoded size of the photo in memory when displayed by the gallery app.  I fear that if we go bigger than 1600x1200 that we'll have memory and performance problems in the gallery app.

We also need to pay attention to the aspect ratio here.  The 1280x720 choice, for example, is a bizarre ratio for a photograph.  Per our discussion on IRC, I suggest we try out 1600x1200 and fall back to 1024x768 if we need to (or if UX wants us to)

Cc'ing casey for his input on the best size.
The photos I tested at the maximum resolution were around 700K. I dont really love the idea of hardcoding particular resolution and the maximum resolution seems reasonable. How about we test at max res and reduce if we expose any problems?
Attachment #678639 - Flags: review?(dflanagan)
Comment on attachment 678639 [details]
Bug 807374 - Set picture size when taking photos

r+ for the actual one-line fix, but please fix largestPictureSize() to use * instead of +.  Currently it would say that a 100x2 picture is larger than a 20x20 picture (because 102 > 40) even though the 20x20 picture has twice as many pixels.

I'm not at all concerned about the on disk compressed size of the images the camera generates.  What worries me is the size in memory of those images when the gallery displays them.  The 2592x1944 size is 5 megapixel. If gecko uses 4 bytes per pixel (and I don't know if it does) then that's 20mb of RAM for each image.  The gallery keeps three of the images open at a time, and since gecko is slow to release the memory, if you pan quickly, there might be times where 80 or 100mb are required.

Also, I question whether most users will want that many pixels. If the typical use case is to view photos on the camera, then full resolution is much bigger than necessary.  And if the photo is to be emailed to someone, then 5mp will eat up much more of the user's data plan than 1 or 2 mp.

Finally, I kind of doubt that the lens and sensor on our phones are really high enough quality to justify a 5mp image.

Actually, that isn't my final thought...  Do we really want a 'pick the largest' algorithm, or do you need an algorithm that takes aspect ratio into account.  Maybe something like "pick the largest resolution that is not over 2mp and also has an aspect ratio close to 4x3"

Anyway, I'm willing to try it this way, if you want, but I anticipate that the large photos will result in gallery bugs being filed.
Attachment #678639 - Flags: review?(dflanagan) → review+
Hey David, been testing this a little bit, and yes both the camera and the gallery have a hard time not crashing taking full res photos

I dont think aspect ratio should be part of the calculation, it seems like we should trust the camera to report reasonable aspect ratios, 16:9 isnt a particularly common aspect ratio for still photography but it isnt that strange either, I can pick 16:9 on my fujifilm,

How about amending the commit to take into account a maximum MP, we can set it to 1920000 to conveniently pick 1600x1200 for us.
Comment on attachment 678639 [details]
Bug 807374 - Set picture size when taking photos

Implemented a maximum resolution limit for photos
Attachment #678639 - Flags: review+ → review?(dflanagan)
Comment on attachment 678639 [details]
Bug 807374 - Set picture size when taking photos

I like this new approach, and I'm giving an r+ now. But I'm still not satisfied with pickPictureSize.  If I'm understanding the code correctly, if all of the camera's supported formats are bigger than the max, then it will return a size of 0x0.  I suspect that in this case you want to return the smallest image size (or even the first or the last or any element of the array of sizes).
Attachment #678639 - Flags: review?(dflanagan) → review+
Good catch, that is what would have happened, fixed and merged

https://github.com/mozilla-b2g/gaia/commit/434de6e6f6d2b88cad2a3397105aa4870aa367d9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Dale Harvey (:daleharvey) from comment #6)
> Hey David, been testing this a little bit, and yes both the camera and the
> gallery have a hard time not crashing taking full res photos
> 

Are you testing with otoro or unagi?  Which kernel version?
I was testing on the unagi with latest nightly

The Camera app currently shows its filmstrip preview images as full sized images resized in content, there should only ever be 4 in memory at any time but it is something that needs fixed (I filed https://bugzilla.mozilla.org/show_bug.cgi?id=809205)
Note that unagi has a 2/3rds higher-res sensor than otoro (and target HW), so recalibrate for OOMs appropriately.
Reviewed and verified on "Unagi"
build ID:20130103070201

Current photo no longer 640x480, current size now is 1600x1200.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: