981047 - [Camera][Gecko] Expose luminance attribute to DOM

Status: RESOLVED FIXED

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

Description

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

The Camera app would like to get a luminance condition (either "high" or "low") from the camera library, to indicate whether or not it should turn on the flash LED (by setting flashMode="torch") when starting video recording.

The luminance condition is exposed by the library through the CameraParameters object, using a key value of "luminance-condition".

We are currently working with an implementation that updates the luminance condition inside the camera driver based on the current amount of light falling on the sensor. It does not, however, send a notification to camera clients indicating a change. Nor is it sufficient to simply call CameraParameters.get("luminance-condition") in Gecko because CameraParameters is a snapshot of the camera configuration from the last time we pulled a parameter set from the camera driver.

Current CameraControl attributes are read synchronously, directly from the CameraParameters object; pushing this object to and pulling it from the camera hardware is an asynchronous operation that happens off-Main Thread.

Other camera implementations may be able to notify us when conditions such as luminance change; many implementations will not support luminance reporting of any kind at all.

To cover all of these possibilities, a luminance API will need the following components:

  interface CameraCapabilities {
    // Values:
    //  - "": (falsy) indicates luminance reporting is not supported
    //  - "live": the CameraControl luminance attribute can be read directly
    //  - "cached": the attribute reflects a cached value that may be stale
    [Constant, Cached] readonly attribute DOMString luminanceMode;
  };

The corresponding CameraControl interface will then need:

  // Called when the luminance condition is updated, the argument is
  // either "high", "low", or "".
  CameraLuminanceCallback = void (DOMString luminance);

  interface CameraControl {
    // Values:
    //  - "", (falsy) indicates not supported; else "high" or "low"
    // If capabilities.luminance is:
    //  - "cached": this returns the last value pulled from the driver
    //  - "live": this returns the last value posted by the driver
    [Throws]
    readonly attribute DOMString luminance;

    // Method to get the current luminance; although this is intended
    // to be used when luminanceMode is "cached", it can be used for
    // "live" as well. If luminance reporting is not supported, the
    // success callback is called with an empty string argument.
    [Throws]
    void getLuminance(optional CameraLuminanceCallback onSuccess,
                      optional CameraErrorCallback onError);

    // If capabilities.luminance is:
    //  - "live": invoked whenever the driver posts a new luminance value
    //  - "cached": whenever getLuminance() gets a new value from the driver
    attribute CameraLuminanceCallback? onLuminanceChange;
  };

Comment 1

[(no longer active)]

I suggest a different API instead: adding a new boolean value to CameraStartRecordingOptions called flashInLowLight (bikeshedding welcome!).  Setting that value to true would put the flash into the torch mode if we detect a low light condition when startRecording is called, and setting it to false or dropping it would mean that the flash status should not be changed based on the luminance conditions.

In the future, if we have a use case for getting notified of the luminance changes at other times rather than when we call startRecording, we should expose that information as an event, which will require the way that the information is delivered to Gecko to change to be some kind of an event which we can subscribe to, but I think my proposal above will address the current use case in a much easier way, and it will be more efficient to implement as well, since we already to a roundtrip when startRecording is called to retrieve the updated camera parameters value and we can just piggyback on that.

Does this sound good?

Comment 2

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

Thanks for the comments, Ehsan. I like the simplicity of that approach.

Comment 3

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

Some pseudo-code:

// In Gecko
startRecording(options)
{
  mAutoFlashModeOverridden = false;

  if (options.autoEnableLowLightFlash) {
    PullParameters(); // we need a current set
    bool lowLight;
    mParams.Get("luminance-condition", lowLight);
    const char* flashMode;
    mParams.Get("flash-mode", flashMode);
    if (lowLight && flashMode == "auto") {
      mAutoFlashModeOverridden = true;
      mParams.Set("flash-mode", "torch");
    }
  }

  recorder->start();
}

stopRecording()
{
  if (mAutoFlashModeOverridden) {
    mParams.Set("flash-mode", "auto");
  }
  recorder->stop();
}

set(parameter, value)
{
  if (parameter == "flash-mode") {
    mParams.Set("flash-mode", value);
    // explicit mode set always wins and sticks
    mAutoFlashModeOverridden = false;
  }
  mParams.Set(parameter, value);
}

Comment 4

[(no longer active)]

(In reply to comment #3)
> Some pseudo-code:
> 
> // In Gecko
> startRecording(options)
> {
>   mAutoFlashModeOverridden = false;
> 
>   if (options.autoEnableLowLightFlash) {
>     PullParameters(); // we need a current set
>     bool lowLight;
>     mParams.Get("luminance-condition", lowLight);
>     const char* flashMode;
>     mParams.Get("flash-mode", flashMode);
>     if (lowLight && flashMode == "auto") {
>       mAutoFlashModeOverridden = true;
>       mParams.Set("flash-mode", "torch");
>     }
>   }
> 
>   recorder->start();
> }
> 
> stopRecording()
> {
>   if (mAutoFlashModeOverridden) {
>     mParams.Set("flash-mode", "auto");
>   }
>   recorder->stop();
> }
> 
> set(parameter, value)
> {
>   if (parameter == "flash-mode") {
>     mParams.Set("flash-mode", value);
>     // explicit mode set always wins and sticks
>     mAutoFlashModeOverridden = false;
>   }
>   mParams.Set(parameter, value);
> }

This is exactly what I had in mind in comment 1.  Thanks for putting this into more concrete terms!

Comment 5

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

Attachment: WIP - If supported, use scene luminance to control camera flash, v1

Here's an initial version of a patch that adds 'autoEnableLowLightTorch' to the startRecording() options. I don't have a device that supports luminance condition reporting, so I can't test it; but it builds.

There are currently no automated tests as the emulator doesn't support recording.

Comment 6

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

(This patch applies on top of those in bug 965421, bug 965420, and bug 981318.)

Comment 7

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

Attachment: If supported, use scene luminance to control camera flash, v2

Applies on top of the patches in bug 965421 and bug 965420.

Ehsan, the only DOM-facing change is the addition of the new attribute to the CameraStartRecordingOptions dictionary.

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 8389323 [details] [diff] [review]
If supported, use scene luminance to control camera flash, v2

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

::: dom/camera/GonkCameraControl.cpp
@@ +888,5 @@
> +nsGonkCameraControl::SetupRecordingFlash(bool aAutoEnableLowLightTorch)
> +{
> +  mAutoFlashModeOverridden = false;
> +
> +  if (aAutoEnableLowLightTorch && mLuminanceSupported && mFlashSupported) {

nit: flip logic around and return early

Comment 9

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

Attachment: If supported, use scene luminance to control camera flash, v2.1, r=dhylands,ehsan

Incorporate review feedback; carrying r+en forward.

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

Comment 10

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

Comment on attachment 8389363 [details] [diff] [review]
If supported, use scene luminance to control camera flash, v2.1, r=dhylands,ehsan

Youngwoo, can you try out this patch with a modified Camera app to make sure it works as expected?

Comment 11

[Youngwoo Jo]

I verified the patch is working fine in our device.

Comment 12

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

Great--thanks, Youngwoo! Attachment 8389363 [details] [diff] is based on the patch for bug 965420, so this will land as soon as that one does.

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/958d209e788e