GonkCameraControl::PullParametersImpl may invalidate cached parameters

RESOLVED FIXED

Status

Firefox OS
Gaia::Camera
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aosmond, Assigned: royang51)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
GonkCameraControl::PullParametersImpl may invalidate cached parameters if the underlying driver modified the values we set for any reason.

The following parameters are cached (directly or indirectly) but should not change:
CAMERA_PARAM_SUPPORTED_MAXMETERINGAREAS -- mCurrentConfiguration.mMaxMeteringAreas
CAMERA_PARAM_SUPPORTED_MAXFOCUSAREAS -- mCurrentConfiguration.mMaxFocusAreas
CAMERA_PARAM_SUPPORTED_VIDEOSIZES -- mSeparateVideoAndPreviewSizesSupported
CAMERA_PARAM_LUMINANCE -- mLuminanceSupported
CAMERA_PARAM_FLASHMODE -- mFlashSupported
CAMERA_PARAM_PICTURE_FILEFORMAT -- mFileFormat

The followed parameters are cached and may change:
CAMERA_PARAM_THUMBNAILSIZE -- mLastThumbnailSize
CAMERA_PARAM_PICTURE_SIZE -- mCurrentConfiguration.mPictureSize
CAMERA_PARAM_PREVIEWSIZE -- mCurrentConfiguration.mPreviewSize
CAMERA_PARAM_VIDEOSIZE -- mLastRecorderSize
(Reporter)

Updated

3 years ago
Depends on: 985494
(Reporter)

Updated

3 years ago
Blocks: 985494
No longer depends on: 985494
(Reporter)

Updated

3 years ago
Assignee: nobody → royang51
(Assignee)

Comment 1

3 years ago
Created attachment 8601855 [details] [diff] [review]
bug1124338, v1
Attachment #8601855 - Flags: review?(aosmond)
(Assignee)

Comment 2

3 years ago
Created attachment 8602394 [details] [diff] [review]
bug1124338, v2
Attachment #8601855 - Attachment is obsolete: true
Attachment #8601855 - Flags: review?(aosmond)
Attachment #8602394 - Flags: review?(aosmond)
(Reporter)

Comment 3

3 years ago
Comment on attachment 8602394 [details] [diff] [review]
bug1124338, v2

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

::: dom/camera/GonkCameraControl.cpp
@@ +1138,5 @@
> +      rv = mParams.Get(CAMERA_PARAM_PREVIEWSIZE, mCurrentConfiguration.mPreviewSize);
> +  }
> +  if (NS_SUCCEEDED(rv)) {
> +      rv = mParams.Get(CAMERA_PARAM_VIDEOSIZE, mLastRecorderSize);
> +  }

Indentation should be two spaces. The return values actually should be independent (i.e. while it *shouldn't* the camera could in theory choose to clear the thumbnail size, while still having a valid picture/preview/recorder size) so they shouldn't chain like this. I'd say just ignore them.

Otherwise this looks fine. Now just add the test case :).
Attachment #8602394 - Flags: review?(aosmond) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8607920 [details] [diff] [review]
bug1124338, v3
Attachment #8602394 - Attachment is obsolete: true
Attachment #8607920 - Flags: review?(aosmond)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8607920 [details] [diff] [review]
bug1124338, v3

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

Almost there :).

::: dom/camera/test/test_camera_fake_parameters.html
@@ +532,5 @@
> +    var sync = new Promise(function(resolve, reject) {
> +      function onEvent(e) {
> +        suite.camera.removeEventListener('focus', onEvent);
> +        ok(e.newState === 'focused', 'autofocus event state focusing == ' + e.newState);
> +        ok(suite.hw.params['preview-size'] === '640x480', 'preview size reset with auto focus');

What is stored in suite.hw.params is the simulated camera hardware. What you want to check here is that the Gecko code got configured correctly. You should check that via the DOM API. Preview size is a bit tricky, I'd recommend using the thumbnail size:

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#277

The thumbnail parameter names are here:

https://dxr.mozilla.org/mozilla-central/source/dom/camera/FallbackCameraPlatform.cpp#36

You should also configure a default thumbnail size which is different from the new one you set in triggerAutoFocus, so you can verify the original value, and the new value when it should have changed.

@@ +539,5 @@
> +      suite.camera.addEventListener('focus', onEvent);
> +    });
> +
> +    suite.hw.params['preview-size'] = '640x480';
> +    suite.hw.fireAutoFocusMoving(false);

This works because of a timeout in nsGonkCameraControl::OnAutoFocusMoving which triggers nsGonkCameraControl::OnAutoFocusComplete. I would just change it to do OnAutoFocusComplete instead, safer and faster.

@@ +542,5 @@
> +    suite.hw.params['preview-size'] = '640x480';
> +    suite.hw.fireAutoFocusMoving(false);
> +    return sync;
> +  }
> +  

Remove whitespace (the line is fine, just remove the spaces on the line).
Attachment #8607920 - Flags: review?(aosmond) → review-
(Assignee)

Comment 6

3 years ago
Created attachment 8609891 [details] [diff] [review]
bug1124338, v4
Attachment #8607920 - Attachment is obsolete: true
Attachment #8609891 - Flags: review?(aosmond)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8609891 [details] [diff] [review]
bug1124338, v4

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

LGTM.
Attachment #8609891 - Flags: review?(aosmond) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a357869a2095
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.