Closed
Bug 960756
Opened 10 years ago
Closed 10 years ago
Decouple max camera snapshot size from max gallery image size
Categories
(Firefox OS Graveyard :: Gaia::Gallery, defect)
Tracking
(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)
People
(Reporter: diego, Assigned: diego)
References
Details
(Whiteboard: [caf priority: p3][CR 600508])
Attachments
(3 files, 1 obsolete file)
Currently both are defined by adding a maxImagePixelSize entry to distribution/camera.json (see bug 932556). However, they don't necessarily need to be the same. Typically the camera app needs much more resources to take a snapshot than it takes the gallery to display an image of the same size. So by setting the same max we may be unnecessarily limiting the gallery.
Assignee | ||
Updated•10 years ago
|
blocking-b2g: --- → 1.3?
Assignee | ||
Comment 1•10 years ago
|
||
Hi djf, As part of the bug 942199 investigation I found out my device is taking larger snapshots than it needs to. The problem is when I limit the snapshot size it also limits the gallery size, which I don't want. What do you think of my proposal?
Flags: needinfo?(dflanagan)
Comment 2•10 years ago
|
||
I think it would be a good idea to separate the two configuration options. I'd still caution against raising the gallery limit too high, since we have a long history of OOM crashes in Gallery. (Fixing bug 854795 will go a long way toward making the gallery limit obsolete, however, and that is another reason that we should go ahead and separate them.) Build time configuration options for the camera is an area that needs a lot more thought. We do a lot of run-time configuration and I think we might want to switch to instead having many build-time configuration options. So any change you make here might soon be replaced by some kind of more comprehensive configuration system. But don't let that stop you!
Flags: needinfo?(dflanagan)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dwilson
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [CR 600508]
Updated•10 years ago
|
blocking-b2g: 1.3? → 1.3+
Updated•10 years ago
|
status-b2g-v1.3:
--- → affected
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8363022 -
Flags: review?(dflanagan)
Comment 5•10 years ago
|
||
David is here in Taipei with me. I'll ask him to review this ASAP. Thanks!
Comment 6•10 years ago
|
||
Comment on attachment 8363022 [details] [diff] [review] patch - m-c - v1 Review of attachment 8363022 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you address the nits I've identified. Because of a recent camera refactoring, the camera.js file has changed dramatically. So note that you are likely to need to do a manual uplift of this patch to 1.3. I'm just guessing that this patch will not cherry-pick cleanly. Let me know if you need help landing or uplifting it. ::: apps/camera/js/camera.js @@ +414,5 @@ > + maxRes = CONFIG_MAX_IMAGE_PIXEL_SIZE; > + } > + else { > + maxRes = CONFIG_MAX_IMAGE_CAPTURE_PIXEL_SIZE; > + } This would be simpler and easier to read if you just did this: maxRes = Math.min(CONFIG_MAX_IMAGE_PIXEL_SIZE, CONFIG_MAX_IMAGE_CAPTURE_PIXEL_SIZE); Also, consider replacing "IMAGE_CAPTURE" with "PHOTO". Less typing that way. ::: build/applications-data.js @@ +530,5 @@ > + '// {\n' + > + '// "maxImagePixelSize": 6000000,\n' + > + '// "maxImageCapturePixelSize": 4000000 }\n' + > + '// }\n' + > + 'var CONFIG_MAX_IMAGE_PIXEL_SIZE = ' + customize.maxImagePixelSize + ';' + This line needs a '\n' at the end of it so that the generated file is readable. @@ +531,5 @@ > + '// "maxImagePixelSize": 6000000,\n' + > + '// "maxImageCapturePixelSize": 4000000 }\n' + > + '// }\n' + > + 'var CONFIG_MAX_IMAGE_PIXEL_SIZE = ' + customize.maxImagePixelSize + ';' + > + 'var CONFIG_MAX_IMAGE_CAPTURE_PIXEL_SIZE = ' + customize.maxImageCapturePixelSize + ';'; This line looks like it is more than 80 characters wide. We probably don't run the linter over these scripts in the build directory, but I'd still like to follow our basic style, so please reformat to wrap on to two lines.
Attachment #8363022 -
Flags: review?(dflanagan) → review+
Updated•10 years ago
|
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Assignee | ||
Comment 7•10 years ago
|
||
Addressed djf nits. Carry forward r=djf.
Attachment #8364047 -
Flags: review+
Flags: needinfo?(dflanagan)
Assignee | ||
Updated•10 years ago
|
Attachment #8363022 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Master: b82d6b2892d5abefc01a3e87cb06bf3bf398dbc6
Comment 10•10 years ago
|
||
v1.3: 0a79a0609a049c6692b4fd9cbd3f07eb2e9d7754
Comment 11•10 years ago
|
||
The v1.3 commit broke the linter, see https://travis-ci.org/mozilla-b2g/gaia/jobs/17501447 Diego, can you push a commit to fix it? Otherwise I'll sadly need to back out this and I'd like not to.
Flags: needinfo?(dwilson)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8366702 -
Flags: review?(felash)
Flags: needinfo?(dwilson)
Comment 13•10 years ago
|
||
Comment on attachment 8366702 [details] [review] Delint - v1.3 r=me thanks !! I'm merging the PR
Attachment #8366702 -
Flags: review?(felash) → review+
Comment 14•10 years ago
|
||
v1.3: 75a134286434647f5ea0434391a53c3c9d19edbb
Updated•10 years ago
|
Flags: in-moztrap-
Updated•10 years ago
|
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
Updated•10 years ago
|
Whiteboard: [CR 600508] → [caf priority: p3][CR 600508]
You need to log in
before you can comment on or make changes to this bug.
Description
•