Closed Bug 1052821 Opened 7 years ago Closed 7 years ago

[Camera][Gecko] Expose metering modes to JS

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Expose metering modes, v2 (obsolete) — Splinter Review
try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=116ed4d36320
Assignee: nobody → mhabicher
Attachment #8471866 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8531038 - Flags: review?(aosmond)
Note that attachment 8531038 [details] [diff] [review] will automatically bump metering mode to 'center-weighted' which seems to improve picture quality on Flame.
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?
Attachment #8531038 - Flags: review?(aosmond) → review+
Attached patch Expose metering modes, v2.1 (obsolete) — Splinter Review
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.
Attachment #8531038 - Attachment is obsolete: true
Attachment #8534679 - Flags: review?(bzbarsky)
Attachment #8534679 - Flags: review?(aosmond)
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 :).
Attachment #8534679 - Flags: review?(aosmond) → review+
> 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.
(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?
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.
try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c0b245b244d8

Still needs WebIDL review.
Attachment #8534679 - Attachment is obsolete: true
Attachment #8534679 - Flags: review?(bzbarsky)
Attachment #8535040 - Flags: review?(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.
Attachment #8535040 - Flags: review?(bzbarsky) → review+
(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.
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?
Attachment #8535040 - Attachment is obsolete: true
Attachment #8535138 - Flags: review?(aosmond)
Comment on attachment 8535138 [details] [diff] [review]
Expose metering modes, v2.3 r=bz

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

LGTM.
Attachment #8535138 - Flags: review?(aosmond) → review+
https://hg.mozilla.org/mozilla-central/rev/cf975e8c033d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.