Closed Bug 980599 Opened 7 years ago Closed 7 years ago

[Camera] Picture and video resolution selection

Categories

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

ARM
Gonk (Firefox OS)
enhancement
Not set
normal

Tracking

(b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: dmarcos, Unassigned)

References

Details

(Whiteboard: [fxos:media] [Land on Branch ETA 3/7])

Attachments

(1 file)

This bug keeps track of the work related to picture size (resolution) selection for the camera app: 

- Configuration file to provide a list of valid picture sizes and video formats for both front and rear cameras
- Camera hardware configuration using the gecko API
- UI for the user to select pictures and video resolutions.
- Video/Photo preview size selection and alignment based on video/photo resolution selected.

As requested in code review this bug's patch is a single commit combining all the work done for bug 974337, bug 974119, bug 975802 and bug 976875.
Attachment #8387154 - Flags: review?(dflanagan)
Whiteboard: [fxos:media] [Land on Branch ETA 3/7]
Target Milestone: --- → 1.4 S3 (14mar)
Earlier today there was a different PR that combined these same four bugs. That PR was https://github.com/mozilla-b2g/gaia/pull/16959

Unfortunately:

- that PR has some needed changes that this one does not have

- this PR has some needed changes that that one does not have

- both PRs are missing at least one change that can be seen in the getAspect() function seen here: https://github.com/justindarc/gaia/blob/bug974337/apps/camera/js/controllers/settings.js#L180

There are comments on github identifying the discrepancies I've seen between the two PRs.

I wonder if you could start a new branch and cherry pick the commits from the two PRs into the same branch and see how many conficts there are.  It might be easy enough to resolve them by hand.  Then you'd still have to track down the missing commit that has the new version of getAspect().
Comment on attachment 8387154 [details] [review]
Pull Request  (camera-new-features)

Giving r- now to get your attention to the discrepances in the PRs.

But if I can keep going tonight, I'll continue to review and test this combined patch to increase my comfort level with landing it.
Attachment #8387154 - Flags: review?(dflanagan) → review-
Comment on attachment 8387154 [details] [review]
Pull Request  (camera-new-features)

David: I have addressed the missing items by manually copying them over from the previous PR into this new one and amending the commit. If you view in GitHub now, it still shows that you commented, but your comments appear now on an "outdated diff". Hopefully those items you found were the last of the missing items, but let us know if you encounter anything else. All unit tests are still passing with the changes I just brought over to this PR, so that's a good sign :-)

Resetting to: r?
Attachment #8387154 - Flags: review- → review?(dflanagan)
Thanks Justin
Comment on attachment 8387154 [details] [review]
Pull Request  (camera-new-features)

I'm ready to land this. There are a couple of nits on github to fix before landing or to fix in a followup soon after landing.

I see showSettings:false in config/app.js, but the settings menu does show up.  Since most of the settings options are not working yet, do we actually want to land a menu that is not enabled?  (If showSettings isn't used anymore, let's remove it.  Should we comment out the code that enables the non-working parts of the settings menu for now?)

I'm not convinced that we're done with the picture size stuff. On hamachi, I'm offered a 30:17 aspect ratio, which is weird. And when I use it, no photo appears in the filmstrip. Probably it is failing because there is no available thumbnail size with that aspect ratio.  So that is one that we'd have to prevent from appearing in the list.  

When I select a 16:9 aspect ratio, it seems that I get a preview that shows more than what is in the actual photo that is taken. That will require more investigation.

Like I said, I'm willing to land this patch now, and file followups for bugs like that.
Attachment #8387154 - Flags: review?(dflanagan) → review+
Blocks: 966832
Blocks: 966831
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530
Target Milestone: 1.4 S3 (14mar) → 1.4 S5 (11apr)
You need to log in before you can comment on or make changes to this bug.