1052821 - [Camera][Gecko] Expose metering modes to JS

Status: RESOLVED FIXED

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

Description

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

Attachment: WIP - expose exposure (heh) metering modes, v1 (from bug 1048454)

Comment 1

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

From bug 1048454 comment 8, Flame reports some curious values:

 - metering modes: default mode 'frame-average', supported modes:
     'frame-average'
     'center-weighted'
     'spot-metering'
     'center-weighted'
     'spot-metering-adv'
     'center-weighted-adv'

No idea what the 'adv' modes are; not sure why there are two 'center-weighted' modes.

Comment 2

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

Attachment: Expose metering modes, v2

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

Comment 3

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

Note that attachment 8531038 [details] [diff] [review] will automatically bump metering mode to 'center-weighted' which seems to improve picture quality on Flame.

Comment 4

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

Comment on attachment 8531038 [details] [diff] [review]
Expose metering modes, v2

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

r+ conditional on addressing that below.

::: dom/camera/GonkCameraControl.cpp
@@ +227,5 @@
>    }
>  
> +  nsString mode;
> +  mParams.Get(CAMERA_PARAM_METERINGMODE, mode);
> +  if (!mode.IsEmpty()) {

If the mode is empty and center weighted is supported, don't we still want to reset the default?

@@ +241,5 @@
> +      DOM_CAMERA_LOGI("     '%s'\n", NS_ConvertUTF16toUTF8(modes[i]).get());
> +      if (modes[i].EqualsASCII(kCenterWeighted)) {
> +        centerWeightSupport = i;
> +      }
> +    }

We should break as soon as the center weight is found. Is there any reason we use a while instead of a for loop for less LOC?

Comment 5

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

Attachment: Expose metering modes, v2.1

Andrew, asking for a re-review since I juggled some code in GCC.cpp.

bz, boilerplate addition of exposure mode settings to JS need WebIDL-blessage.

Comment 6

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

Comment on attachment 8534679 [details] [diff] [review]
Expose metering modes, v2.1

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

r+ with below fixed or commented on.

::: dom/camera/DOMCameraCapabilities.cpp
@@ +677,5 @@
> +  if (NS_WARN_IF(!mCameraControl)) {
> +    return;
> +  }
> +
> +  if (mMeteringModes.Length() == 0) {

nit: I think .IsEmpty() is nicer :).

On a more serious note, if the WebIDL layer has caching enabled, do we really need to cache them again in our layer? We could probably save some memory by eliminating it. I'm not sure how often the JS engine will garbage collect these optimized cache bits, but I assume it would only do so if it was getting low on memory, in which case not using the memory here would be a good idea too...?

::: dom/camera/GonkCameraControl.cpp
@@ +245,5 @@
> +      }
> +    }
> +
> +    mParams.Get(CAMERA_PARAM_METERINGMODE, mode);
> +    DOM_CAMERA_LOGI(" - metering mode:                 '%s'\n", kCenterWeighted);

What's the point of the .Get? We should print mode, not the constant, or remove line 248. I favour the former :).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I'm not sure how often the JS engine will garbage collect these optimized cache bits

A Web IDL object with a [Cached] attribute will get pinned to the lifetime of its C++ reflection if that attribute is gotten, because the cached value disappearing is observable so we can't allow that to happen on GC.

Comment 8

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

(In reply to Boris Zbarsky [:bz] from comment #7)
> > I'm not sure how often the JS engine will garbage collect these optimized cache bits
> 
> A Web IDL object with a [Cached] attribute will get pinned to the lifetime
> of its C++ reflection if that attribute is gotten, because the cached value
> disappearing is observable so we can't allow that to happen on GC.

Couldn't we switch to Pure instead of Constant, get the caching benefit, without the constraint of being unable to change/fail?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That doesn't matter.  [Cached] means "cached until manually cleared", so the binding system ensures it doesn't go away until you manually clear it.

Comment 10

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

Attachment: Expose metering modes, v2.2 r=aosmond

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

Still needs WebIDL review.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8535040 [details] [diff] [review]
Expose metering modes, v2.2 r=aosmond

Andrew is right.  mMeteringModes seems totally unnecessary and you can just get into retval directly.

r=me with that.

Comment 12

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

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #11)

> Andrew is right.  mMeteringModes seems totally unnecessary and you can just
> get into retval directly.

Yeah, I was planning to remove those in a different patch, but I can bundle it into this one.

Comment 13

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

Attachment: Expose metering modes, v2.3 r=bz

Removing caching of capabilities in DOMCameraCapabilities.*. The WebIDL framework will handle this for us.

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

Andrew, would you mind giving this a once-over to make sure I didn't botch anything?

Comment 14

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

Comment on attachment 8535138 [details] [diff] [review]
Expose metering modes, v2.3 r=bz

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

LGTM.

Comment 15

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

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

Comment 16

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

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

Comment 17

[Carsten Book [:Tomcat]]

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