[iOS] YouTube playback can unexpectedly interrupt other apps playing audio #13330

pxlcoder · 2023-05-01T17:05:19Z

`b300e9f`

[iOS] YouTube playback can unexpectedly interrupt other apps playing audio
https://bugs.webkit.org/show_bug.cgi?id=256168
rdar://108741963

Reviewed by Jean-Yves Avenard.

Consider the following sequence of events:

1. Begin playback of a video on YouTube.
2. Open another app like Music, and play some audio while the video is playing.
3. Play the YouTube video again, as it will be paused following (2).
4. Play the audio in Music again, as it will be paused following (3).

After (4), the YouTube video does not get paused, and ends up interrupting
the content in Music. Instead, the same behavior as (2) should occur, where
the video gets paused, and the audio from Music is allowed to play uninterrupted.

The issue arises from the existing implemention of audio playback using the
GPU process. With the GPU process, there is now an `AudioSession` in both the
Web and GPU processes. After (2), the system dispatches an
`AVAudioSessionInterruptionNotification` notification, observed in the GPU
process, and used to pause playback. The interruption state is stored on the
`AudioSession` in the GPU process, and is sent to the `AudioSession` in the Web
process (`RemoteAudioSession`). Consequently, the video playback is correctly
paused following (2).

After (3), the video is resumed in the Web process, and the `RemoteAudioSession`
is marked as uninterrupted. However, the `AudioSession` is the GPU process is
not informed that the audio interruption has ended. Consequently, when the
notification is dispatched again after (4), it is ignored, as the GPU process
believes the audio session is already interrupted. This results in a failure
to correctly interrupt the audio session, and media playback in WebKit ends
up interrupting the session in another app.

To fix, ensure the GPU process is notified when interruption ends as a result
of change in the Web process.

* LayoutTests/media/audio-play-with-video-element-interrupted-expected.txt: Added.
* LayoutTests/media/audio-play-with-video-element-interrupted.html: Added.

Added a layout test to exercise the issue. Without this fix, the test times out.

* Source/WebCore/Modules/audiosession/DOMAudioSession.cpp:
(WebCore::DOMAudioSession::audioSessionActiveStateChanged):
(WebCore::DOMAudioSession::activeStateChanged): Deleted.
* Source/WebCore/Modules/audiosession/DOMAudioSession.h:
* Source/WebCore/platform/audio/AudioSession.cpp:
(WebCore::AudioSession::activeStateChanged):
* Source/WebCore/platform/audio/AudioSession.h:

Rename an `InterruptionObserver` method that has the same name as an
`AudioSession` method, so that `RemoteAudioSession` (a derived class of
`AudioSession`) can also be an `InterruptionObserver`.

* Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.cpp:
(WebKit::RemoteAudioSessionProxy::beginInterruption):
(WebKit::RemoteAudioSessionProxy::endInterruption):
(WebKit::RemoteAudioSessionProxy::beginInterruptionRemote):

Call into `RemoteAudioSessionProxyManager`, which is the actual
`InterruptionObserver`.

(WebKit::RemoteAudioSessionProxy::endInterruptionRemote):

Ditto.

* Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.h:
* Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.messages.in:

Add new IPC messages to begin/end interruptions from the Web process.

* Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:
(WebKit::RemoteAudioSessionProxyManager::beginInterruptionRemote):

Temporarily remove `this` as an `InterruptionObserver` to avoid sending a
spurious interruption message to the Web process, since this interruption
was triggered by the Web process.

(WebKit::RemoteAudioSessionProxyManager::endInterruptionRemote):

Ditto.

* Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.h:
* Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.cpp:
(WebKit::RemoteAudioSession::RemoteAudioSession):
(WebKit::RemoteAudioSession::~RemoteAudioSession):
(WebKit::RemoteAudioSession::beginInterruptionRemote):

Temporarily remove `this` as an `InterruptionObserver` to avoid sending a
spurious interruption message to the GPU process, since this interruption
was triggered by the GPU process.

(WebKit::RemoteAudioSession::endInterruptionRemote):

Ditto.

(WebKit::RemoteAudioSession::beginAudioSessionInterruption):

Send IPC to the GPU process to reflect the Web process state.

(WebKit::RemoteAudioSession::endAudioSessionInterruption):
* Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.h:

