1124338 - GonkCameraControl::PullParametersImpl may invalidate cached parameters

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 1

[royang51]

Attachment: bug1124338, v1

Comment 2

[royang51]

Attachment: bug1124338, v2

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

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 :).

Comment 4

[royang51]

Attachment: bug1124338, v3

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

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).

Comment 6

[royang51]

Attachment: bug1124338, v4

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

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

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

LGTM.

Comment 8

[royang51]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2934d487bd38

Comment 9

[Pulsebot]

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

Comment 10

[Phil Ringnalda (:philor)]

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