1104913 - [Camera][Gecko] Provide sane defaults to the CameraControl API

Status: RESOLVED FIXED

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

Description

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

To encourage 3rd-party camera apps, we need to smooth some of the sharp edges off of the camera API. One good place to start is providing sane defaults to getCamera() and setConfiguration().

Ideally, the API user should be able to do:

  getCamera().then(function(camera) { camera.takePicture(); });

...and the API should, by default, open a suitable camera (i.e. the forward-facing one) and select 'picture' mode, configured for the best picture size.

Similarly,

  getCamera('video').then(function(camera) { camera.startRecording(...); });

...should open a suitable default camera, configure it for best-quality recording, and start the recording process.

Comment 1

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

Attachment: Sane defaults and improved preview selection algorithm, v1

A few things to note:

1) This gracefully breaks the camera app in a slightly worse off fashion than bug 1098660. The video preview looks fine but the picture preview is too small. App needs to be updated to listen on config changes to resolve this.

2) Our test cases appear to expect the recording profile to be set even if we don't validate it. (See FIXME in GonkCameraControl::SetConfigurationInternal).

3) The new test case doesn't try to take a picture. The emulated camera crashes after reinterpret casting a bad pointer instead its take_picture method. It seems to work fine on flame, so I blame the sketchy emulator which we will be moving away from anyways.

Comment 2

[Wilson Page [:wilsonpage]]

Wow you guys are quick! Few questions:

1. Mikes rough examples above seem to use a promise API, will that be the case?
2. .getCamera() currently takes an options object as the first argument, will that remain?
3. Do I need to now remove all preview-size selection logic from the app and instead keep a reference to the preview-size chosen by Gecko?

Comment 3

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

(In reply to Wilson Page [:wilsonpage] from comment #2)
> Wow you guys are quick! Few questions:
> 
> 1. Mikes rough examples above seem to use a promise API, will that be the
> case?
> 2. .getCamera() currently takes an options object as the first argument,
> will that remain?
> 3. Do I need to now remove all preview-size selection logic from the app and
> instead keep a reference to the preview-size chosen by Gecko?

1) The promise API is not required but is currently in master and should work (unless you need to attach to the preview stream first).

2) Yes. I didn't change any of the API prototypes beyond adding pictureSize to the configuration object because we select the preview size based on that and if you want to set a different one you can. You don't need to set any of the parameters however because I changed the default mode from unspecified to picture; it can fill in the rest for you. If you want to start with video mode, just supply { mode: 'video' }.

3) You don't have to remove preview size selection but it is recommended because we will "do the right thing" :). If you aren't using the full screen, I recommend you provide your container size for the preview rather than try to figure out the best one yourself after the fact.

Also note that if you update the picture size after configuring via set/getPictureSize instead of using setConfiguration, it will not update the preview size. Similar with the case where you provide the picture size in takePicture.

I will update the WebIDL with more comments to clarify these points.

Question for you:

1) I would like to port bug 1098660 to 2.1 in order to fix bug 1078435. However I now realize that that breaks the application with it not checking the updated preview size :). Do you have plans to land the app changes for this bug on 2.1? Or could you fix the regression from bug 1098660 in the app such that we could land that on 2.1?

Comment 4

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

Attachment: Sane defaults and improved preview selection algorithm, v1.1

Updated documentation in WebIDL.

Comment 5

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

Attachment: Sane defaults and improved preview selection algorithm, v2

Comment 6

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

Attachment: Delay get camera return until config set -- modified from bug 1099381, v1

Comment 7

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

Comment on attachment 8529201 [details] [diff] [review]
Delay get camera return until config set -- modified from bug 1099381, v1

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

Drivebyreview.

