Bug 137464 – Take Web Audio into account for the Page::isPlayingAudio() API

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Ada Chan]

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.

[Ada Chan]

Created attachment 241821 [details]
Patch

[Jer Noble]

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.

[Chris Dumez]

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.

[Ada Chan]

(In reply to comment #2)
Thanks for your feedback, Jer!  I'll update my patch.

[Ada Chan]

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

[Chris Dumez]

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.

[Ada Chan]

Created attachment 241873 [details]
Revised patch: incorporated feedback from Jer and Chris.

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/176350