Closed
Bug 1052821
Opened 10 years ago
Closed 10 years ago
[Camera][Gecko] Expose metering modes to JS
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mikeh, Assigned: mikeh)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 4 obsolete files)
28.50 KB,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → mhabicher
Attachment #8471866 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8531038 -
Flags: review?(aosmond)
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Comment 7•10 years ago
|
||
> 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•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•