Closed Bug 900399 Opened 11 years ago Closed 11 years ago

Make camera image size restriction customisable

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
x86
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: wchang, Assigned: jj.evelyn)

References

Details

(Keywords: feature, Whiteboard: [leo-triage])

Attachments

(2 files, 1 obsolete file)

point to https://github.com/mozilla-b2g/gaia/pull/12118
(deleted), text/html
djf
: review-
Details
point to https://github.com/mozilla-b2g/gaia/pull/12390
(deleted), text/html
djf
: review+
Details
Currently as is we restrict the camera photo and gallery image viewing size to 2M pixels, but as more partners are on board with devices that have higher resolution camera, more RAM, we should have an easier way to lift this restriction. https://bugzilla.mozilla.org/show_bug.cgi?id=896425 addresses the restriction with video recording, we should have similar mechanism for camera and gallery restrictions.
See also 844921, 838512 Probably a late bug for leo? but new partners taking on v1.1 may also benefit from this. Leo has altered this restriction in their build. Otherwise HD?
blocking-b2g: --- → leo?
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=844921, https://bugzilla.mozilla.org/show_bug.cgi?id=838512
Whiteboard: [leo-triage]
Too late for 1.1 for a new feature request, pushing this to 1.2 .
blocking-b2g: leo? → koi?
Keywords: feature
Based on the discussion that we had in the media triage, it probably needs to be in HD. Please triage. Thanks Hema
blocking-b2g: koi? → hd?
blocking-b2g: hd? → hd+
(In reply to bhavana bajaj [:bajaj] from comment #2) > Too late for 1.1 for a new feature request, pushing this to 1.2 . HD triage - putting this to koi? given the current stage and too late for feature addition on v1.1HD. Will guide OEM partners on v1.1HD to modify this restriction to suit their hardware. Recommend to have this customization ready on koi.
blocking-b2g: hd+ → koi?
Until a partner needs to block on this for a specific release on a specific device in a specific country, we cannot hold a release back for it. Blocking-. Please renominate if a blocker for a specific device like above.
blocking-b2g: koi? → -
As dpv mentioned in bug 912504, this is an important issue that happens since first commercial version of ikura (which camera supports 3.2 MPixel pictures) and users are reporting this issue to post-sales carrier services. What chances are ther to fixing this in leo?
blocking-b2g: - → leo?
Moving to koi as it is a feature enhancement.
blocking-b2g: leo? → koi?
Please keep in mind that this issue is coming from the support department. From the builds with 1.0.1. We need a fix for v1-train. Moreover, this is a potencial blocker of the certification for v1.1 Nominating to leo+
blocking-b2g: koi? → leo?
According to the discussing during the regular ZTE/TEF meeting, TEF requests to nominate this bug to leo+ and target for v1.1 Spain IOT cycle. Partner will update the schedule for v1.1 Spain IOT cycle on 9/6 or early next week.
blocking-b2g: leo? → leo+
David, Can you please check the complexity of this bug. We believe it is an enhancement and want to the complexity of the same.
Flags: needinfo?(dflanagan)
I'm not sure I understand the question. And I don't know much about how we've been doing build-time customization. But if this is just a matter of adding a new setting that the camera and gallery can read, it should be pretty simple. Note, however, that it can have profound impact on the memory consumption of those apps, and any carrier that modifies the current values will want to do careful testing.
Flags: needinfo?(dflanagan)
I can help on this issue.
Assignee: nobody → ehung
Gallery supports 5M pixel images now, and Wayne told me it's sufficient for leo device. As :djf has pointed out in comment 14, we should be carefully on memory consumption issue if the MAX_IMAGE_PIXEL_SIZE in gallery is customizable. Before we have a safe design to avoid inappropriate customized value breaks the device, let's only considering camera restriction in this issue.
Summary: Make Camera and Gallery image size restriction customisable → Make camera image size restriction customisable
Depends on: 914602
in this patch: 1. add `getPreferredResolution` function to query the value of `_preferredImageResolution` from settings. (key `camera.image.preferredResolution`) 2. update some function and variable names of the original `getPreferredSizes` function to make code consistent.
Attachment #803003 - Flags: review?(dflanagan)
Comment on attachment 803003 [details] point to https://github.com/mozilla-b2g/gaia/pull/11286 ><html> > <head> > <meta http-equiv="Refresh" > content="2; url=https://github.com/mozilla-b2g/gaia/pull/11286" /> > </head> > <body> > Redirect to pull request #11286 > </body> ></html> Sorry point to the wrong PR.
Attachment #803003 - Attachment is obsolete: true
Attachment #803003 - Flags: review?(dflanagan)
in this patch: 1. add `getPreferredResolution` function to get `_preferredImageResolution` from settings. (setting key `camera.image.preferredResolution`) 2. update some function and variable names of the original `getPreferredSizes` function to make code consistent.
Attachment #803006 - Flags: review?(dflanagan)
Comment on attachment 803006 [details] point to https://github.com/mozilla-b2g/gaia/pull/12118 add Yuren to give some feedback since he made the change of customizable recording size thing.
Attachment #803006 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 803006 [details] point to https://github.com/mozilla-b2g/gaia/pull/12118 r- because there is (or the code appears as if there could be) a race condition where you could request a preview stream before the preview size is known. See my comments on github.
Attachment #803006 - Flags: review?(dflanagan) → review-
(In reply to David Flanagan [:djf] from comment #21) > Comment on attachment 803006 [details] > point to https://github.com/mozilla-b2g/gaia/pull/12118 > > r- because there is (or the code appears as if there could be) a race > condition where you could request a preview stream before the preview size > is known. See my comments on github. Thanks for the carefully review. I will evaluate the race condition problem and find a better solution.
Attachment #803006 - Flags: feedback?(yurenju.mozilla)
Hi djf, After thinking more, I feel uncomfortable to customize this value because wrong configured value will cause inconsistency with Gallery app, or even hardware problem. For v1.1, I think we can simply lift Camera's restriction to 5MP in this issue, so it's a consistent size with what Gallery can handle. Do you think it's okay?
Flags: needinfo?(dflanagan)
I wish we had an easy way to do build-time customizations that did not use the settings db. This ought to be easy to configure, but, as you've found here, it is not currently. I'm nervous about changing the image size unconditionally because I suspect it will cause everything to be a bit slower, but I think you are right that just changing the number to make it bigger is what we should do here. Here's what I propose. For both Camera and Gallery, move the variables or constants that are configurable into separate files named configuration.js. That should make it very easy for partners to edit those files and change size limits. Make sure that all the constants defined in those files have good comments. (For example, the size limits for camera and gallery should refer to each other so that anyone editing one knows to look at the other file as well.) Does that make sense?
Flags: needinfo?(dflanagan)
(In reply to David Flanagan [:djf] from comment #25) > Here's what I propose. For both Camera and Gallery, move the variables or > constants that are configurable into separate files named configuration.js. > That should make it very easy for partners to edit those files and change > size limits. Make sure that all the constants defined in those files have > good comments. (For example, the size limits for camera and gallery should > refer to each other so that anyone editing one knows to look at the other > file as well.) > > Does that make sense? +1 becuase it seems to fit our partners needs and cover a wider range of devices porfolio.
(In reply to David Flanagan [:djf] from comment #25) > I wish we had an easy way to do build-time customizations that did not use > the settings db. This ought to be easy to configure, but, as you've found > here, it is not currently. > > I'm nervous about changing the image size unconditionally because I suspect > it will cause everything to be a bit slower, but I think you are right that > just changing the number to make it bigger is what we should do here. > Okay, so it's a performance issue, not only because b2g crash or other hardware problem. Then I agree that this value should be customizable. I was thinking it should be a run-time detection of hardware ability (in Camera app), and an error handling when Gecko can't open a large image (in Gallery app). That was why I didn't want it be customizable. > Here's what I propose. For both Camera and Gallery, move the variables or > constants that are configurable into separate files named configuration.js. > That should make it very easy for partners to edit those files and change > size limits. Make sure that all the constants defined in those files have > good comments. (For example, the size limits for camera and gallery should > refer to each other so that anyone editing one knows to look at the other > file as well.) > > Does that make sense? Yes, I will do it. (In reply to Beatriz Rodríguez [:brg] from comment #26) > (In reply to David Flanagan [:djf] from comment #25) > > Here's what I propose. For both Camera and Gallery, move the variables or > > constants that are configurable into separate files named configuration.js. > > That should make it very easy for partners to edit those files and change > > size limits. Make sure that all the constants defined in those files have > > good comments. (For example, the size limits for camera and gallery should > > refer to each other so that anyone editing one knows to look at the other > > file as well.) > > > > Does that make sense? > +1 becuase it seems to fit our partners needs and cover a wider range of > devices porfolio. I will say it's a temporary solution for leo branch. We should add build-time customizations into our customization framework, so we can let the other things alike (e.g. video recording) go this way.
(In reply to Beatriz Rodríguez [:brg] from comment #26) > (In reply to David Flanagan [:djf] from comment #25) > > Here's what I propose. For both Camera and Gallery, move the variables or > > constants that are configurable into separate files named configuration.js. > > That should make it very easy for partners to edit those files and change > > size limits. Make sure that all the constants defined in those files have > > good comments. (For example, the size limits for camera and gallery should > > refer to each other so that anyone editing one knows to look at the other > > file as well.) > > > > Does that make sense? > +1 becuase it seems to fit our partners needs and cover a wider range of > devices porfolio. Let me just add that, from the OEM point of view, we are quite happy with having this solution for the short term (leo branch) as it solves what our customer is complaining about. For future releases of the OS, we also expect to provide users with options within the camera app (that could be made customizable for OEMs, depending on the device capabilities), so that they can, themselves select the resolution they prefer for each picture they take or video they record.
Dave, Please review the patch and comment 28.
Flags: needinfo?(dflanagan)
Preeti, Which patch are you asking me to review? I reviewed Evelyn's patch. She hasn't posted a new one yet as far as I can tell.
Flags: needinfo?(dflanagan)
Yuren told me there is a way to generate build-time configuration into a JavaScript file under apps, and it's been hooked on customization framework. So I follow the same way to generate a presets.js for both Camera and Gallery app, so partners can customize it once and the value will be shared between these two apps.
Attachment #809081 - Flags: review?(dflanagan)
Attachment #809081 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 809081 [details] point to https://github.com/mozilla-b2g/gaia/pull/12390 I left a lot of comments on github. Mostly nits about names and code structure. I'd like it if you made those changes, but I know that this is an urgent fix and you probably don't have much time, so r+ to land even without those changes. It looks like your patch is only for v1-train. You're going to fix this on master as well, I assume.
Attachment #809081 - Flags: review?(dflanagan) → review+
Please check in
Keywords: checkin-needed
(In reply to David Flanagan [:djf] from comment #32) > Comment on attachment 809081 [details] > point to https://github.com/mozilla-b2g/gaia/pull/12390 > > I left a lot of comments on github. Mostly nits about names and code > structure. I'd like it if you made those changes, but I know that this is an > urgent fix and you probably don't have much time, so r+ to land even without > those changes. > > It looks like your patch is only for v1-train. You're going to fix this on > master as well, I assume. Thanks for reviewing. I've updated according to your comment. merged into v1-train: https://github.com/mozilla-b2g/gaia/commit/2cae0622ee942906daaca90293441afdbd2cb7de I will do another patch for master.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #809081 - Flags: feedback?(yurenju.mozilla)
Presumably this needs landing on v1.2 still?
Keywords: checkin-needed
v1.1.0hd: 64b91d8cf8d6baf6e7c9a6bb55024782c400457f
status-b2g-v1.1hd: --- → fixed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #36) > Presumably this needs landing on v1.2 still? Yes, please, we need it on v1.2, shall I mark the tracking flag status-b2g-v1.2 --> Affected?
I was not able to uplift this bug to v1.2. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1.2 git cherry-pick -x -m1 7ddf9a8d3826c07b5bf303a3501dbe969d3c5727 <RESOLVE MERGE CONFLICTS> git commit
Flags: needinfo?(ehung)
v1.2: 998b22382dda6e136866a553d8665d9f9a8f3398
status-b2g-v1.2: --- → fixed
Flags: needinfo?(ehung)
reverted in 1.2 85c4af3f48a91878d565f518ba0eed68f0628e21 we need to keep the 1.2 tree green too (and other release branches going forward) its just a lint issue: https://travis-ci.org/mozilla-b2g/gaia/builds/12218011#L41 so it should be easy to fix...
status-b2g-v1.2: fixed → affected
Flags: needinfo?(ehung)
Hi James, Sorry for turning it red! I check the lint error, and guess the generated js files can be excluded from gjslint checking. Therefore, I make a PR here: https://github.com/mozilla-b2g/gaia/pull/12697/files#diff-439b286e23b514094467e99613311d9fR1 Could you take a look to confirm my update is okay? Thanks a lot!
Flags: needinfo?(ehung) → needinfo?(jlal)
Thank you!
Hema Koka deleted the linked story in Pivotal Tracker
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: