Closed Bug 1197673 Opened 4 years ago Closed 4 years ago

Use float type for SetAudioOutputVolume

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(1 file, 2 obsolete files)

I think that max volume is 1.0f, so we should not use uint32_t for volume parameter.
Assignee: nobody → m_kato
Attachment #8651600 - Attachment is obsolete: true
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-
Attachment #8651618 - Attachment is obsolete: true
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)
(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)
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+
https://hg.mozilla.org/mozilla-central/rev/fdd0c566464b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.