::: dom/camera/DOMCameraControl.cpp
@@ +311,5 @@
>  #endif
>      // Start the camera...
>      if (haveInitialConfig) {
>        rv = mCameraControl->Start(&config);
> +      if (NS_SUCCEEDED(rv)) {

#ifdef MOZ_WIDGET_GONK

@@ +321,5 @@
>  #ifdef MOZ_WIDGET_GONK
>    } else {
>      if (haveInitialConfig) {
>        rv = mCameraControl->SetConfiguration(config);
> +      if (NS_SUCCEEDED(rv)) {

#ifdef MOZ_WIDGET_GONK

@@ +1276,5 @@
>    switch (aState) {
>      case CameraControlListener::kHardwareOpen:
>        DOM_CAMERA_LOGI("DOM OnHardwareStateChange: open\n");
>        MOZ_ASSERT(aReason == NS_OK);
> +      if (!mSetInitialConfig) {

#ifdef MOZ_WIDGET_GONK

@@ +1465,5 @@
>  nsDOMCameraControl::OnConfigurationChange(DOMCameraConfiguration* aConfiguration)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aConfiguration != nullptr);
> +  

nit: trailing whitespace.

Comment 8

[Wilson Page [:wilsonpage]]

Attachment: pull-request (v2.1)

Comment 9

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

Attachment: Sane defaults and improved preview selection algorithm, v3

Now validates the initial configuration provided and will set the defaults for the other mode if not given.

Comment 10

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

Attachment: Delay get camera return until config set -- modified from bug 1099381, v2

(In reply to Mike Habicher [:mikeh] from comment #7)
> Comment on attachment 8529201 [details] [diff] [review]
> Delay get camera return until config set -- modified from bug 1099381, v1
> 
> Review of attachment 8529201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drivebyreview.
> 
> ::: dom/camera/DOMCameraControl.cpp
> @@ +311,5 @@
> >  #endif
> >      // Start the camera...
> >      if (haveInitialConfig) {
> >        rv = mCameraControl->Start(&config);
> > +      if (NS_SUCCEEDED(rv)) {
> 
> #ifdef MOZ_WIDGET_GONK
> 

Actually it shouldn't have MOZ_WIDGET_GONK since mSetInitialConfig can be true without the cached camera. But I wasn't using it right so thanks for pointing it out :).

> 
> @@ +1465,5 @@
> >  nsDOMCameraControl::OnConfigurationChange(DOMCameraConfiguration* aConfiguration)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(aConfiguration != nullptr);
> > +  
> 
> nit: trailing whitespace.

Fixed.

Comment 11

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

Attachment: Sane defaults and improved preview selection algorithm, v3.1

Remove some debugging prints I forgot about.

Comment 12

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

Comment on attachment 8529228 [details] [review]
pull-request (v2.1)

LGTM. Usage of the updated APIs looks good to me. I had some trouble loading but I'm not sure whose fault that is.

Although this is getting far more complicated for 2.1 than I had originally intended. We would need to land three separate and large gecko patches (this, bug 1098660 and bug 1054803) in addition to your gaia patch. All to simply fix bug 1078435.

It is really because the aspect ratio of the video is not being taken into account for the optimal preview size. I spent some time tonight to see how easy it would be to make a gaia patch for it, and I think it is preferable. Much easier to revert if I mess up too.

[1] https://github.com/mozilla-b2g/gaia/pull/26506

Comment 13

[Wilson Page [:wilsonpage]]

(In reply to Andrew Osmond [:aosmond] from comment #12)
> It is really because the aspect ratio of the video is not being taken into
> account for the optimal preview size. I spent some time tonight to see how
> easy it would be to make a gaia patch for it, and I think it is preferable.
> Much easier to revert if I mess up too.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/26506

I'm happy to go either-way for v2.1, so just let me know :)

Comment 14

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

Attachment: Sane defaults and improved preview selection algorithm, v4

Update algorithm to restrict preview sizes to those the same or lesser size than the capture size.

Comment 15

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

Attachment: Sane defaults and improved preview selection algorithm, v5

Fix some small bugs now that bug 804359 has landed, merge two patches.

Comment 16

[Justin D'Arcangelo [:justindarc]]

NI? myself so I can remember to build and test this.

Comment 17

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

Comment on attachment 8532219 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v5

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

Looks good. Please make the changes below and re-r?.

::: dom/camera/DOMCameraControl.cpp
@@ +350,5 @@
>    return nsDOMCameraManager::IsWindowStillActive(mWindow->WindowID());
>  }
>  
> +void
> +nsDOMCameraControl::SelectPreviewSize(const CameraSize& aPreviewSize, ICameraControl::Size& aSize)

'aRequestedPreviewSize' as the input, and 'aSelectedPreviewSize' as output.

@@ +359,5 @@
> +  } else {
> +    /* Use the window width and height if no preview size is provided.
> +       Note that the width and height are actually reversed from the
> +       camera perspective. */
> +    int32_t width, height;

Declarations on separate lines, please.

@@ +360,5 @@
> +    /* Use the window width and height if no preview size is provided.
> +       Note that the width and height are actually reversed from the
> +       camera perspective. */
> +    int32_t width, height;
> +    float ratio;

nit: blank line after the declaration block.

@@ +363,5 @@
> +    int32_t width, height;
> +    float ratio;
> +    mWindow->GetDevicePixelRatio(&ratio);
> +    mWindow->GetInnerWidth(&height);
> +    mWindow->GetInnerHeight(&width);

MOZ_ASSERT() that height and width are >= 0, please. Also, handle error from GetInnerWidth/Height(), please.

::: dom/camera/GonkCameraControl.cpp
@@ +249,5 @@
>  
>  nsresult
> +nsGonkCameraControl::ValidateConfiguration(const Configuration& aConfig, Configuration& aValidatedConfig)
> +{
> +  nsAutoTArray<Size, 8> supportedSizes;

Is 8 enough, e.g. for Flame? If not, just set the initial size guess to the list size returned by Flame.

@@ +250,5 @@
>  nsresult
> +nsGonkCameraControl::ValidateConfiguration(const Configuration& aConfig, Configuration& aValidatedConfig)
> +{
> +  nsAutoTArray<Size, 8> supportedSizes;
> +  Get(CAMERA_PARAM_SUPPORTED_PICTURESIZES, supportedSizes);

I assume we catch any errors in this Get() call below?

@@ +286,5 @@
>  {
>    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
>  
> +  // Ensure sanity of all provided parameters and determine defaults if
> +  // none provided when given a new configuration

"none are provided when given a new configuration."

@@ +288,5 @@
>  
> +  // Ensure sanity of all provided parameters and determine defaults if
> +  // none provided when given a new configuration
> +  Configuration config;
> +  nsresult rv = ValidateConfiguration(aConfig, config);

If you haven't already done so, please verify that this function doesn't add a significant amount of time to the camera start-up.

@@ +935,5 @@
>      DOM_CAMERA_LOGW("Ignoring requested picture size of %ux%u\n", aSize.width, aSize.height);
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> +  if (aSize.width == mCurrentConfiguration.mPictureSize.width && aSize.height == mCurrentConfiguration.mPictureSize.height) {

Break this line after the && please.

@@ +1484,5 @@
> +  DOM_CAMERA_LOGI("Select capture size %ux%u, preview size %ux%u, maximum size %ux%u\n",
> +                  aCaptureSize.width, aCaptureSize.height,
> +                  aPreviewSize.width, aPreviewSize.height,
> +                  aMaxSize.width, aMaxSize.height);
> + 

nit: trailing whitespace.

@@ +1491,4 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
> + 

nit: ditto.

@@ +1522,5 @@
> +    // preview size must be smaller or equal to the capture size
> +    if (aCaptureSize.width < s.width || aCaptureSize.height < s.height) {
> +      continue;
> +    }
> + 

nit: whitespace.

@@ +1929,3 @@
>      // Limit profiles to those video sizes supported by the camera hardware...
>      for (nsTArray<RecorderProfile>::size_type i = 0; i < profiles.Length(); ++i) {
>        int width = profiles[i]->GetVideo().GetSize().width; 

nit: trailing whitespace.

@@ +1951,5 @@
> +
> +    // Default profile is the one with the largest area.
> +    if (bestAreaMatch > 0) {
> +      nsAutoString name;
> +      name.AssignASCII("");

name.Truncate(); please.

Actually, change this as we discussed offline: "default" in the WebIDL, etc.

::: dom/camera/ICameraControl.h
@@ +142,5 @@
>  
>    struct Configuration {
>      Mode      mMode;
>      Size      mPreviewSize;
> +    Size      mPictureSize;

See my comments in the WebIDL about this change.

::: dom/webidl/CameraConfigurationEvent.webidl
@@ +9,5 @@
>  {
>    readonly attribute CameraMode mode;
>    readonly attribute DOMString recorderProfile;
>    readonly attribute DOMRectReadOnly? previewSize;
> +  readonly attribute DOMRectReadOnly? pictureSize;

I originally intended CameraConfiguration to reflect the most basic information required to get the camera up and running. I'm not sure that 'pictureSize' is one of those. This can always be got from getPictureSize().

::: dom/webidl/CameraControl.webidl
@@ +291,5 @@
>    /* the size of the picture to be returned by a call to takePicture();
>       an object with 'height' and 'width' properties that corresponds to
> +     one of the options returned by capabilities.pictureSizes.
> +
> +     note that unlike when one uses setConfiguration instead to update the

Capitalize "note".

@@ +424,5 @@
>  
>    /* the event dispatched when the camera is successfully configured.
>  
> +     event type is CameraConfigurationEvent where it has the same members as
> +     CameraConfiguration. */

Ditto.

::: dom/webidl/CameraManager.webidl
@@ +17,5 @@
>  
> +/* Pre-emptive camera configuration options. If mode is set to unspecified, the
> +   camera will not be configured immediately. If the mode is set to video or
> +   picture, then the camera automatically configures itself and will be ready
> +   for use upon return.

Put "unspecified", "video", and "picture" in quotes, and indicate that any other value will be interpreted as either "unspecified".

Comment 18

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

Attachment: Sane defaults and improved preview selection algorithm, v6

(In reply to Mike Habicher [:mikeh] from comment #17)
> Comment on attachment 8532219 [details] [diff] [review]
> Sane defaults and improved preview selection algorithm, v5
> 
> Review of attachment 8532219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Please make the changes below and re-r?.
> 
> ::: dom/camera/DOMCameraControl.cpp
> @@ +350,5 @@
> >    return nsDOMCameraManager::IsWindowStillActive(mWindow->WindowID());
> >  }
> >  
> > +void
> > +nsDOMCameraControl::SelectPreviewSize(const CameraSize& aPreviewSize, ICameraControl::Size& aSize)
> 
> 'aRequestedPreviewSize' as the input, and 'aSelectedPreviewSize' as output.
> 

Done.

> @@ +359,5 @@
> > +  } else {
> > +    /* Use the window width and height if no preview size is provided.
> > +       Note that the width and height are actually reversed from the
> > +       camera perspective. */
> > +    int32_t width, height;
> 
> Declarations on separate lines, please.
> 

Done.

> @@ +360,5 @@
> > +    /* Use the window width and height if no preview size is provided.
> > +       Note that the width and height are actually reversed from the
> > +       camera perspective. */
> > +    int32_t width, height;
> > +    float ratio;
> 
> nit: blank line after the declaration block.
> 

Done.

> @@ +363,5 @@
> > +    int32_t width, height;
> > +    float ratio;
> > +    mWindow->GetDevicePixelRatio(&ratio);
> > +    mWindow->GetInnerWidth(&height);
> > +    mWindow->GetInnerHeight(&width);
> 
> MOZ_ASSERT() that height and width are >= 0, please. Also, handle error from
> GetInnerWidth/Height(), please.
> 

Done.

> ::: dom/camera/GonkCameraControl.cpp
> @@ +249,5 @@
> >  
> >  nsresult
> > +nsGonkCameraControl::ValidateConfiguration(const Configuration& aConfig, Configuration& aValidatedConfig)
> > +{
> > +  nsAutoTArray<Size, 8> supportedSizes;
> 
> Is 8 enough, e.g. for Flame? If not, just set the initial size guess to the
> list size returned by Flame.
> 

Done.

> @@ +250,5 @@
> >  nsresult
> > +nsGonkCameraControl::ValidateConfiguration(const Configuration& aConfig, Configuration& aValidatedConfig)
> > +{
> > +  nsAutoTArray<Size, 8> supportedSizes;
> > +  Get(CAMERA_PARAM_SUPPORTED_PICTURESIZES, supportedSizes);
> 
> I assume we catch any errors in this Get() call below?
> 

GetSupportedSize on the next line checks for an empty supported array and will fail which we handle.

> @@ +286,5 @@
> >  {
> >    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
> >  
> > +  // Ensure sanity of all provided parameters and determine defaults if
> > +  // none provided when given a new configuration
> 
> "none are provided when given a new configuration."
> 

Done.

> @@ +288,5 @@
> >  
> > +  // Ensure sanity of all provided parameters and determine defaults if
> > +  // none provided when given a new configuration
> > +  Configuration config;
> > +  nsresult rv = ValidateConfiguration(aConfig, config);
> 
> If you haven't already done so, please verify that this function doesn't add
> a significant amount of time to the camera start-up.
> 

I will verify again before checkin but my own testing suggests no regression. The camera application will also be able to reduce requests for parameter significantly which should more than make up for any losses.

> @@ +935,5 @@
> >      DOM_CAMERA_LOGW("Ignoring requested picture size of %ux%u\n", aSize.width, aSize.height);
> >      return NS_ERROR_INVALID_ARG;
> >    }
> >  
> > +  if (aSize.width == mCurrentConfiguration.mPictureSize.width && aSize.height == mCurrentConfiguration.mPictureSize.height) {
> 
> Break this line after the && please.
>

Done.
 
> @@ +1484,5 @@
> > +  DOM_CAMERA_LOGI("Select capture size %ux%u, preview size %ux%u, maximum size %ux%u\n",
> > +                  aCaptureSize.width, aCaptureSize.height,
> > +                  aPreviewSize.width, aPreviewSize.height,
> > +                  aMaxSize.width, aMaxSize.height);
> > + 
> 
> nit: trailing whitespace.
> 

Done.

> @@ +1491,4 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return rv;
> >    }
> > + 
> 
> nit: ditto.
> 

Done.

> @@ +1522,5 @@
> > +    // preview size must be smaller or equal to the capture size
> > +    if (aCaptureSize.width < s.width || aCaptureSize.height < s.height) {
> > +      continue;
> > +    }
> > + 
> 
> nit: whitespace.
> 

Done.

> @@ +1929,3 @@
> >      // Limit profiles to those video sizes supported by the camera hardware...
> >      for (nsTArray<RecorderProfile>::size_type i = 0; i < profiles.Length(); ++i) {
> >        int width = profiles[i]->GetVideo().GetSize().width; 
> 
> nit: trailing whitespace.
> 

Done

> @@ +1951,5 @@
> > +
> > +    // Default profile is the one with the largest area.
> > +    if (bestAreaMatch > 0) {
> > +      nsAutoString name;
> > +      name.AssignASCII("");
> 
> name.Truncate(); please.
> 
> Actually, change this as we discussed offline: "default" in the WebIDL, etc.
> 

Done.

> ::: dom/camera/ICameraControl.h
> @@ +142,5 @@
> >  
> >    struct Configuration {
> >      Mode      mMode;
> >      Size      mPreviewSize;
> > +    Size      mPictureSize;
> 
> See my comments in the WebIDL about this change.
> 
> ::: dom/webidl/CameraConfigurationEvent.webidl
> @@ +9,5 @@
> >  {
> >    readonly attribute CameraMode mode;
> >    readonly attribute DOMString recorderProfile;
> >    readonly attribute DOMRectReadOnly? previewSize;
> > +  readonly attribute DOMRectReadOnly? pictureSize;
> 
> I originally intended CameraConfiguration to reflect the most basic
> information required to get the camera up and running. I'm not sure that
> 'pictureSize' is one of those. This can always be got from getPictureSize().
> 

As discussed offline, picture size is now used in the preview size selection algorithm, just like recording profile. If we want minimal app intervention, we need to supply it with the configuration.

> ::: dom/webidl/CameraControl.webidl
> @@ +291,5 @@
> >    /* the size of the picture to be returned by a call to takePicture();
> >       an object with 'height' and 'width' properties that corresponds to
> > +     one of the options returned by capabilities.pictureSizes.
> > +
> > +     note that unlike when one uses setConfiguration instead to update the
> 
> Capitalize "note".
> 
> @@ +424,5 @@
> >  
> >    /* the event dispatched when the camera is successfully configured.
> >  
> > +     event type is CameraConfigurationEvent where it has the same members as
> > +     CameraConfiguration. */
> 
> Ditto.
> 

Discussed offline, all of the docs need to be changed to follow this convention, will do in a seperate bug.

> ::: dom/webidl/CameraManager.webidl
> @@ +17,5 @@
> >  
> > +/* Pre-emptive camera configuration options. If mode is set to unspecified, the
> > +   camera will not be configured immediately. If the mode is set to video or
> > +   picture, then the camera automatically configures itself and will be ready
> > +   for use upon return.
> 
> Put "unspecified", "video", and "picture" in quotes, and indicate that any
> other value will be interpreted as either "unspecified".

Done.

Comment 19

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

Comment on attachment 8534392 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v6

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

LGTM, with a few nits and a question.

::: dom/camera/GonkCameraControl.cpp
@@ +1480,5 @@
>    return rv;
>  }
>  
>  nsresult
> +nsGonkCameraControl::SelectCaptureAndPreviewSize(const Size& aPreviewSize, const Size& aCaptureSize, const Size& aMaxSize, uint32_t aCaptureSizeKey)

Please put the various arguments on their own lines.

::: dom/camera/test/test_bug1037322.html
@@ +56,5 @@
>        // Check the default configuration
>        ok(cfg.mode === "unspecified", "Initial mode = " + cfg.mode);
>        ok(cfg.previewSize.width === 0 && cfg.previewSize.height === 0,
>           "Initial preview size = " + cfg.previewSize.width + "x" + cfg.previewSize.height);
> +      ok(cfg.recorderProfile === "default",

Remind me: did we want 'default' as an input to cause 'default' as an output? Or should the returned config specify what the chosen default recorder profile actually is?

::: dom/webidl/CameraControl.webidl
@@ +292,5 @@
>       an object with 'height' and 'width' properties that corresponds to
> +     one of the options returned by capabilities.pictureSizes.
> +
> +     note that unlike when one uses setConfiguration instead to update the
> +     picture size, this will not recalculate the ideal preview size. */

Hmm, should it? :-/

@@ +423,5 @@
>                                                  optional CameraErrorCallback onError);
>  
>    /* the event dispatched when the camera is successfully configured.
>  
> +     event type is CameraConfigurationEvent where it has the same members as

s/where it/which/

::: dom/webidl/CameraManager.webidl
@@ +14,5 @@
>    unsigned long width = 0;
>    unsigned long height = 0;
>  };
>  
> +/* Pre-emptive camera configuration options. If mode is set to "unspecified", the

'mode'

@@ +20,5 @@
> +   "picture", then the camera automatically configures itself and will be ready
> +   for use upon return.
> +
> +   The remaining parameters are optional and are considered hints by the
> +   camera. The application should use the values returned in GetCameraCallback

...the GetCameraCallback...

@@ +24,5 @@
> +   camera. The application should use the values returned in GetCameraCallback
> +   configuration because while the camera makes a best effort to adhere to the
> +   requested values, it may need to change them to ensure optimal behavior.
> +
> +   The pictureSize and recorderProfile default to the best or highest

...'pictureSize' and 'recorderProfile'...

@@ +28,5 @@
> +   The pictureSize and recorderProfile default to the best or highest
> +   resolution supported by the camera hardware.
> +
> +   To determine previewSize, one should generally provide the size of the
> +   element which will contain the preview rather than guess which supported

...'previewSize'...

@@ +29,5 @@
> +   resolution supported by the camera hardware.
> +
> +   To determine previewSize, one should generally provide the size of the
> +   element which will contain the preview rather than guess which supported
> +   preview size is the best. It defaults to the inner window size. */

If not specified, 'previewSize' defaults to the inner window size.

Comment 20

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

Attachment: Sane defaults and improved preview selection algorithm, v7 [carries r=mikeh]

(In reply to Mike Habicher [:mikeh] from comment #19)
> Comment on attachment 8534392 [details] [diff] [review]
> Sane defaults and improved preview selection algorithm, v6
> 
> Review of attachment 8534392 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, with a few nits and a question.
> 
> ::: dom/camera/GonkCameraControl.cpp
> @@ +1480,5 @@
> >    return rv;
> >  }
> >  
> >  nsresult
> > +nsGonkCameraControl::SelectCaptureAndPreviewSize(const Size& aPreviewSize, const Size& aCaptureSize, const Size& aMaxSize, uint32_t aCaptureSizeKey)
> 
> Please put the various arguments on their own lines.
> 

Done.

> ::: dom/camera/test/test_bug1037322.html
> @@ +56,5 @@
> >        // Check the default configuration
> >        ok(cfg.mode === "unspecified", "Initial mode = " + cfg.mode);
> >        ok(cfg.previewSize.width === 0 && cfg.previewSize.height === 0,
> >           "Initial preview size = " + cfg.previewSize.width + "x" + cfg.previewSize.height);
> > +      ok(cfg.recorderProfile === "default",
> 
> Remind me: did we want 'default' as an input to cause 'default' as an
> output? Or should the returned config specify what the chosen default
> recorder profile actually is?
> 

It will replace default with the proper name of the recorder profile, but only if you actually configure the camera. When the mode is unspecified no configuration happens because DOMCameraControl doesn't pass the given configuration down to GonkCameraControl to process.

> ::: dom/webidl/CameraControl.webidl
> @@ +292,5 @@
> >       an object with 'height' and 'width' properties that corresponds to
> > +     one of the options returned by capabilities.pictureSizes.
> > +
> > +     note that unlike when one uses setConfiguration instead to update the
> > +     picture size, this will not recalculate the ideal preview size. */
> 
> Hmm, should it? :-/
> 

In my opinion, no. There is already a mechanism to update the preview size based on the capture size, and that is set configuration. If it is important to an application to re-size the preview, it should use that instead. It is a big weird though; perhaps we should deprecate the API.

> @@ +423,5 @@
> >                                                  optional CameraErrorCallback onError);
> >  
> >    /* the event dispatched when the camera is successfully configured.
> >  
> > +     event type is CameraConfigurationEvent where it has the same members as
> 
> s/where it/which/
> 

Done.

> ::: dom/webidl/CameraManager.webidl
> @@ +14,5 @@
> >    unsigned long width = 0;
> >    unsigned long height = 0;
> >  };
> >  
> > +/* Pre-emptive camera configuration options. If mode is set to "unspecified", the
> 
> 'mode'
> 

Done.

> @@ +20,5 @@
> > +   "picture", then the camera automatically configures itself and will be ready
> > +   for use upon return.
> > +
> > +   The remaining parameters are optional and are considered hints by the
> > +   camera. The application should use the values returned in GetCameraCallback
> 
> ...the GetCameraCallback...
> 

Done.

> @@ +24,5 @@
> > +   camera. The application should use the values returned in GetCameraCallback
> > +   configuration because while the camera makes a best effort to adhere to the
> > +   requested values, it may need to change them to ensure optimal behavior.
> > +
> > +   The pictureSize and recorderProfile default to the best or highest
> 
> ...'pictureSize' and 'recorderProfile'...
> 

Done.

> @@ +28,5 @@
> > +   The pictureSize and recorderProfile default to the best or highest
> > +   resolution supported by the camera hardware.
> > +
> > +   To determine previewSize, one should generally provide the size of the
> > +   element which will contain the preview rather than guess which supported
> 
> ...'previewSize'...
> 

Done.

> @@ +29,5 @@
> > +   resolution supported by the camera hardware.
> > +
> > +   To determine previewSize, one should generally provide the size of the
> > +   element which will contain the preview rather than guess which supported
> > +   preview size is the best. It defaults to the inner window size. */
> 
> If not specified, 'previewSize' defaults to the inner window size.

Done.

Comment 21

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

Attachment: Sane defaults and improved preview selection algorithm, v7.1 [carries r=mikeh]

Minor optimization, switches camera sensor angle to be constant/cached in WebIDL as the app frequently queries this but it never changes.

Comment 22

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

Comment on attachment 8534603 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v7.1 [carries r=mikeh]

bz: This changes how preview sizes are selected and grants a lot of leeway in gecko to control what is used in the end (we could change it before but it wasn't properly documented).

Comment 23

[Boris Zbarsky [:bzbarsky]]

Are you looking for my review just on the IDL-related bits here, or the whole thing?

Comment 24

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

bz: Just the WebIDL.

Comment 25

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

Attachment: Sane defaults and improved preview selection algorithm, v7.2 [carries r=mikeh]

Remove temporary debug prints that were forgotten.

Comment 26

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

Attachment: Change gaia to rely upon defaults provided by camera app, v1

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8535031 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v7.2 [carries r=mikeh]

What's the point of making sensorAngle cached?  I'm not sure that's worth the memory tradeoff.

Apart from that, looks fine on the IDL end.

Comment 28

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

Attachment: Sane defaults and improved preview selection algorithm, v8 [carries r=bz]

Found a bug where we didn't handle open failures from the camera when given an initial configuration. Added a new test case to verify this.

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #27)
> Comment on attachment 8535031 [details] [diff] [review]
> Sane defaults and improved preview selection algorithm, v7.2 [carries
> r=mikeh]
> 
> What's the point of making sensorAngle cached?  I'm not sure that's worth
> the memory tradeoff.
> 
> Apart from that, looks fine on the IDL end.

I added that because the app is querying this value quite often (several times a second at least, more if faces are detected) and we have a few levels of indirection in order to read that value. I could cache in the application if that is preferred.

Comment 29

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

Comment on attachment 8535708 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v8 [carries r=bz]

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

This looks fine, except for the change to AddListenerImpl(). What's going on there?

Holding r+ off pending an explanation for why some state isn't being pushed.

::: dom/camera/CameraControlImpl.cpp
@@ +706,5 @@
> +    l->OnHardwareStateChange(mHardwareState, mHardwareStateChangeReason);
> +  }
> +  if (mPreviewState != CameraControlListener::kPreviewStopped) {
> +    l->OnPreviewStateChange(mPreviewState);
> +  }

Why did you change this?

::: dom/camera/DOMCameraControl.cpp
@@ +275,5 @@
>      // Start the camera...
>      if (haveInitialConfig) {
>        rv = mCameraControl->Start(&config);
> +      if (NS_SUCCEEDED(rv)) {
> +        mSetInitialConfig = true;

Good catch.

Comment 30

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

(In reply to Mike Habicher [:mikeh] from comment #29)
> Comment on attachment 8535708 [details] [diff] [review]
> Sane defaults and improved preview selection algorithm, v8 [carries r=bz]
> 
> Review of attachment 8535708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, except for the change to AddListenerImpl(). What's going on
> there?
> 
> Holding r+ off pending an explanation for why some state isn't being pushed.
> 
> ::: dom/camera/CameraControlImpl.cpp
> @@ +706,5 @@
> > +    l->OnHardwareStateChange(mHardwareState, mHardwareStateChangeReason);
> > +  }
> > +  if (mPreviewState != CameraControlListener::kPreviewStopped) {
> > +    l->OnPreviewStateChange(mPreviewState);
> > +  }
> 
> Why did you change this?

For the cached camera when starting the app, it works fine, but when I switch cameras, I get a kHardwareClosed/NS_OK event which is indistinguishable from the get camera + set configuration failed case. As far as I can tell, I can either solve this by adding additional state variables in DOMCameraControl, or I can suppress the listener events if the state hasn't changed from the default.

Comment 31

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

Attachment: Sane defaults and improved preview selection algorithm, v9 [carries r=bz]

Updated based on IRC discussion (add new hardware state to know the difference between closed vs not yet completed open).

Comment 32

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

Comment on attachment 8535906 [details] [diff] [review]
Sane defaults and improved preview selection algorithm, v9 [carries r=bz]

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

LGTM. Thanks, Andrew.

Comment 33

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

Attachment: Sane defaults and improved preview selection algorithm, v10 [carries r=mikeh,bz]

Rebase onto latest b2g-inbound.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa1e90e621f4

Comment 34

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

Attachment: Part 2. Update WebRTC to handle new camera hardware states.

Fix broken WebRTC test cases.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4946a2e884d2

Comment 35

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8535034 [details] [review]
Change gaia to rely upon defaults provided by camera app, v1

Finally got around to flashing and testing this. Looks great! Major code reduction win! \o/

Comment 36

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

The gecko changes can go in first with partially degraded preview (but otherwise functional camera). Gaia should follow soon after though.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Master: https://github.com/mozilla-b2g/gaia/commit/4ac6f6807e62de1b60dc88dd2b8d8dbbea63b0d4

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce9707a48002
https://hg.mozilla.org/mozilla-central/rev/e002b6b44e66