Closed
Bug 1197673
Opened 9 years ago
Closed 9 years ago
Use float type for SetAudioOutputVolume
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
References
Details
Attachments
(1 file, 2 obsolete files)
5.02 KB,
patch
|
eeejay
:
review+
|
Details | Diff | Splinter Review |
I think that max volume is 1.0f, so we should not use uint32_t for volume parameter.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8651600 -
Attachment is obsolete: true
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8651618 [details] [diff] [review] Use float type for SetAudioOutputVolume SetAudioOutputVolume should use float instead of uint32_t
Attachment #8651618 -
Flags: review?(eitan)
Comment 4•9 years ago
|
||
Comment on attachment 8651618 [details] [diff] [review] Use float type for SetAudioOutputVolume Review of attachment 8651618 [details] [diff] [review]: ----------------------------------------------------------------- Good catch! I think the implementation in nsSpeechTask::WindowVolumeChanged also needs to be fixed, in light of these changes. Not sure how I didn't catch that earlier, but the multiplication there does not look right for either type.
Attachment #8651618 -
Flags: review?(eitan) → review-
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8651618 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8652706 [details] [diff] [review] Use float type for SetAudioOutputVolume v2 Fix WindowVolumeChanged too.
Attachment #8652706 -
Flags: review?(eitan)
Comment 7•9 years ago
|
||
Comment on attachment 8652706 [details] [diff] [review] Use float type for SetAudioOutputVolume v2 Review of attachment 8652706 [details] [diff] [review]: ----------------------------------------------------------------- OOps, didn't see this in my queue. Sorry. ::: dom/media/webspeech/synth/nsSpeechTask.cpp @@ +680,5 @@ > > NS_IMETHODIMP > nsSpeechTask::WindowVolumeChanged(float aVolume, bool aMuted) > { > + SetAudioOutputVolume(aMuted ? 0.0 : mVolume * aVolume); Is the volume from WindowVolumeChanged relative or absolute? From other places it looks absolute, so it should be assigned, and not multiplied.
Updated•9 years ago
|
Flags: needinfo?(m_kato)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #7) > Comment on attachment 8652706 [details] [diff] [review] > Use float type for SetAudioOutputVolume v2 > > Review of attachment 8652706 [details] [diff] [review]: > ----------------------------------------------------------------- > > OOps, didn't see this in my queue. Sorry. > > ::: dom/media/webspeech/synth/nsSpeechTask.cpp > @@ +680,5 @@ > > > > NS_IMETHODIMP > > nsSpeechTask::WindowVolumeChanged(float aVolume, bool aMuted) > > { > > + SetAudioOutputVolume(aMuted ? 0.0 : mVolume * aVolume); > > Is the volume from WindowVolumeChanged relative or absolute? From other > places it looks absolute, so it should be assigned, and not multiplied. Actually, aVolume is always 1.0 because window->SetAudioVolume isn't called by tab UI now. But it allows over 1.0 by bug 923247 comment #10. Roc, this question is design issue of bug 923247. I think that volume parameter of WindowVolumeChanged (Get/SetAudioVolume for audio indicator) is relative, is it right?
Flags: needinfo?(m_kato) → needinfo?(roc)
I don't know. Try baku...
Flags: needinfo?(roc) → needinfo?(amarchesini)
Comment 10•9 years ago
|
||
This is the other place where it looks like an absolute value: https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4546 https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4459
Comment 11•9 years ago
|
||
I and Eitan spoke on IRC and, no, the value is relative. Here what we do in HTMLMediaElement: float HTMLMediaElement::ComputedVolume() const { return mMuted ? 0.0f : float(mVolume * mAudioChannelVolume); } Where mVolume is the current volume of this element, and mAudioChannelVolume is what WindowVolumeChanged receives as param.
Flags: needinfo?(amarchesini)
Updated•9 years ago
|
Attachment #8652706 -
Flags: review?(eitan) → review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fdd0c566464b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•