Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle #2891

heycam · 2022-08-01T04:30:25Z

`033dfee`

Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle
https://bugs.webkit.org/show_bug.cgi?id=243393
rdar://problem/97890582

Reviewed by Eric Carlson.

ActivityState::IsVisuallyIdle on Cocoa platforms uses notifications from the
window server to determine whether there have been no updates to the window
recently. DOM timers are throttled when the ActivityState::IsVisuallyIdle flag
is present. But due to rdar://97530095, certain videos do not cause the window
server to consider the window as being updated, and so we can end up being
IsVisuallyIdle despite the window updating, and throttling timers unexpectedly.

We use the existing viewport visibility registration that <video>
elements do to add a new field to MediaUsageInfo that records whether
the video is currently visible in the viewport. This state is then
consulted in WebPageProxy::updateActivityState when computing the new
value of the ActivityState::IsVisuallyIdle flag.

* Source/WTF/wtf/PlatformHave.h:
* Source/WebCore/html/MediaElementSession.cpp:
(WebCore::MediaElementSession::isVisibleInViewportChanged):
(WebCore::MediaElementSession::updateMediaUsageIfChanged):
* Source/WebCore/platform/graphics/MediaUsageInfo.h:
(WebCore::MediaUsageInfo::operator== const):
(WebCore::MediaUsageInfo::encode const):
(WebCore::MediaUsageInfo::decode):
* Source/WebKit/UIProcess/Media/MediaUsageManager.h:
* Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.h:
* Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.mm:
(WebKit::MediaUsageManagerCocoa::isPlayingVideoInViewport const):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::updateActivityState):

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

cdumez · 2022-08-01T14:58:04Z

Source/WebKit/UIProcess/WebPageProxy.cpp

+    bool isVisuallyIdle = pageClient().isVisuallyIdle();
+#if PLATFORM(COCOA) && !HAVE(CGS_FIX_FOR_RADAR_97530095) && ENABLE(MEDIA_USAGE)
+    if (pageClient().isViewVisible() && m_mediaUsageManager && m_mediaUsageManager->isPlayingVideoInViewport())
+        isVisuallyIdle = false;


There is already logic here:

bool WebPage::isThrottleable() const { bool isActive = m_activityState.containsAny({ ActivityState::IsLoading, ActivityState::IsAudible, ActivityState::IsCapturingMedia, ActivityState::WindowIsActive }); bool isVisuallyIdle = m_activityState.contains(ActivityState::IsVisuallyIdle); return m_isAppNapEnabled && !isActive && isVisuallyIdle; }

To prevent throttling / app napping in certain media cases. Why can't we simply augment isThrottleable() instead of overriding the value of isVisuallyIdle?

VisuallyIdle is used in a couple of other places:

it causes us to drop the WebPageProxy::m_pageIsUserObservableCount token, in WebPageProxy::updateThrottleState

it influences the rendering update frequency, in Page::setIsVisuallyIdleInternal

I think it's cleaner to make VisuallyIdle correct in the face of this issue with videos rather than add a new ActivityState::IsPlayingVideoInViewport flag and to check it in these places too.

Ok, I'm not objecting to the approach. Would be good if this was reviewed by a media engineer though.

eric-carlson · 2022-08-01T15:41:37Z

Source/WebCore/html/MediaElementSession.cpp

@@ -298,6 +298,10 @@ void MediaElementSession::isVisibleInViewportChanged()

    if (m_element.isFullscreen() || m_element.isVisibleInViewport())
        m_elementIsHiddenUntilVisibleInViewport = false;
+
+#if PLATFORM(COCOA) && !HAVE(CGS_FIX_FOR_RADAR_97530095)


Minor nit: doesn't "have fix for Radar ..." imply PLATFORM(COCOA)?

But "don't have fix for Radar ..." doesn't. And we want this block to be skipped on non-Cocoa platforms.

I meant that Radar is Cocoa-specific so you could use PLATFORM(COCOA) as part of the definition of HAVE_CGS_FIX_FOR_RADAR_97530095 now (in PlatformEnableCocoa.h ?), as you will probably do once we have a fix for rdar://97530095.

But it is a minor nit, so feel free to ignore the suggestion.

cdumez · 2022-08-01T21:45:06Z

Source/WebCore/html/MediaElementSession.cpp

@@ -1273,6 +1277,9 @@ void MediaElementSession::updateMediaUsageIfChanged()
        m_element.hasEverNotifiedAboutPlaying(),
        isOutsideOfFullscreen,
        isLargeEnoughForMainContent(MediaSessionMainContentPurpose::MediaControls),
+#if PLATFORM(COCOA) && !HAVE(CGS_FIX_FOR_RADAR_97530095)
+        m_element.isVisibleInViewport()


Does this function get called whenever we scroll? i.e. whenever the element may have entered/exited the viewport?

No it doesn't. The RenderView "is in viewport" tracking machinery, for which we already register for all video elements, only invokes its callback when it switches between in viewport / not in viewport.

eric-carlson

r=me

eric-carlson · 2022-08-02T00:40:46Z

