Skip to content
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

[MMS] Add logging for streaming state changes and streamingAllowed timer #18460

Merged
merged 1 commit into from
Oct 1, 2023
Merged

[MMS] Add logging for streaming state changes and streamingAllowed timer #18460

merged 1 commit into from
Oct 1, 2023

Conversation

jyavenard
Copy link
Member

@jyavenard jyavenard commented Sep 30, 2023 •
edited by webkit-early-warning-system
Loading

9b0f88f

[MMS] Add logging for `streaming` state changes and `streamingAllowed` timer
https://bugs.webkit.org/show_bug.cgi?id=262435
rdar://116158344

Reviewed by Eric Carlson.

Logging change only.

* Source/WebCore/Modules/mediasource/ManagedMediaSource.cpp:
(WebCore::ManagedMediaSource::setStreaming):
(WebCore::ManagedMediaSource::streamingTimerFired):
* Source/WebCore/Modules/mediasource/ManagedMediaSource.h:
* Source/WebCore/Modules/mediasource/MediaSource.h:
(WebCore::MediaSource::streaming const):
* Source/WebCore/Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::appendBufferInternal):

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

4137c2b

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
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@jyavenard jyavenard self-assigned this Sep 30, 2023
@jyavenard jyavenard added the Media Bugs related to the HTML 5 Media elements. label Sep 30, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 30, 2023 •
edited
Loading

EWS run on current version of this PR (hash 4137c2b)

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
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 30, 2023
eric-carlson
eric-carlson approved these changes Sep 30, 2023
View reviewed changes
Source/WebCore/Modules/mediasource/ManagedMediaSource.h
@@ -44,7 +44,7 @@ class ManagedMediaSource final : public MediaSource {

static bool isTypeSupported(ScriptExecutionContext&, const String& type);

bool streaming() const { return m_streaming; }
bool streaming() const override { return m_streaming; }
Copy link
Contributor

Choose a reason for hiding this comment

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

final?

Copy link
Member Author

Choose a reason for hiding this comment

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

The class is already final

Source/WebCore/Modules/mediasource/SourceBuffer.cpp
@@ -503,7 +503,7 @@ ExceptionOr<void> SourceBuffer::appendBufferInternal(const unsigned char* data,
if (isRemoved() || m_updating)
return Exception { InvalidStateError };

DEBUG_LOG(LOGIDENTIFIER, "size = ", size, ", buffered = ", m_private->buffered());
ALWAYS_LOG(LOGIDENTIFIER, "size = ", size, ", buffered = ", m_private->buffered(), " streaming = ", m_source->streaming());
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 going to generate a lot of logging, are you sure you want ALWAYS?

Copy link
Member Author

Choose a reason for hiding this comment

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

The completion of appendBuffer is already always.
so it doesn’t add that much logging: it just now have a match for begin and end.

@jyavenard jyavenard added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Oct 1, 2023
@jyavenard
[MMS] Add logging for streaming state changes and `streamingAllowed…
9b0f88f 
…` timer

https://bugs.webkit.org/show_bug.cgi?id=262435
rdar://116158344

Reviewed by Eric Carlson.

Logging change only.

* Source/WebCore/Modules/mediasource/ManagedMediaSource.cpp:
(WebCore::ManagedMediaSource::setStreaming):
(WebCore::ManagedMediaSource::streamingTimerFired):
* Source/WebCore/Modules/mediasource/ManagedMediaSource.h:
* Source/WebCore/Modules/mediasource/MediaSource.h:
(WebCore::MediaSource::streaming const):
* Source/WebCore/Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::appendBufferInternal):

Canonical link: https://commits.webkit.org/268701@main
@webkit-commit-queue
Copy link
Collaborator

Committed 268701@main (9b0f88f): https://commits.webkit.org/268701@main

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

@webkit-commit-queue webkit-commit-queue merged commit 9b0f88f into WebKit:main Oct 1, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-carlson eric-carlson eric-carlson approved these changes

@jernoble jernoble Awaiting requested review from jernoble

Assignees

@jyavenard jyavenard

Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
Milestone
No milestone
5 participants
@jyavenard @webkit-early-warning-system @webkit-commit-queue @eric-carlson @webkit-ews-buildbot