965533 - [Camera][Gecko] Zoom attribute on CameraControl is just broken

Status: RESOLVED FIXED

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

Description

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

This attribute was initially implemented on Otoro/Unagi which didn't support zooming of any kind, and so we were unable to test it. As it happens, it's just plain broken and needs to be fixed.

Comment 1

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

Attachment: WIP - Fix Zoom Attribute, v1 [b2g-inbound]

Must be landed on top of patches in bug 909542 and bug 940424.

Comment 2

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

Attachment: WIP - Fix Zoom Attribute, v2 [b2g-inbound]

Comment 3

[Justin D'Arcangelo [:justindarc]]

Mike: The updated v2 of the patch seems to produce the `zoomRatios` array properly now. However, the `zoom` attribute is still broken for me on Helix. If I simply call the getter after the app launches, it correctly returns `1.0`. But, when I attempt to set the value to anything else, it does nothing. If I try this through the console using App Manager, the console stops responding after I attempt to set the `zoom` value. Can you let me know if this is happening for you as well? I thought that possibly the reason it was doing this with v1 of the patch was because of the empty `zoomRatios` array, but that doesn't seem to be the case.

Comment 4

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

Attachment: WIP - Fix Zoom Attribute, v3 [b2g-inbound]

It helps to put the binary division inside the loop.

Comment 5

[Justin D'Arcangelo [:justindarc]]

Mike: The v3 patch seems to be able to do the scale factor lookups now, however the camera's picture is not scaling to reflect the scale factor you have set it to. The only time I ran into an issue where the attribute setter seemed to stop responding was if I tried to set the max value (4.0).

Comment 6

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP - Fix Zoom Attribute, v4 [b2g-inbound]

Mike: I think I found the issue with v3 of the patch and got the zoom functionality in Gaia wired up to the `zoom` attribute. Everything seems to be working! Strangely, the `zoom` attribute seems to have no effect on the preview stream even though it was definitely manipulating it before. However, when you take a photo, the end result is *definitely* affected by it. I simply kept my previous implementation using a CSS transform to scale the viewfinder in Gaia and I just keep the `zoom` attribute in sync with that. Please review v4 of this patch and make sure everything is ok.

Comment 7

[Justin D'Arcangelo [:justindarc]]

Mike: I just realized that it shouldn't matter if that `return` is inside or outside of the {...} (why JS devs shouldn't write C). I may have just thought it wasn't working because the preview wasn't scaling. Either way, I was still getting strange lock-ups in the console in App Manager (might just be an App Manager bug though).

Comment 8

[Justin D'Arcangelo [:justindarc]]

Mike: I was curious so I just reverted back to v3 of the patch and it is working just the same as v4. Its just strange that the preview image seems to no longer be affected by the `zoom` attribute. Its no big deal since this is easily solved in Gaia. So, feel free to use either v3 or v4 since the change made in v4 is nothing more than a coding standards difference.

Comment 9

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

Attachment: Fix Zoom Attribute, v5 [b2g-inbound]

Fix the boundary condition.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8375597 [details] [diff] [review]
Fix Zoom Attribute, v5 [b2g-inbound]

Looks good Mike! Everything seems to be working well for me now. You can go ahead and proceed to land this (not sure who needs to review).

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 8375597 [details] [diff] [review]
Fix Zoom Attribute, v5 [b2g-inbound]

Review of attachment 8375597 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, although I think that you should use bsearch for the binary search

::: dom/camera/GonkCameraParameters.cpp
@@ +449,5 @@
> +            top = middle - 1;
> +          }
> +        }
> +        index = middle;
> +      }

Shouldn't you use one of the already implemented and tested binary search algorithims, like bsearch?
http://linux.die.net/man/3/bsearch

::: dom/camera/test/test_camera.html
@@ +36,5 @@
>  }
>  
>  var capabilities = [ 'previewSizes', 'pictureSizes', 'fileFormats', 'maxFocusAreas', 'minExposureCompensation',
>                       'maxExposureCompensation', 'stepExposureCompensation', 'maxMeteringAreas', 'videoSizes', 
> +                     'recorderProfiles', 'zoomRatios'];

nit: trailing space on line before (ok not part of your patch)

@@ +107,5 @@
>    takePictureSuccess: function taken_foto(blob) {
>      var img = new Image();
>      var test = this._currentTest;
>      img.onload = function Imgsize() {
>        ok(this.width == test.pictureSize.width, "The image taken has the width " + 

nit: a couple more trailing spaces (also not part of your patch)

Comment 12

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

(In reply to Dave Hylands [:dhylands] from comment #11)
> Comment on attachment 8375597 [details] [diff] [review]
> Fix Zoom Attribute, v5 [b2g-inbound]
> 
> Review of attachment 8375597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, although I think that you should use bsearch for the binary search

Using bsearch(), finding a partial match--where a specified zoom value lies between two supported values--becomes really awkward.

Comment 13

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

Attachment: WIP - Fix Zoom Attribute, v5.1 [b2g-inbound] r=dhylands

Rebase. Carrying r+ forward.

Comment 14

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=5dfcfe956a8d&showall=1

Comment 15

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

https://hg.mozilla.org/integration/b2g-inbound/rev/aee74e1d4958

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aee74e1d4958