Source/WebCore/html/MediaElementSession.cpp

@@ -298,6 +298,10 @@ void MediaElementSession::isVisibleInViewportChanged()

    if (m_element.isFullscreen() || m_element.isVisibleInViewport())
        m_elementIsHiddenUntilVisibleInViewport = false;
+
+#if PLATFORM(COCOA) && !HAVE(CGS_FIX_FOR_RADAR_97530095)


I meant that Radar is Cocoa-specific so you could use PLATFORM(COCOA) as part of the definition of HAVE_CGS_FIX_FOR_RADAR_97530095 now (in PlatformEnableCocoa.h ?), as you will probably do once we have a fix for rdar://97530095.

But it is a minor nit, so feel free to ignore the suggestion.

…tivityState::IsVisuallyIdle https://bugs.webkit.org/show_bug.cgi?id=243393 rdar://problem/97890582 Reviewed by Eric Carlson. ActivityState::IsVisuallyIdle on Cocoa platforms uses notifications from the window server to determine whether there have been no updates to the window recently. DOM timers are throttled when the ActivityState::IsVisuallyIdle flag is present. But due to rdar://97530095, certain videos do not cause the window server to consider the window as being updated, and so we can end up being IsVisuallyIdle despite the window updating, and throttling timers unexpectedly. We use the existing viewport visibility registration that <video> elements do to add a new field to MediaUsageInfo that records whether the video is currently visible in the viewport. This state is then consulted in WebPageProxy::updateActivityState when computing the new value of the ActivityState::IsVisuallyIdle flag. * Source/WTF/wtf/PlatformHave.h: * Source/WebCore/html/MediaElementSession.cpp: (WebCore::MediaElementSession::isVisibleInViewportChanged): (WebCore::MediaElementSession::updateMediaUsageIfChanged): * Source/WebCore/platform/graphics/MediaUsageInfo.h: (WebCore::MediaUsageInfo::operator== const): (WebCore::MediaUsageInfo::encode const): (WebCore::MediaUsageInfo::decode): * Source/WebKit/UIProcess/Media/MediaUsageManager.h: * Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.h: * Source/WebKit/UIProcess/Media/cocoa/MediaUsageManagerCocoa.mm: (WebKit::MediaUsageManagerCocoa::isPlayingVideoInViewport const): * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::updateActivityState): Canonical link: https://commits.webkit.org/253027@main

webkit-commit-queue · 2022-08-02T07:59:50Z

Committed 253027@main (033dfee): https://commits.webkit.org/253027@main

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

heycam requested a review from cdumez as a code owner August 1, 2022 04:30

heycam self-assigned this Aug 1, 2022

heycam added Media Bugs related to the HTML 5 Media elements. WebKit Nightly Build labels Aug 1, 2022

heycam requested a review from eric-carlson August 1, 2022 04:31

heycam force-pushed the video-activity-state branch from 24ed6ba to 5f48c89 Compare August 1, 2022 04:32

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

heycam removed the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

heycam force-pushed the video-activity-state branch from 5f48c89 to 1cf244a Compare August 1, 2022 04:58

heycam removed the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

heycam force-pushed the video-activity-state branch from 1cf244a to 06e9669 Compare August 1, 2022 06:31

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

heycam removed the merging-blocked Applied to prevent a change from being merged label Aug 1, 2022

heycam force-pushed the video-activity-state branch from 06e9669 to 4cd9d42 Compare August 1, 2022 07:29

cdumez reviewed Aug 1, 2022

View reviewed changes

eric-carlson reviewed Aug 1, 2022

View reviewed changes

heycam requested review from cdumez and eric-carlson August 1, 2022 21:36

cdumez reviewed Aug 1, 2022

View reviewed changes

heycam requested review from cdumez and removed request for cdumez August 1, 2022 21:57

eric-carlson approved these changes Aug 2, 2022

View reviewed changes

heycam added the merge-queue Applied to send a pull request to merge-queue label Aug 2, 2022

webkit-early-warning-system force-pushed the video-activity-state branch from 4cd9d42 to 033dfee Compare August 2, 2022 07:59

webkit-early-warning-system merged commit 033dfee into WebKit:main Aug 2, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle #2891

Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle #2891

heycam commented Aug 1, 2022 •

edited by webkit-commit-queue

cdumez Aug 1, 2022

heycam Aug 1, 2022

cdumez Aug 1, 2022

eric-carlson Aug 1, 2022

heycam Aug 1, 2022

heycam Aug 1, 2022

eric-carlson Aug 2, 2022

cdumez Aug 1, 2022

heycam Aug 1, 2022

eric-carlson left a comment

eric-carlson Aug 2, 2022

webkit-commit-queue commented Aug 2, 2022

Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle #2891

Explicitly check for videos playing in the viewport when computing ActivityState::IsVisuallyIdle #2891

Conversation

heycam commented Aug 1, 2022 • edited by webkit-commit-queue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-carlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-commit-queue commented Aug 2, 2022

heycam commented Aug 1, 2022 •

edited by webkit-commit-queue