Skip to content

Migrate HTML media/video element logs to efficient version #46881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Migrate HTML media/video element logs to efficient version #46881

wants to merge 1 commit into from
+94 −67

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Jun 18, 2025 •
edited by webkit-early-warning-system
Loading

dfa213d

Migrate HTML media/video element logs to efficient version
https://bugs.webkit.org/show_bug.cgi?id=294648
rdar://153696898

Reviewed by NOBODY (OOPS!).

Migrate HTML media/video element logs to efficient version for log forwarding.

* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::prepareForLoad):
(WebCore::HTMLMediaElement::selectMediaResource):
(WebCore::HTMLMediaElement::setNetworkState):
(WebCore::HTMLMediaElement::setReadyState):
(WebCore::HTMLMediaElement::setPlaybackRate):
(WebCore::HTMLMediaElement::pause):
(WebCore::HTMLMediaElement::pauseInternal):
(WebCore::HTMLMediaElement::setVolume):
(WebCore::HTMLMediaElement::setMutedInternal):
(WebCore::HTMLMediaElement::removeAudioTrack):
(WebCore::HTMLMediaElement::scheduleConfigureTextTracks):
(WebCore::HTMLMediaElement::mediaPlayerDurationChanged):
(WebCore::HTMLMediaElement::mediaPlayerSizeChanged):
(WebCore::HTMLMediaElement::scheduleMediaEngineWasUpdated):
(WebCore::HTMLMediaElement::mediaPlayerCharacteristicChanged):
(WebCore::HTMLMediaElement::setAutoplayEventPlaybackState):
(WebCore::HTMLMediaElement::sceneIdentifierDidChange):
(WebCore::HTMLMediaElement::setShowPosterFlag):
* Source/WebCore/html/HTMLVideoElement.cpp:
(WebCore::HTMLVideoElement::scheduleResizeEvent):
(WebCore::HTMLVideoElement::mediaPlayerFirstVideoFrameAvailable):
* Source/WebCore/platform/LogMessages.in:

dfa213d

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan requested review from cdumez and rniwa as code owners June 18, 2025 02:29
@pvollan pvollan self-assigned this Jun 18, 2025
@pvollan pvollan added the WebKit Process Model Bugs related to WebKit's multi-process architecture label Jun 18, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 18, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style   🛠 ios   🛠 mac ✅ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings   🛠 ios-sim   🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 wpe-cairo
⏳ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
  🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2 ⏳ 🛠 playstation
  🛠 tv ⏳ 🛠 mac-safer-cpp
⏳ 🛠 tv-sim
⏳ 🛠 watch
  🛠 watch-sim

@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from c0510b1 to 8a67724 Compare June 18, 2025 02:33
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 18, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 8a67724)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ❌ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

youennf
youennf reviewed Jun 18, 2025
View reviewed changes
Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility to update LoggerHelper log, logVerbose... implementation to fix the potential inefficiencies?

Source/WebCore/html/HTMLVideoElement.cpp
@@ -319,7 +319,7 @@ bool HTMLVideoElement::shouldDisplayPosterImage() const

