933902 - [Camera] 1.4 Add zoom functionality

Status: RESOLVED FIXED

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

Description

[Peter La]

Add zoom functionality.

User Stories:

Media 54
As a user, while taking a picture using the camera app, I want the
ability to use zoom in and zoom out functions to make the subject
appear bigger or smaller.

Media 104
In camera/camcorder mode, zoom functions should provide the user
with zoom-in and zoom-out. (mandatory)

Version 0.5 Camera Spec:
https://app.box.com/files/0/f/1211601585/1/f_11402480095

Comment 1

[Peter La]

Attachment: _Camera1_VisualSpec-ZoomPart1V1_Nov5.png

Zoom Visual Spec / Part 1

Note: The zoom max and min indicators may be contentious - I'll have to run them by Rob when he gets back, but I think it should be an easy swap if we don't go with the circle design.  I think it's worth testing out and getting into a build we can play with.

Comment 2

[Peter La]

Attachment: _Camera1_VisualSpec-ZoomPart2V1_Nov5.png

Zoom Visual Design Spec / Part 2

Comment 3

[Tiffanie Shakespeare [:tif]]

Comment on attachment 827772 [details]
_Camera1_VisualSpec-ZoomPart1V1_Nov5.png

Visually, I think it looks good, but conceptually I find the meter part a little confusing, especially in the third screen when the blue and white colors swap. It could be because the mockups are "static" so my brain isn't making the connection, but it just seems like I have to think about it too much.

Comment 4