Make `RemoteAudioSession` an `InterruptionObserver` so that it can send IPC to
the GPU process when the Web process state is changed.

* Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.messages.in:

Rename the IPC method to avoid conflicting the method on the `AudioSession` base
class that drives interruptions. This change is necessary to avoid sending a
spurious IPC to the GPU process when the interrupted state is changed in the Web
process.

Canonical link: https://commits.webkit.org/263810@main

152fd99

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-01T17:10:26Z

EWS run on previous version of this PR (hash fd03cbf)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

AZero13 · 2023-05-01T17:16:43Z

There also seems to be an issue when swiping on the scrubber on YouTube in Safari where playback doesn't happen as expected, or skips. Is this related?

pxlcoder · 2023-05-01T18:05:45Z

There also seems to be an issue when swiping on the scrubber on YouTube in Safari where playback doesn't happen as expected, or skips. Is this related?

Unrelated. I would recommend filing a bug with configuration details and reproduction steps.

jyavenard

r=me with renaming the methods back to what they were earlier

jyavenard · 2023-05-05T22:22:53Z

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.cpp

Remote is redundant, everything here is always remote.
Otherwise you end up with method have Remote twice.

The naming is not redundant, see the ChangeLog:

Rename the IPC method to avoid conflicting the method on the `AudioSession` base class that drives interruptions. This change is necessary to avoid sending a spurious IPC to the GPU process when the interrupted state is changed in the Web process.

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.cpp

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.h

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.messages.in

jyavenard · 2023-05-05T22:25:52Z

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp

that feels a bit messy and difficult to understand at first glance, it deserves some comment

This is also explained in the ChangeLog 😅

But I am happy to leave a comment here too

Added comment.

Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp

webkit-early-warning-system · 2023-05-05T22:44:00Z

EWS run on current version of this PR (hash 152fd99)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

…audio https://bugs.webkit.org/show_bug.cgi?id=256168 rdar://108741963 Reviewed by Jean-Yves Avenard. Consider the following sequence of events: 1. Begin playback of a video on YouTube. 2. Open another app like Music, and play some audio while the video is playing. 3. Play the YouTube video again, as it will be paused following (2). 4. Play the audio in Music again, as it will be paused following (3). After (4), the YouTube video does not get paused, and ends up interrupting the content in Music. Instead, the same behavior as (2) should occur, where the video gets paused, and the audio from Music is allowed to play uninterrupted. The issue arises from the existing implemention of audio playback using the GPU process. With the GPU process, there is now an `AudioSession` in both the Web and GPU processes. After (2), the system dispatches an `AVAudioSessionInterruptionNotification` notification, observed in the GPU process, and used to pause playback. The interruption state is stored on the `AudioSession` in the GPU process, and is sent to the `AudioSession` in the Web process (`RemoteAudioSession`). Consequently, the video playback is correctly paused following (2). After (3), the video is resumed in the Web process, and the `RemoteAudioSession` is marked as uninterrupted. However, the `AudioSession` is the GPU process is not informed that the audio interruption has ended. Consequently, when the notification is dispatched again after (4), it is ignored, as the GPU process believes the audio session is already interrupted. This results in a failure to correctly interrupt the audio session, and media playback in WebKit ends up interrupting the session in another app. To fix, ensure the GPU process is notified when interruption ends as a result of change in the Web process. * LayoutTests/media/audio-play-with-video-element-interrupted-expected.txt: Added. * LayoutTests/media/audio-play-with-video-element-interrupted.html: Added. Added a layout test to exercise the issue. Without this fix, the test times out. * Source/WebCore/Modules/audiosession/DOMAudioSession.cpp: (WebCore::DOMAudioSession::audioSessionActiveStateChanged): (WebCore::DOMAudioSession::activeStateChanged): Deleted. * Source/WebCore/Modules/audiosession/DOMAudioSession.h: * Source/WebCore/platform/audio/AudioSession.cpp: (WebCore::AudioSession::activeStateChanged): * Source/WebCore/platform/audio/AudioSession.h: Rename an `InterruptionObserver` method that has the same name as an `AudioSession` method, so that `RemoteAudioSession` (a derived class of `AudioSession`) can also be an `InterruptionObserver`. * Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.cpp: (WebKit::RemoteAudioSessionProxy::beginInterruption): (WebKit::RemoteAudioSessionProxy::endInterruption): (WebKit::RemoteAudioSessionProxy::beginInterruptionRemote): Call into `RemoteAudioSessionProxyManager`, which is the actual `InterruptionObserver`. (WebKit::RemoteAudioSessionProxy::endInterruptionRemote): Ditto. * Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.h: * Source/WebKit/GPUProcess/media/RemoteAudioSessionProxy.messages.in: Add new IPC messages to begin/end interruptions from the Web process. * Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp: (WebKit::RemoteAudioSessionProxyManager::beginInterruptionRemote): Temporarily remove `this` as an `InterruptionObserver` to avoid sending a spurious interruption message to the Web process, since this interruption was triggered by the Web process. (WebKit::RemoteAudioSessionProxyManager::endInterruptionRemote): Ditto. * Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.h: * Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.cpp: (WebKit::RemoteAudioSession::RemoteAudioSession): (WebKit::RemoteAudioSession::~RemoteAudioSession): (WebKit::RemoteAudioSession::beginInterruptionRemote): Temporarily remove `this` as an `InterruptionObserver` to avoid sending a spurious interruption message to the GPU process, since this interruption was triggered by the GPU process. (WebKit::RemoteAudioSession::endInterruptionRemote): Ditto. (WebKit::RemoteAudioSession::beginAudioSessionInterruption): Send IPC to the GPU process to reflect the Web process state. (WebKit::RemoteAudioSession::endAudioSessionInterruption): * Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.h: Make `RemoteAudioSession` an `InterruptionObserver` so that it can send IPC to the GPU process when the Web process state is changed. * Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.messages.in: Rename the IPC method to avoid conflicting the method on the `AudioSession` base class that drives interruptions. This change is necessary to avoid sending a spurious IPC to the GPU process when the interrupted state is changed in the Web process. Canonical link: https://commits.webkit.org/263810@main

