Closed Bug 965533 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Whiteboard: [fxos:media])

Attachments

(1 file, 5 obsolete files)

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.
Whiteboard: [fxos:media]
Target Milestone: --- → 1.4 S1 (14feb)
Must be landed on top of patches in bug 909542 and bug 940424.
Blocks: 933902
Attachment #8368915 - Flags: feedback?(jdarcangelo)
blocking-b2g: 1.4? → ---
Attachment #8368915 - Attachment is obsolete: true
Attachment #8368915 - Flags: feedback?(jdarcangelo)
Attachment #8373697 - Flags: feedback?(jdarcangelo)
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.
Flags: needinfo?(mhabicher)
It helps to put the binary division inside the loop.
Attachment #8373697 - Attachment is obsolete: true
Attachment #8373697 - Flags: feedback?(jdarcangelo)
Attachment #8375139 - Flags: feedback?(justindarc)
Flags: needinfo?(mhabicher)
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).
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.
Attachment #8375139 - Attachment is obsolete: true
Attachment #8375139 - Flags: feedback?(justindarc)
Attachment #8375296 - Flags: review?(mhabicher)
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).
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.
Fix the boundary condition.
Attachment #8375296 - Attachment is obsolete: true
Attachment #8375296 - Flags: review?(mhabicher)
Attachment #8375597 - Flags: feedback?(jdarcangelo)
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).
Attachment #8375597 - Flags: feedback?(jdarcangelo) → feedback+
Attachment #8375597 - Attachment description: WIP - Fix Zoom Attribute, v5 [b2g-inbound] → Fix Zoom Attribute, v5 [b2g-inbound]
Attachment #8375597 - Flags: review?(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)
Attachment #8375597 - Flags: review?(dhylands) → review+
(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.
Rebase. Carrying r+ forward.
Attachment #8375597 - Attachment is obsolete: true
Attachment #8379108 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/aee74e1d4958
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
You need to log in before you can comment on or make changes to this bug.