1197673 - Use float type for SetAudioOutputVolume

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

I think that max volume is 1.0f, so we should not use uint32_t for volume parameter.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Use float type for SetAudioOutputVolume

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Use float type for SetAudioOutputVolume

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8651618 [details] [diff] [review]
Use float type for SetAudioOutputVolume

SetAudioOutputVolume should use float instead of uint32_t

Comment 4

[Eitan Isaacson [:eeejay]]

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.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Use float type for SetAudioOutputVolume v2

Comment 6

[Makoto Kato [:m_kato]]

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

Fix WindowVolumeChanged too.

Comment 7

[Eitan Isaacson [:eeejay]]

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.

Comment 8

[Makoto Kato [:m_kato]]

(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?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know. Try baku...

Comment 10

[Eitan Isaacson [:eeejay]]

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

[Andrea Marchesini [:baku]]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd0c566464b

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/fdd0c566464b