Make camera image size restriction customisable

RESOLVED FIXED

Status

Firefox OS
Gaia::Camera
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wchang, Assigned: Evelyn Hung)

Tracking

({feature})

unspecified
x86
Gonk (Firefox OS)
feature

Firefox Tracking Flags

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

Details

(Whiteboard: [leo-triage])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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: → bug 844921, bug 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

Comment 3

5 years ago
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?
Duplicate of this bug: 905427
(Reporter)

Updated

5 years ago
blocking-b2g: hd? → hd+
(Reporter)

Comment 5

5 years ago
(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?
Duplicate of this bug: 906694
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? → -
Duplicate of this bug: 912504
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?

Comment 12

5 years ago
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)
(Assignee)

Comment 15

5 years ago
I can help on this issue.
Assignee: nobody → ehung
(Assignee)

Comment 16

5 years ago
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
See Also: → bug 914602
(Assignee)

Updated

5 years ago
Depends on: 914602
(Assignee)

Comment 17

5 years ago
Created attachment 803003 [details]
point to https://github.com/mozilla-b2g/gaia/pull/11286

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)
(Assignee)

Comment 18

5 years ago
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)
(Assignee)

Comment 19

5 years ago
Created attachment 803006 [details]
point to https://github.com/mozilla-b2g/gaia/pull/12118

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)
(Assignee)

Comment 20

5 years ago
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-
(Assignee)

Comment 22

5 years ago
(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)
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 27

5 years ago
(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)
(Assignee)

Comment 31

5 years ago
Created attachment 809081 [details]
point to https://github.com/mozilla-b2g/gaia/pull/12390

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
(Assignee)

Comment 34

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 35

5 years ago
gaia-master: https://github.com/mozilla-b2g/gaia/commit/7ddf9a8d3826c07b5bf303a3501dbe969d3c5727
status-b2g18: --- → 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)
(Assignee)

Comment 40

5 years ago
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)
(Assignee)

Comment 42

5 years ago
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)
Looks green landed it: 

v1.2: https://github.com/mozilla-b2g/gaia/commit/672c47bf94b69a329e0aacb9228a6aa16ade6226
status-b2g-v1.2: affected → fixed
Flags: needinfo?(jlal)
(Assignee)

Comment 44

5 years ago
Thank you!

Comment 45

5 years ago
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.