Bug 225927 - [Cocoa] Update AudioSession buffer size handling
Summary: [Cocoa] Update AudioSession buffer size handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-18 11:04 PDT by Eric Carlson
Modified: 2021-05-23 15:33 PDT (History)
9 users (show)

See Also:


Attachments
Patch (23.71 KB, patch)
2021-05-18 12:03 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (23.67 KB, patch)
2021-05-18 13:11 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Update patch (27.61 KB, patch)
2021-05-19 14:00 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (25.38 KB, patch)
2021-05-20 15:41 PDT, Eric Carlson
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2021-05-18 11:04:58 PDT
Update AudioSession buffer size handling
Comment 1 Eric Carlson 2021-05-18 11:05:20 PDT
rdar://76920375
Comment 2 Eric Carlson 2021-05-18 12:03:23 PDT
Created attachment 428959 [details]
Patch
Comment 3 Eric Carlson 2021-05-18 13:11:45 PDT
Created attachment 428973 [details]
Patch
Comment 4 Peng Liu 2021-05-18 13:37:16 PDT
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.
Comment 5 Eric Carlson 2021-05-18 13:54:10 PDT
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.
Comment 6 youenn fablet 2021-05-19 07:36:14 PDT
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?
Comment 7 Eric Carlson 2021-05-19 11:45:15 PDT
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.
Comment 8 Eric Carlson 2021-05-19 14:00:18 PDT
Created attachment 429096 [details]
Update patch
Comment 9 youenn fablet 2021-05-20 08:20:00 PDT
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.
Comment 10 Eric Carlson 2021-05-20 15:41:54 PDT
Created attachment 429233 [details]
Patch
Comment 11 EWS 2021-05-21 12:47:23 PDT
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].
Comment 12 Darin Adler 2021-05-23 15:33:19 PDT
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.