[Justin D'Arcangelo [:justindarc]]

I don't see anywhere that a "max zoom" value is specified. Are there any opinions on what the "max" digital zoom scale should be? 4x? 8x?

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to jdarcangelo from comment #4)
>
> I don't see anywhere that a "max zoom" value is specified. Are there any
> opinions on what the "max" digital zoom scale should be? 4x? 8x?

The _cameraObj.capabilities.zoomRatios attribute returns an array of zoom ratios (as floating-point values) supported by the camera hardware.

Comment 6

[Diego Marcos [:dmarcos]]

Is the zoomRatios refering to digital zoom? I thought of digital zoom as not hardware dependent

Comment 7

[Justin D'Arcangelo [:justindarc]]

I second that notion. Digital zoom should not depend on the hardware.

Comment 8

[Diego Marcos [:dmarcos]]

We can do digital zoom on the gaia side but it seems that the hardware can provide digitally zoomed images. Is that right Mike?

Comment 9

[Justin D'Arcangelo [:justindarc]]

It seems to me that the UX spec calls for a *continuous* zoom slider that correlates with the pinch-to-zoom touch gesture. If there were simply an array of possible scale factors to work with, then that would almost imply there are zoom *steps* (e.g. 1x, 1.5x, 2x, etc.) instead of a continuous, flowing value (e.g. the zoom could be 1.34215323x).

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

Sorry for the confusion: yes, .zoomRatios are the *optical* zoom steps supported by the camera.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Ok. So my original question still stands as to what the *max* zoom ratio should be for digital zoom.

Comment 12

[Rob MacDonald [:robmac]]

Comment on attachment 827772 [details]
_Camera1_VisualSpec-ZoomPart1V1_Nov5.png

Hi Peter - Overall this looks fantastic but I had to minus it due to a couple of items. First, the "No Zoom" screen has an artifact on the right side. Second, It's difficult to differentiate between the zoom in indicator and the zoom out indicator. I find the difference a bit too subtle and would prefer +/- buttons. And, since the user can also tap on these icons to increase or decrease the zoom level, +/- would be clearer. Does this all make sense? - Rob

Comment 13

[Hema Koka [:hema]]

At this point, we are not going to be adding any new features or features that are not close to finishing into 1.3. Lets work this for 1.4 and focus on stabilizing 1.3 codebase

Comment 14

[Peter La]

clearing my needinfo as we had this discussion offline (about max zoom).

Comment 16

[Amy Lee [:amylee] UX]

Will be adding updated specs to this bug

Comment 18

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP - Patch to implement pinch-to-zoom and Zoom Bar

Work in progress. Still need to refine visual styles a bit more to match specs.

Comment 19

[Amy Lee [:amylee] UX]

Attachment: CSS_Pixels_Dual_Shutter_Orientation.png

Hi, 

Here is the updated spec for zoom bar orientation (same as Madai).

Comment 20

[Amy Lee [:amylee] UX]

Attachment: CSS_Pixels_Camera_Zoom.jpg

Updated specs for zoom bar (same as Madai).

Comment 21

[Amy Lee [:amylee] UX]

Attachment: CSS_Pixels_Camera_Zoom.png

Updated spec. Zoom text (1.0x, 2.5x, etc.) should be Fira Sans Medium.

Comment 22

[Justin D'Arcangelo [:justindarc]]

As a result of recent changes regarding how we select the `previewSize` based on the selected `pictureSize`, a glaring issue with the way the `zoom` attribute works has come up. I'm going to use simple, round numbers here to explain.

First, let's assume we have a camera sensor that is 1000x1000 and the Camera API supports 1.0x-4.0x zoom ratios.

Scenario A: `previewSize` = 250x250
*************************************
`zoom` = 1.0 - The preview stream will use the entire 1000x1000 pixel area of the sensor and apply 0.25x scale to fit the preview size
`zoom` = 4.0 - The preview stream will use the center-most 250x250 pixel area of the sensor and apply 1x scale to fit the preview size (cropping the sensor area)

RESULT: `zoom` appears to work as intended and all zoom ratios have visible impact on the preview stream

Scenario B: `previewSize` = 1000x1000
*************************************
`zoom` = 1.0 - The preview stream will use the entire 1000x1000 pixel area of the sensor and apply 1x scale to fit the preview size
`zoom` = 4.0 - The preview stream will use the entire 1000x1000 pixel area of the sensor and apply 1x scale to fit the preview size

RESULT: `zoom` appears to have NO EFFECT on the preview stream because it cannot "crop" the sensor area and still have a 1000x1000 preview stream

It seems that when applying `zoom`, the preview stream can only crop and SCALE DOWN to fit the preview size. But, it cannot simply SCALE UP the entire pixel area. Strangely enough though, if you take a picture, the resulting picture seems to always honor the `zoom` value meaning it *can* SCALE UP the final picture, just not the preview stream in real-time. This means, in many cases, we can have a viewfinder showing one zoom level, but when we take a photo, we get another zoom level. I have tested this and verified this behavior on both Helix and Nexus4. On those devices, if I override the `previewSize` to pick the largest available, the `zoom` attribute has little-to-no effect on the preview stream. But, if I pick a much smaller `previewSize`, the `zoom` attribute appears to accurately reflect itself in the preview stream. Regardless of the `previewSize`, on both of those devices, the actual photo appears to be taken at the correct zoom level.

I can think of two solutions to this that could be implemented in Gaia:

Solution #1:
We always pick the largest available `previewSize` and use a CSS transform to scale the viewfinder accordingly.

Pros:
- We may need to manually "crop" the viewfinder in CSS anyway to support 1:1 "square" aspect ratios in the near future (we've seen NO DEVICES that support this aspect ratio natively)
- I already have a working implementation of this (with the exception of the aspect ratio cropping)

Cons:
- We would have to use some additional CSS to "crop" the viewfinder appropriately to match the selected `pictureSize`

Solution #2:
We pick the `previewSize` that closely matches the selected `pictureSize` (the way we currently do) and determine how far we can "zoom" before we run out of area on the camera sensor. We would then use a CSS transform the scale the viewfinder for any `zoom` level beyond what the camera sensor can provide.

Pros:
- The viewfinder aspect ratio would remain correct

Cons:
- Complexity
- Accuracy

If anyone else has any ideas or insight into the AOSP Camera API, I'd be glad to hear it! Setting NEEDINFO for the rest of the Camera team to see if we can get some consensus on this issue. Thanks!

Comment 23

[Wilson Page [:wilsonpage]]

VOTE: Solution #1

The viewfinder has changed a little bit in camera-dev following the frame-grid patch. The `<video>` element now sits inside a `<div>` which is correctly scaled to match the pictureSize.

I've tested out in devtools and it seems adding a `transform: scale(<1-4>)` to the `<video>` element scales the preview stream no problem.

I suggest adding an API to the ViewfinderView:

viewfinder.scale(<float>);

Which can take care of setting the scale transform on the `<video>` element. Setting `overflow: hidden` on this parent div will take care of the cropping.

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

Are we running into a case where, when previewSize it set to approximately the screen size, maximum zoom means we run out of sensor area? Or is this problem a result of choosing a previewSize that is (much?) larger than necessary to support the device's screen size?

Comment 25

[Justin D'Arcangelo [:justindarc]]

Mike: we're running into cases where on some devices, even if the previewSize is set to a reasonable size, the preview stream won't show any zooming beyond 1.5x-2x. If you select the largest possible previewSize, you can't zoom at all. The full zoom range is only visible if you use a really small previewSize, which then looks blurry and scaled.

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

Justin: okay, thanks for the info. The blurry scaled preview will be an artifact of the digital zoom; I don't think there much we can do. Assuming that's okay for the preview, is it feasible to change the previewSize on the fly as we run out of sensor headroom, and just rely on CSS to fluff it up to screen size?

Comment 27

[Justin D'Arcangelo [:justindarc]]

Mike: Check out the comments in the AOSP source here on the `setZoom` method:

https://code.google.com/p/android-source-browsing/source/browse/QualcommCameraHardware.cpp?repo=platform--hardware--qcom--camera#8876

You know the backend side of this better than I, so you can probably tell us if this is applicable. But, the comments on the `setZoom` method linked above describe *exactly* the behavior I'm observing:

    // No matter how many different zoom values the driver can provide, HAL
    // provides applictations the same number of zoom levels. The maximum driver
    // zoom value depends on sensor output (VFE input) and preview size (VFE
    // output) because VFE can only crop and cannot upscale. If the preview size
    // is bigger, the maximum zoom ratio is smaller. However, we want the
    // zoom ratio of each zoom level is always the same whatever the preview
    // size is. Ex: zoom level 1 is always 1.2x, zoom level 2 is 1.44x, etc. So,
    // we need to have a fixed maximum zoom value and do read it from the
    // driver.

Comment 28

[Mike Habicher [:mikeh] (high bugzilla latency)]

Re, comment 27: that comments creates as many questions as it answers. "However, we want the zoom ratio of each zoom level is always the same whatever the preview size is"? And "...we need to have a fixed maximum zoom value and do read it from the driver"?

Comment 29

[David Flanagan [:djf]]

Here are my thoughts on the various issues above.

- Justin's solution #1 proposal is not guaranteed to always work. We will probably find phones where the maximum preview size still allows some zoom So I think that we're going to have to implement a solution that mixes the hardware-provided zooming with CSS zooming, no matter what.  I fear that we just have to suck it up and deal with the complexity.

- Solution #1 also presumably carries a performance penalty because we're pushing more bits around for a larger preview stream.

- I've been an advocate for always choosing a preview size that matches the aspect ratio of the photo we're recording.  If we're going to allow widescreen photos, that would mean changing preview size.  And this latest twist with zoom makes me think that would not be wise.  So I now favor picking the preview that is the closest match to the screen size and sticking with that for everything, even if that means CSS cropping for widescreen and square photos.

- Nice find in the AOSP code, Justin.  Does the code itself include anything that indicates how the Android camera apps deal with this changing maximum zoom ratio?

- Note that the code Justin found is in a file called QualcommCameraHardware.cpp. We should assume that Spreadtrum-based devices will be different. I think the Tarako is Spreadtrum, but it probably doesn't support zoom, so that won't be an issue for now.

- Nevertheless, history suggests that different devices will be different. I would guess that we won't be able to find an algorithm that works everywhere and should plan to include a build-time configuration option that will set the maximum zoom size above which we have to take over with CSS scaling.

- And while we're at it, the preview size should be a build-time configuration option as well.

- Justin: Can you determine the maximum native preview zoom by just dividing each dimension of the largest supported photo size (which is presumably the sensor size) by the corresponding dimension of the preview size and taking the smaller of the two values?  If that gives you the maximum native zoom, then calculating the addition CSS scaling required should be pretty easy.  And if that doesn't give you the maximum size then we might have to figure it out by trial and error and configure it at build time.

Comment 30

[Justin D'Arcangelo [:justindarc]]

A couple of replies to the last few comments...

Wilson: My current patch also does the same as far as wrapping the <video> tag in a <div> with `overflow: hidden`. I did see that the new camera grid also does this and I had a conflict when rebasing :-)

Also, it is not as simple as just applying a transform to the <video> tag 100% of the time because when you call `mozCamera.zoom = X`, it will alter the preview stream. So, if you *always* applied the transform to the <video> tag, you would effectively be applying the scale *twice* up until a certain point (when the sensor runs out of area). This is why my "Solution #1" required always using the maximum `previewSize`.

However, as David pointed out, we may not always be guaranteed that the maximum `previewSize` won't have any room to scale.

Unfortunately, I think David is right in saying that we'll just have to "suck it up and deal with the complexity" as I don't really see any easy way around this problem.

Ideally, the Gecko API would just apply the correct scale to the preview stream 100% of the time. I'm not certain, but I have a hard time believing that the AOSP Camera API doesn't just do this. So, with that being said...

Mike: Is the AOSP interface you're working with at a "lower level" than the one that a native Android camera app might be using? For example, would a 3rd-party Android camera app have to deal with this issue? I'm just wondering if there's not another intermediate layer somewhere in AOSP that can automatically compensate for the camera driver's shortcomings.

For now, I'm going to investigate David's suggestion of dividing the dimensions to see if I can accurately determine the point at which the preview stream will stop "zooming" on its own. If I can nail this down accurately, I believe that the end solution may not be as complex as I first thought.

Comment 31

[Mike Habicher [:mikeh] (high bugzilla latency)]

Justin: in AOSP, there's a Camera app written in Java[1], and a JNI layer that more or less talks directly to the same library we do. Here's where previewSize is selected and set[2].

Also, some more comments on the maximum zoom parameter[3].

I don't see any attempt to adjust previewSize based on zoom.

1. http://androidxref.com/4.0.4/xref/packages/apps/Camera/src/com/android/camera/Camera.java
2. http://androidxref.com/4.0.4/xref/packages/apps/Camera/src/com/android/camera/Camera.java#1956
3. http://androidxref.com/4.0.4/xref/frameworks/base/core/java/android/hardware/Camera.java#3002

Comment 32

[Justin D'Arcangelo [:justindarc]]

http://developer.android.com/reference/android/hardware/Camera.Parameters.html#getMaxZoom()

"This value may change in different preview size. Applications should call this again after setting preview size."

So, there's our definitive answer. The maximum allowed zoom will vary depending on the `previewSize`. Additionally, there does seem to be a correlation between the largest available `previewSize` and the amount that you can zoom...

On Helix (max `previewSize` is: 1280 x 720):

`previewSize`|Max Visible Zoom|1280 / `previewSize.w`
-----------------------------------------------------
1280 x  720  |1.00            |1.00
 800 x  480  |1.62            |1.60
 720 x  480  |1.78            |1.78
 480 x  320  |2.70            |2.67
 320 x  240  |4.00            |4.00
 176 x  144  |4.00            |7.27

NOTE: The "Max Visible Zoom" is the highest `zoom` value I could set that had a visible change coming from the `mozCamera.capabilities.zoomRatios` array. So, in cases where the value doesn't match exactly, it is because the step between two values overshoots the (1280 / `previewSize.w`) calculation. Also, note that with a very small `previewSize`, the (1280 / `previewSize.w`) calculation exceeds the 4.0 value, but you are still limited to the largest value from the `zoomRatios` array (which in the case of Helix is 4.0).


Mike is going to expose the `getMaxZoom()` method in Gecko in bug 977756

Comment 33

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request #62 to dmarcos/camera-dev-rebased

Comment 34

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (camera-new-features)

Comment 35

[David Flanagan [:djf]]

Comment on attachment 8385873 [details] [review]
pull-request (camera-new-features)

Overall this looks really good, Justin.

I haven't had time to do a really thorough review, but I have identified some things on github that I'll want you to change before landing, so setting r- now so you can work on addressing those issues.

Consider using gesture_detector.js for pinch gestures since camera already uses it and it is proven. Simplify the max zoom calculation. Get rid of the mouse events. Change the CSS to use rems. Figure out the merge conflict resolution errors or whatever they are. Etc.

Comment 36

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (camera-new-features)

Comment 37

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

Rob, I'm adding you for the ui-review. I submitted the pull request for this feature from Diego's repo since he said he set your git repo up to be able to pull from his. To pull down this patch:

git fetch dmarcos
git checkout dmarcos/bug933902-rebased
make install-gaia APP=camera

Let me know if you have any issues flashing this patch. Thanks!

Comment 38

[Justin D'Arcangelo [:justindarc]]

Also adding Hung Nguyen to review the UI.

Comment 39

[Rob MacDonald [:robmac]]

(In reply to Justin D'Arcangelo from comment #37)

> Rob, I'm adding you for the ui-review. 

Thanks, Justin. I will review the patch this evening or tomorrow morning at the latest and contact you if there are any issues flashing.

Comment 40

[Hung Nguyen]

Adding Amy for review. 

Thanks

Comment 41

[Rob MacDonald [:robmac]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

I reviewed the patch on the Nexus 4 (yay). Here's a summary of the issues identified:

Zoom indicator placement and size 
- Currently the vertical placement and height is off. I understand that this is dependent upon other patches and will be fixed soon. Amy may have additional comments related to the visual appearance.

Numeric zoom level indicator (1.0x, 1.2x, etc) 
- Although this is in the visual spec, it was not actually in scope based on the UI spec and should be removed. Our feeling is it doesn't add a lot of value over what's visible in the viewfinder and adds noise to the UI. Sorry for any confusion.

Zoom levels in Nexus 4 
- For some reason, the zoom does not seem to work between 1.5 and 2.4 magnification on the Nexus 4. While the numbers increase (ok it was handy here!), the viewfinder does not change.

Viewfinder rotation
- To avoid having the zoom indicator jump around the screen, it disappear when the viewfinder orientation changes.

Zoom indicator / control interaction
- This is described on page 14 of the 3.6 spec - https://bugzilla.mozilla.org/attachment.cgi?id=8383365. Behaviours are defined for when the user taps on the zoom indicator bar versus what happens when the user taps or holds on the circles at top or bottom of the zoom indicator. Let me know if you have any questions.

I'll review the patch a bit further this afternoon and, if I notice anything else, will post additional information to the bug.

Let me know if there are any additional questions or concerns.

Thanks!

Comment 42

[Rob MacDonald [:robmac]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

One additional item...

Zoom indicator activation
- In the patch the zoom indicator can be activated with a single finger tap. It should be activated using a pinch or expand gesture with two fingers.

Comment 43

[David Flanagan [:djf]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

r- because of the timeout that doesn't get cleared and because the maxHardwareZoom calculation isn't working on nexus 4. But I think both of those should be easy enough to fix.

Various other comments left on github.

I notice that when the zoom bar is hidden it still responds to events.  So if you know exactly where it is, you can zoom with one finger.  But I'm guessing that is not intentional and not actually desired.  You should probably have pointer-events:none when it is hidden.  Or use display:none instead of opacity:0 

If I put one finger down on the zoom bar and one finger on the viewfinder, I can move the viewfinder finger to zoom, I guess because a pinch is being detected.

If the zoom bar is visible and I put one finger down on the viewfinder, and then one finger on the zoombar, then pinch to zoom in I sometimes see the viewfinder zooming out instead, even when the finger on the zoom bar is moving up.

If I put one finger down on the viewfinder, and then tap a second finger on the viewfinder, I can make the filmstrip come up, as if it was a regular one-finger tap. This problem will obviously go away when we get rid of the filmstrip, but we need to be sure that we don't end up setting the touch focus area when the user is zooming.

The zoom level does not change when I switch from front to rear camera or when I switch from camera to video. I think it should probably reset to be zoomed all the way out when we switch, but I suppose that is for UX to decide.

The way that the pinch gesture works here is different than the way the gesture detector does it for the photo preview code.  Your code zooms proportionally to the number of pixels moved divided by 200. Spreading your fingers 200px all at once doubles the zoom level.  But if you spread your fingers slowly, then each pixel you move will increase the zoom by 1.005. If you do that 200 times, you get an overall zoom factor of 2.7 instead of 2.

For gesture detector, the zoom is proportional to the distance between your fingers divided by the initial distance. So if you start with your fingers close together then small changes will create big zooms and if you start with your fingers farther appart then small changes will create small zooms.  I think that with gesture detector, the zoom level is independent of pinch speed.

Comment 44

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

Rob: I've updated the PR. When you get a chance, take a look to see if the comments you made have been addressed. The only outstanding item would be some of the placement (with regards to the new controls, since they're not in yet).

Comment 45

[Amy Lee [:amylee] UX]

Attachment: Zoombar_VSD_Review.png

Hi Justin, 

Zoom bar looks great. I know the positioning is still being worked out so I'll leave that for now. I've attached a spec for active state. Basically the grabber should change to the highlight state colour while active and the Min/Max zoom indicators become 100% opacity. Let me know if you have any questions. Thanks!

Comment 46

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

David: I've addressed all of the issues from UX review and went over a few additional UX details with Amy this afternoon. Also, I've addressed your review comments and updated the PR. One item I want to mention before you re-review:

I added a build-time configuration setting called `CONFIG_USE_ASPECT_RATIO_FOR_MAX_ZOOM`. I cannot understand why on the Nexus 4 the max zoom is not being correctly calculated, but as you discovered, it seems to work properly if you consider the aspect ratio. Unfortunately, if I consider the aspect ratio, this breaks on every other device I've tested. While I don't think this is the best solution, it is unfortunately the only solution I know of at this time. Anyhow, let me know what you think. Thanks!

Comment 47

[David Flanagan [:djf]]

The settings app appears to query the settingsdb for deviceinfo.hardware to display the Model information in Device Information.  Let's query that on camera startup and customize the zoom behavior if it equals 'mako', which is the codename for nexus 4.  

I say this because I don't think we have a way to configure by hardware at build time for general testing.  Developers who just flash their own nexus 4s won't get the right config file.

Comment 48

[David Flanagan [:djf]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

Justin,

I like the changes you've made. My comments are github are mostly just requesting some more comments in the code and CSS.

The one issue that I'd like fixed before landing is dealing with making it just work on nexus4 out of the box.  If we require a special build step in order to make zoom work right on nexus4, I'm pretty sure we'll get lots of bug reports about zoom not working on nexus4.  So let's see if we can use run-time configuration for this one using the device.hardware setting, even if you have to query it on the startup path.

And let's file a followup bug to figure out a more robust solution.

Comment 49

[David Flanagan [:djf]]

Justin,

I tried playing around with this some.  I added this debugging code to lib/camera.js:configureZoom:

  var sizes = this.previewSizes();
  navigator.mozSettings
    .createLock().get('deviceinfo.hardware')
    .onsuccess = gotDeviceName;

  function gotDeviceName(e) {
    var device = e.target.result['deviceinfo.hardware'];
    var ratio = previewSize.width / previewSize.height;
    var maxsize = CameraUtils.getMaximumPreviewSize(sizes);
    var maxmatch = CameraUtils.getMaximumPreviewSize(sizes, ratio);
    var maxzoom1 = Math.min(maxsize.width/previewSize.width,
                            maxsize.height/previewSize.height);
    var maxzoom2 = Math.min(maxmatch.width/previewSize.width,
                            maxmatch.height/previewSize.height);
    console.log('Device:', device);
    console.log('device pixel ratio', window.devicePixelRatio);
    console.log('Preview sizes', JSON.stringify(sizes));
    console.log('current preview size and ratio',
                JSON.stringify(previewSize), ratio);
    console.log('max size', JSON.stringify(maxsize));
    console.log('max size, same ratio', JSON.stringify(maxmatch));
    console.log('using maxHarwareZoom', maxHardwareZoom);
    console.log('computed with maxsize', maxzoom1);
    console.log('computed with maxmatch', maxzoom2);
  }

I found that the deviceinfo.hardware setting does work.  Nexus 4 is "maki".  The Helix is "huawei", and the hamachi is just "qcom".

On the Helix, the computed maxZoomSize of 2.5 is too small.  When I zoom in all the way what I see on the screen is more zoomed in than the picture the camera actually takes.  If I compare the widths instead of the heights then I get 3.33 as the maxZoom value and that seems to work perfectly.

On the nexus4, using the max resolution of the same aspect ratio work, and gets a maxZoom of 1.11. But if I don't use that value and compare the widths instead of heights as above, then I get a value fo 2.22. That value is way to high. But if I divide it by the maxPixelRatio of 2, then I get the 1.11 value that does work just right. If we have to pick a hack for the nexus 4, dividing by the device pixel ratio seems like a better one than using the current aspect ratio.

On hamachi, if I compare the widths rather than the heights, I get a max zoom of 1.33 which works well. If I compare the preview height to the max preview height as you do now, I get a max zoom of 1.5 which seems a little off when I zoom in all the way and take a picture.

Note that on hamachi we get a 320x480 preview, and if I use Math.max() and compare both height and widths, I get a the bad 1.5 value. On Hamachi, a comparison with Math.min() does work and Math.max() does not.  It is the opposite on Helix.  On both, we just have to compare the preview width to the max preview width. So I retract what I said earlier in this review about using Math.min().

So the general rule I'm finding is that the max zoom value is the preview width divided by the max preview width, except that on nexus 4, you have to divide this by the device pixel ratio

If we're going to special case the nexus 4, rather than inventing a calculation rule that works, I suppose we could also just hardcode the 1.11 value for that device.

I hope this helps!

Comment 50

[Rob MacDonald [:robmac]]

Comment on attachment 8389460 [details] [review]
pull-request (camera-new-features)

All interaction issues have been addressed. Thanks!

Comment 51

[Justin D'Arcangelo [:justindarc]]

Landed on camera-new-features:

https://github.com/mozilla-b2g/gaia/commit/2f9d1662770a2f2edf5a6dc697ab81ff48ecaa9e

Comment 52

[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