Closed Bug 960756 Opened 6 years ago Closed 6 years ago

Decouple max camera snapshot size from max gallery image size

Categories

(Firefox OS Graveyard :: Gaia::Gallery, defect, critical)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
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.
Blocks: 942267
blocking-b2g: --- → 1.3?
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)
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: nobody → dwilson
Status: NEW → ASSIGNED
Whiteboard: [CR 600508]
blocking-b2g: 1.3? → 1.3+
Attached patch patch - m-c - v1 (obsolete) — Splinter Review
Attachment #8363022 - Flags: review?(dflanagan)
David,

Please help with patch review here.
Flags: needinfo?(dflanagan)
David is here in Taipei with me.  I'll ask him to review this ASAP.  Thanks!
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+
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Addressed djf nits. Carry forward r=djf.
Attachment #8364047 - Flags: review+
Flags: needinfo?(dflanagan)
Attachment #8363022 - Attachment is obsolete: true
Manual rebase to v1.3
Attachment #8364048 - Flags: review+
Keywords: checkin-needed
Master: b82d6b2892d5abefc01a3e87cb06bf3bf398dbc6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
v1.3: 0a79a0609a049c6692b4fd9cbd3f07eb2e9d7754
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)
Attached file Delint - v1.3
Attachment #8366702 - Flags: review?(felash)
Flags: needinfo?(dwilson)
Comment on attachment 8366702 [details] [review]
Delint - v1.3

r=me

thanks !!
I'm merging the PR
Attachment #8366702 - Flags: review?(felash) → review+
v1.3: 75a134286434647f5ea0434391a53c3c9d19edbb
Flags: in-moztrap-
Whiteboard: [CR 600508] → [caf priority: p3][CR 600508]
You need to log in before you can comment on or make changes to this bug.