Page::isPlayingAudio(), first added in https://bugs.webkit.org/show_bug.cgi?id=137218, does not take into account AudioNodes that are playing audio yet. We need to support that.
Created attachment 241821 [details] Patch
Comment on attachment 241821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241821&action=review > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:198 > + if (!result) { > m_isPlaying = true; > + updateIsEffectivelyPlayingAudio(); > + } We should add a "void setPlaying(bool)" to avoid duplicated code here... > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:213 > - if (!result) > + if (!result) { > m_isPlaying = false; > + updateIsEffectivelyPlayingAudio(); > + } ...And here. > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:245 > +void AudioDestinationIOS::updateIsSilent() > +{ > + bool isSilent = true; > + for (unsigned i = 0; i < m_renderBus->numberOfChannels(); ++i) { > + AudioChannel* channel = m_renderBus->channel(i); > + if (!channel->isSilent()) { > + isSilent = false; > + break; > + } > + } AudioBus has an isSilent() member which returns true if all channels are silent. You shouldn't have to write this loop yourself. In fact, this could probably be a "void setSilent(bool)" method, and you could call "setSilent(m_renderBus.isSilent())" from render() above. > Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:151 > - if (!result) > + if (!result) { > m_isPlaying = true; > + updateIsEffectivelyPlayingAudio(); > + } And since there's nothing platform specific here, perhaps we should move this up into the AudioDestinationNode class, in AudioDestinationNode::render(). > Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:161 > - if (!result) > + if (!result) { > m_isPlaying = false; > + updateIsEffectivelyPlayingAudio(); > + } Ditto. > Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:209 > +void AudioDestinationMac::updateIsSilent() > +{ > + bool isSilent = true; > + for (unsigned i = 0; i < m_renderBus->numberOfChannels(); ++i) { > + AudioChannel* channel = m_renderBus->channel(i); > + if (!channel->isSilent()) { > + isSilent = false; > + break; > + } > + } > + > + if (isSilent != m_isSilent) { > + m_isSilent = isSilent; > + updateIsEffectivelyPlayingAudio(); > + } > +} > + > +void AudioDestinationMac::updateIsEffectivelyPlayingAudio() > +{ > + bool isEffectivelyPlayingAudio = isPlaying() && !isSilent(); > + if (isEffectivelyPlayingAudio != m_isEffectivelyPlayingAudio) { > + m_isEffectivelyPlayingAudio = isEffectivelyPlayingAudio; > + m_callback.isPlayingAudioDidChange(); > + } > +} Ditto.
Comment on attachment 241821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241821&action=review > LayoutTests/webaudio/web-audio-is-playing.html:4 > + <script type="text/javascript" src="../resources/js-test-pre.js"></script> nit: Can omit the type as this is the default. > LayoutTests/webaudio/web-audio-is-playing.html:32 > + }, 300); Would it work with a 0 delay? It is really so that we need to wait that long, I am worried that test is going to be flaky.
(In reply to comment #2) Thanks for your feedback, Jer! I'll update my patch.
(In reply to comment #3) > Comment on attachment 241821 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241821&action=review > > > LayoutTests/webaudio/web-audio-is-playing.html:4 > > + <script type="text/javascript" src="../resources/js-test-pre.js"></script> > > nit: Can omit the type as this is the default. OK, will fix. > > > LayoutTests/webaudio/web-audio-is-playing.html:32 > > + }, 300); > > Would it work with a 0 delay? It is really so that we need to wait that > long, I am worried that test is going to be flaky. Yes, I've tested with different delays and values less than this one do not pass consistently. Do you have suggestions on other ways to test this? This test also produces a very short beep.
Comment on attachment 241821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241821&action=review >>> LayoutTests/webaudio/web-audio-is-playing.html:32 >>> + }, 300); >> >> Would it work with a 0 delay? It is really so that we need to wait that long, I am worried that test is going to be flaky. > > Yes, I've tested with different delays and values less than this one do not pass consistently. Do you have suggestions on other ways to test this? This test also produces a very short beep. You should probably use shouldBecomeEqual() from js-test-pre.js. I think it is used exactly for such cases.
Created attachment 241873 [details] Revised patch: incorporated feedback from Jer and Chris.
Committed: http://trac.webkit.org/changeset/176350