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.

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

[Gstreamer][HLS] Unable to play TED videos #8100

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

philn
Copy link
Member

@philn philn commented Dec 29, 2022

e2afda4

[Gstreamer][HLS] Unable to play TED videos
https://bugs.webkit.org/show_bug.cgi?id=174458

Reviewed by Xabier Rodriguez-Calvar.

Disable HLS support by default in GStreamer ports. The underlying GStreamer hlsdemux element is not
receiving much maintenance and its replacement, hlsdemux2, cannot be used by WebKit due to
sandboxing requirements.

Most websites should now fallback to MSE if HLS is not supported by the User-Agent. This was checked
on TED and Apple developer websites.

HLS support can be re-enabled at runtime by setting the `WEBKIT_GST_ENABLE_HLS_SUPPORT=1`
environment variable.

* Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:
(WebCore::GStreamerRegistryScanner::initializeDecoders):

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

c231120

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

@philn philn self-assigned this Dec 29, 2022
@philn philn added the WebKitGTK Bugs related to the Gtk API layer. label Dec 29, 2022
@philn philn requested a review from calvaris December 29, 2022 13:03
@eocanha
Copy link
Contributor

eocanha commented Dec 29, 2022

In case it helps, this looks good to me (but I'm not a reviewer).

The idea of the environment variable is great, because we might still need native HLS support downstream in the short/mid term.

@@ -416,6 +415,12 @@ void GStreamerRegistryScanner::initializeDecoders(const GStreamerRegistryScanner
{ ElementFactories::Type::Demuxer, "video/quicktime, variant=(string)3gpp", { "video/3gpp"_s }, { } },
{ ElementFactories::Type::Demuxer, "video/x-ms-asf", { }, { } },
};

if (const char* hlsSupport = getenv("WEBKIT_GST_ENABLE_HLS_SUPPORT")) {
if (!strcmp(hlsSupport, "1"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use g_getenv and g_strcmp0 instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't remember about g_getenv, is this some policy? Because we have a mix of getenv and g_getenv currently. About g_strcmp0 well we're already guaranteed here that hlsSupport is not NULL, so I'm not sure there's a point :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think both are safer and more platform compatible, specially if code changes and for some reason strcmp is not protected by the if above.

@philn philn added the merge-queue Applied to send a pull request to merge-queue label Jan 10, 2023
https://bugs.webkit.org/show_bug.cgi?id=174458

Reviewed by Xabier Rodriguez-Calvar.

Disable HLS support by default in GStreamer ports. The underlying GStreamer hlsdemux element is not
receiving much maintenance and its replacement, hlsdemux2, cannot be used by WebKit due to
sandboxing requirements.

Most websites should now fallback to MSE if HLS is not supported by the User-Agent. This was checked
on TED and Apple developer websites.

HLS support can be re-enabled at runtime by setting the `WEBKIT_GST_ENABLE_HLS_SUPPORT=1`
environment variable.

* Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:
(WebCore::GStreamerRegistryScanner::initializeDecoders):

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

Committed 258717@main (e2afda4): https://commits.webkit.org/258717@main

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

@webkit-commit-queue webkit-commit-queue merged commit e2afda4 into WebKit:main Jan 10, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 10, 2023
@philn philn deleted the eng/174458 branch January 10, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKitGTK Bugs related to the Gtk API layer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants