If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use float type for SetAudioOutputVolume

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
I think that max volume is 1.0f, so we should not use uint32_t for volume parameter.
(Assignee)

Updated

2 years ago
Assignee: nobody → m_kato
(Assignee)

Comment 1

2 years ago
Created attachment 8651600 [details] [diff] [review]
Use float type for SetAudioOutputVolume
(Assignee)

Updated

2 years ago
Attachment #8651600 - Attachment is obsolete: true
(Assignee)

Comment 2

2 years ago
Created attachment 8651618 [details] [diff] [review]
Use float type for SetAudioOutputVolume
(Assignee)

Comment 3

2 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 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

2 years ago
Created attachment 8652706 [details] [diff] [review]
Use float type for SetAudioOutputVolume v2
Attachment #8651618 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Comment on attachment 8652706 [details] [diff] [review]
Use float type for SetAudioOutputVolume v2

Fix WindowVolumeChanged too.
Attachment #8652706 - Flags: review?(eitan)
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.
Flags: needinfo?(m_kato)
(Assignee)

Comment 8

2 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)
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
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)
Attachment #8652706 - Flags: review?(eitan) → review+

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd0c566464b
https://hg.mozilla.org/mozilla-central/rev/fdd0c566464b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.