Bug 225927 – [Cocoa] Update AudioSession buffer size handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Update AudioSession buffer size handling

[Eric Carlson]

rdar://76920375

[Eric Carlson]

Created attachment 428959 [details]
Patch

[Eric Carlson]

Created attachment 428973 [details]
Patch

[Peng Liu]

Comment on attachment 428973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428973&action=review

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:96
> +                ++webAudioCount;

I am thinking whether we need to change
```
PlatformMediaSession::MediaType mediaType() const final { return isSuspended() ? PlatformMediaSession::MediaType::None : PlatformMediaSession::MediaType::WebAudio; }
```
to
```
PlatformMediaSession::MediaType mediaType() const final { return isStopped() ? PlatformMediaSession::MediaType::None : PlatformMediaSession::MediaType::WebAudio; }
```

If we make this change, then we don't need to change `MediaSessionManagerCocoa::updateSessionState()` here.

[Eric Carlson]

Comment on attachment 428973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428973&action=review

>> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:96
>> +                ++webAudioCount;
> 
> I am thinking whether we need to change
> ```
> PlatformMediaSession::MediaType mediaType() const final { return isSuspended() ? PlatformMediaSession::MediaType::None : PlatformMediaSession::MediaType::WebAudio; }
> ```
> to
> ```
> PlatformMediaSession::MediaType mediaType() const final { return isStopped() ? PlatformMediaSession::MediaType::None : PlatformMediaSession::MediaType::WebAudio; }
> ```
> 
> If we make this change, then we don't need to change `MediaSessionManagerCocoa::updateSessionState()` here.

I don't think that is sufficient because `AudioContext.isStopped()` returns true only when the context is stopped, and we should not set the buffer size to the render quantum size when an AudioContext is reachable but not currently playing.

It also isn't sufficient because we want to reset the buffer size to the "low power" size when nothing at all is playing.

[youenn fablet]

Comment on attachment 428973 [details]
Patch

Some API test failures seem related.

View in context: https://bugs.webkit.org/attachment.cgi?id=428973&action=review

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:129
> +    if (bufferSize)

There will be cases where bufferSize will be 0, for instance when lowPowerVideoAudioBufferSizeEnabled and there are some sessions.
Is it what we want?

[Eric Carlson]

Comment on attachment 428973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428973&action=review

>> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:129
>> +    if (bufferSize)
> 
> There will be cases where bufferSize will be 0, for instance when lowPowerVideoAudioBufferSizeEnabled and there are some sessions.
> Is it what we want?

I'll update the patch to use the default size in this case.

[Eric Carlson]

Created attachment 429096 [details]
Update patch

[youenn fablet]

Comment on attachment 429096 [details]
Update patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429096&action=review

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:97
> +                ++webAudioCount;

It seems some api tests are breaking, maybe due to that change?
Worth checking whether we want to update the tests or keep status quo.

[Eric Carlson]

Created attachment 429233 [details]
Patch

[EWS]

Committed r277876 (238014@main): <https://commits.webkit.org/238014@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429233 [details].

[Darin Adler]

Comment on attachment 429233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429233&action=review

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:202
> +        m_supportedAudioHardwareBufferSizes = m_audioHardwareListener->supportedBufferSizes();

Kind of wish this and the one line in audioOutputDeviceChanged was the same.

> Source/WebCore/testing/Internals.cpp:5447
> +#if USE(AUDIO_SESSION)
> +    return AudioSession::sharedSession().bufferSize();
> +#endif
> +    return 0;

Please use #else here instead of two returns in a row.