void HTMLVideoElement::mediaPlayerFirstVideoFrameAvailable()
{
ALWAYS_LOG(LOGIDENTIFIER, "m_showPoster = ", showPosterFlag());
HTMLVIDEOELEMENT_RELEASE_LOG(HTMLVIDEOELEMENT_MEDIAPLAYERFIRSTVIDEOFRAMEAVAILABLE, showPosterFlag());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rather verbose, is there a way we could make things shorter somehow?
Can we use __FILE__ and __func__?

Also we have twice the HTMLVIDEOELEMENT_, maybe we could have it only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point! I will look closer into this.

Thanks for reviewing!

Source/WebCore/html/HTMLMediaElement.cpp Outdated
@@ -9441,7 +9440,7 @@ void HTMLMediaElement::visibilityAdjustmentStateDidChange()
void HTMLMediaElement::sceneIdentifierDidChange()
{
if (RefPtr page = document().page()) {
ALWAYS_LOG(LOGIDENTIFIER, page->sceneIdentifier());
HTMLMEDIAELEMENT_RELEASE_LOG(HTMLMEDIAELEMENT_SCENEIDENTIFIERDIDCHANGE, page->sceneIdentifier().utf8().data());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is somehow unfortunate to have to call utf8 here?
Ideally, we would do it like ALWAYS_LOG, not sure whether feasible.

Source/WebCore/html/HTMLMediaElement.cpp Outdated
@@ -3194,7 +3193,7 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)

m_tracksAreReady = tracksAreReady;

ALWAYS_LOG(LOGIDENTIFIER, "new state = ", state, ", current state = ", m_readyState);
HTMLMEDIAELEMENT_RELEASE_LOG(HTMLMEDIAELEMENT_SETREADYSTATE, convertEnumerationToString(state).utf8().data(), convertEnumerationToString(m_readyState).utf8().data());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we would do the conversion if we are actually logging.
ALWAYS_LOG / Logger::logAlwaysVerbose will do the conversion only if willLog will return true.
We are losing some efficiency here.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 18, 2025
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from 8a67724 to d9afc86 Compare June 28, 2025 21:41
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ⏳ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios   🧪 mac-wk2 ⏳ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress ⏳ 🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2 ❌ 🛠 playstation
✅ 🛠 tv   🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from d9afc86 to 48cacdb Compare June 28, 2025 22:18
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 48cacdb)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ⏳ 🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ⏳ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress ⏳ 🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from 48cacdb to a2f8b00 Compare June 28, 2025 22:29
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios   🛠 mac ⏳ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings   🛠 ios-sim   🛠 mac-AS-debug ⏳ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ⏳ 🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ⏳ 🛠 gtk
  🛠 vision   🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress ⏳ 🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
  🛠 tv ❌ 🛠 mac-safer-cpp
❌ 🛠 tv-sim
❌ 🛠 watch
  🛠 watch-sim

@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from a2f8b00 to 14ab8c6 Compare June 28, 2025 22:36
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 14ab8c6)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings   🛠 ios-sim ❌ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ❌ 🧪 api-mac ⏳ 🧪 api-wpe
  🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
  🧪 api-ios ❌ 🧪 mac-wk2 ⏳ 🛠 gtk
❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ⏳ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2   🛠 playstation
  🛠 tv ❌ 🛠 mac-safer-cpp
  🛠 tv-sim
  🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from 14ab8c6 to 7dba585 Compare June 28, 2025 22:45
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 7dba585)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios loading 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ⏳ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ⏳ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ⏳ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ❌ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 29, 2025
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jun 29, 2025
@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from 7dba585 to b331d17 Compare June 29, 2025 01:39
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 29, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe   🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac   🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan
Migrate HTML media/video element logs to efficient version
dfa213d 
https://bugs.webkit.org/show_bug.cgi?id=294648
rdar://153696898

Reviewed by NOBODY (OOPS!).

Migrate HTML media/video element logs to efficient version for log forwarding.

* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::prepareForLoad):
(WebCore::HTMLMediaElement::selectMediaResource):
(WebCore::HTMLMediaElement::setNetworkState):
(WebCore::HTMLMediaElement::setReadyState):
(WebCore::HTMLMediaElement::setPlaybackRate):
(WebCore::HTMLMediaElement::pause):
(WebCore::HTMLMediaElement::pauseInternal):
(WebCore::HTMLMediaElement::setVolume):
(WebCore::HTMLMediaElement::setMutedInternal):
(WebCore::HTMLMediaElement::removeAudioTrack):
(WebCore::HTMLMediaElement::scheduleConfigureTextTracks):
(WebCore::HTMLMediaElement::mediaPlayerDurationChanged):
(WebCore::HTMLMediaElement::mediaPlayerSizeChanged):
(WebCore::HTMLMediaElement::scheduleMediaEngineWasUpdated):
(WebCore::HTMLMediaElement::mediaPlayerCharacteristicChanged):
(WebCore::HTMLMediaElement::setAutoplayEventPlaybackState):
(WebCore::HTMLMediaElement::sceneIdentifierDidChange):
(WebCore::HTMLMediaElement::setShowPosterFlag):
* Source/WebCore/html/HTMLVideoElement.cpp:
(WebCore::HTMLVideoElement::scheduleResizeEvent):
(WebCore::HTMLVideoElement::mediaPlayerFirstVideoFrameAvailable):
* Source/WebCore/platform/LogMessages.in:
@pvollan pvollan force-pushed the eng/Migrate-HTML-media-video-element-logs-to-efficient-version branch from b331d17 to dfa213d Compare June 30, 2025 20:38
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on current version of this PR (hash dfa213d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youennf youennf youennf left review comments

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

Assignees

@pvollan pvollan

Labels
merging-blocked Applied to prevent a change from being merged WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pvollan @webkit-early-warning-system @youennf @webkit-ews-buildbot