webkit-commit-queue · 2023-05-08T18:44:48Z

Committed 263810@main (b300e9f): https://commits.webkit.org/263810@main

Reviewed commits have been landed. Closing PR #13330 and removing active labels.

pxlcoder requested a review from cdumez as a code owner May 1, 2023 17:05

pxlcoder self-assigned this May 1, 2023

pxlcoder added the Media Bugs related to the HTML 5 Media elements. label May 1, 2023

pxlcoder requested review from eric-carlson, jernoble and youennf May 1, 2023 17:05

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2023

jyavenard approved these changes May 5, 2023

View reviewed changes

pxlcoder removed the merging-blocked Applied to prevent a change from being merged label May 5, 2023

pxlcoder force-pushed the eng/256168 branch from fd03cbf to 152fd99 Compare May 5, 2023 22:43

pxlcoder added the merge-queue Applied to send a pull request to merge-queue label May 8, 2023

webkit-commit-queue force-pushed the eng/256168 branch from 152fd99 to b300e9f Compare May 8, 2023 18:44

webkit-commit-queue merged commit b300e9f into WebKit:main May 8, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 8, 2023

[iOS] YouTube playback can unexpectedly interrupt other apps playing audio #13330

[iOS] YouTube playback can unexpectedly interrupt other apps playing audio #13330

Uh oh!

Conversation

pxlcoder commented May 1, 2023 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

b300e9f

Uh oh!

webkit-early-warning-system commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AZero13 commented May 1, 2023

Uh oh!

pxlcoder commented May 1, 2023

Uh oh!

jyavenard left a comment

Choose a reason for hiding this comment

Uh oh!

jyavenard May 5, 2023

Choose a reason for hiding this comment

Uh oh!

pxlcoder May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyavenard May 5, 2023

Choose a reason for hiding this comment

Uh oh!

pxlcoder May 5, 2023

Choose a reason for hiding this comment

Uh oh!

pxlcoder May 5, 2023

Choose a reason for hiding this comment

Uh oh!

pxlcoder May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

webkit-early-warning-system commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pxlcoder commented May 1, 2023 •

edited by webkit-early-warning-system

Loading

`b300e9f`

webkit-early-warning-system commented May 1, 2023 •

edited

Loading

webkit-early-warning-system commented May 5, 2023 •

edited

Loading