980599 - [Camera] Picture and video resolution selection

Status: RESOLVED FIXED

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

Description

[Diego Marcos [:dmarcos]]

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.

Comment 1

[Diego Marcos [:dmarcos]]

These are the original PRs for each of the bugs for reference purpose:

Bug 974337

https://github.com/mozilla-b2g/gaia/pull/16485 

Bug 974119

https://github.com/mozilla-b2g/gaia/pull/16820 

Bug 975802

https://github.com/mozilla-b2g/gaia/pull/16824

Bug 976875

https://github.com/mozilla-b2g/gaia/pull/16877

Comment 2

[Diego Marcos [:dmarcos]]

Attachment: Pull Request (camera-new-features)

Comment 3

[David Flanagan [:djf]]

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 4

[David Flanagan [:djf]]

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.

Comment 5

[Justin D'Arcangelo [:justindarc]]

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?

Comment 6

[Diego Marcos [:dmarcos]]

Thanks Justin

Comment 7

[David Flanagan [:djf]]

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.

Comment 8

[Diego Marcos [:dmarcos]]

Landed in camera new features:

https://github.com/mozilla-b2g/gaia/commit/cab07100493b2b2970eaa61f9460a5375996e122

Comment 9

[David Flanagan [:djf]]

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