Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

types of setting objects #77

Closed
fluffy opened this issue Oct 4, 2016 · 8 comments
Closed

types of setting objects #77

fluffy opened this issue Oct 4, 2016 · 8 comments

Comments

@fluffy
Copy link

fluffy commented Oct 4, 2016

Attributes such as exposureCompensation would be better as a double than a long x100. I can not see any disadvantage to using a double here.
@gmandyam
Copy link
Collaborator

@miguelao

Any objection to the suggestion of @fluffy?

@yellowdoge
Copy link
Member

@gmandyam, if you're ok I'm ok.
I'd suggest making it float which is generally as large as int (vs double which is usually larger) and I don't see any need for the extra ultra-precision.
I can write the PR then.

yellowdoge added a commit to yellowdoge/mediacapture-image that referenced this issue Nov 5, 2016
@yellowdoge
(w3c#77) s/long/float/ in MediaSettingsRange, and removed entries men…
e1e4fef 
…tioning a x100 multiplier to avoid floating point.
@yellowdoge
Copy link
Member

#120 is the PR, @fluffy, @gmandyam, PTAL

@dontcallmedom
Copy link
Member

@miguelao float is discouraged:

Unless there are specific reasons to use a 32 bit floating point type, specifications should use double rather than float, since the set of values that a double can represent more closely matches an ECMAScript Number.

@yellowdoge
Copy link
Member

@dontcallmedom, that's exactly what I mentioned in #120 (comment), but still "I feel that float can represent all of our value ranges and is smaller in size." Which one will win?

@dontcallmedom
Copy link
Member

but it's not really smaller in size - the underlying ecmascript engine only sees double.

@yellowdoge yellowdoge mentioned this issue Nov 7, 2016
Merged
yellowdoge added a commit that referenced this issue Nov 7, 2016
@yellowdoge
(#77) s/long/double/ in MediaSettingsRange (#120)
601abfe 
* (#77) s/long/double/ in MediaSettingsRange; removing entries mentioning a x100 multiplier to avoid floating point.

* Make it double
@yellowdoge
Copy link
Member

Closed per #120.

yellowdoge added a commit to yellowdoge/mediacapture-image that referenced this issue Nov 8, 2016
@yellowdoge
Follow up w3c#77 (pr w3c#120), s/unsigned long/double/ in PhotoSettin…
155bc7b 
…gs dictionary
yellowdoge added a commit that referenced this issue Nov 8, 2016
@yellowdoge
Follow up #77 (pr #120), s/unsigned long/double/ in PhotoSettings dic…
bea37a7 
…tionary (#121)
@yellowdoge
Copy link
Member

Landed in chrome, see https://crbug.com/663021.

xqq pushed a commit to xqq/chromium-media that referenced this issue May 17, 2017
@yellowdoge
MediaSettingsRange: s/long/double/ in MediaSettingsRange.idl and Phot…
86c074c 
…oCapabilities.idl

This CL follows issue [1] in replacing integer
types with doubles in the capabilities/settings idl
(WebIdl encourages doubles and discourages
floats).

The change is rippled down to the mojom and
the implementations.  In Android:
- Reading capabilities and status are changed to
using double.
- Configuring settings are changed to use doubles.
- zoom and exposure compensation setting/reading
don't need to have x100 multipliers anymore, removed.

Otherwise:
- v4l2_capture_delegate.cc bugfix: unnecessary scaling
of zoom during setting (unmatched on retrieving).
- FakeVideoCaptureDevice changes to doubles.

[1] w3c/mediacapture-image#77
BUG=663021

Review-Url: https://codereview.chromium.org/2482983002
Cr-Original-Commit-Position: refs/heads/master@{#431293}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 5a4284b492c9511bd41ccad6e53f339a6de409a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@dontcallmedom @fluffy @yellowdoge @gmandyam