[GStreamer] refactor video decoder limit customization #7381

pgorszkowski-igalia · 2022-12-09T10:24:57Z

`c367583`

[GStreamer] refactor video decoder limit customization
https://bugs.webkit.org/show_bug.cgi?id=248961

Reviewed by Philippe Normand.

Moved the same functionality to one place - from
MediaPlayerPrivateGStreamerMSE::supportsType to
GStreamerRegistryScanner::isContentTypeSupported.

* Source/WebCore/platform/GStreamer.cmake:
* Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:
(videoDecoderLimitsDefaults):
(WebCore::GStreamerRegistryScanner::isContentTypeSupported const):
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::supportsType):
(videoDecoderLimitsDefaults): Deleted.

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

7e64ee9

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 · 2022-12-09T10:25:05Z

EWS run on previous version of this PR (hash 3f4aec5)

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
⏳ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

eocanha

Appart from the comments, the rest of the changes look fine to me.

eocanha · 2022-12-09T10:50:12Z

Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp

+        videoDecodingLimits = videoDecoderLimitsDefaults();
+        if (!videoDecodingLimits) {
+            GST_WARNING("Parsing VIDEO_DECODING_LIMIT failed");
+            ASSERT_NOT_REACHED();


The style checker complained about this. Try adding return SupportsType::IsNotSupported; after this line to ensure that an early return happens in case assertions are disabled at runtime.

Do we really want to return IsNotSupport in case of wrongly set VIDEO_DECODING_LIMIT? The consequence will be that none of the stream will be played.

If the packager has decided to enable VIDEO_DECODING_LIMIT in CMake, then they should take care of properly defining the limit. Getting all the videos failing is a good way of noticing that something is wrong, so it's acceptable to me (although I'm not a reviewer).

eocanha · 2022-12-09T11:04:15Z

Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp

+        : mediaMaxWidth(mediaMaxWidth)
+        , mediaMaxHeight(mediaMaxHeight)
+        , mediaMaxFrameRate(mediaMaxFrameRate)
+        {


I think this should be one tab less, like in this example.

webkit-early-warning-system · 2022-12-13T16:04:13Z

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

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
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

philn

Could these checks be done only when configuration == Configuration::Decoding?

pgorszkowski-igalia · 2023-03-03T08:17:42Z

Could these checks be done only when configuration == Configuration::Decoding?

@philn : Sorry for late response, I will add it ASAP.

webkit-early-warning-system · 2023-04-03T10:14:43Z

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

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

Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp

webkit-early-warning-system · 2023-04-25T14:59:16Z

EWS run on current version of this PR (hash 7e64ee9)

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

pgorszkowski-igalia · 2023-04-26T06:14:20Z

It works with correct value e.g.:
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024x768@25

I tested it also with few incorrect values:
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024x768@
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024x768
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024x
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024
Tools/Scripts/build-webkit --gtk --cmakeargs=-DVIDEO_DECODING_LIMIT=1024@25

In case of incorrect value the limit is not applied - as expected.

For testing I used dashjs reference player:
Tools/Scripts/run-minibrowser --gtk https://reference.dashif.org/dash.js/nightly/samples/dash-if-reference-player/index.html

https://bugs.webkit.org/show_bug.cgi?id=248961 Reviewed by Philippe Normand. Moved the same functionality to one place - from MediaPlayerPrivateGStreamerMSE::supportsType to GStreamerRegistryScanner::isContentTypeSupported. * Source/WebCore/platform/GStreamer.cmake: * Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp: (videoDecoderLimitsDefaults): (WebCore::GStreamerRegistryScanner::isContentTypeSupported const): * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp: (WebCore::MediaPlayerPrivateGStreamerMSE::supportsType): (videoDecoderLimitsDefaults): Deleted. Canonical link: https://commits.webkit.org/263408@main

webkit-commit-queue · 2023-04-26T08:18:04Z

Committed 263408@main (c367583): https://commits.webkit.org/263408@main

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

pgorszkowski-igalia requested a review from philn as a code owner December 9, 2022 10:24

pgorszkowski-igalia self-assigned this Dec 9, 2022

pgorszkowski-igalia added the Media Bugs related to the HTML 5 Media elements. label Dec 9, 2022

pgorszkowski-igalia mentioned this pull request Dec 9, 2022

[GStreamer][MSE][CMake] make configurable video decoder limits #4744

Merged

eocanha requested changes Dec 9, 2022

View reviewed changes

pgorszkowski-igalia force-pushed the eng/GStreamer-refactor-video-decoder-limit-customization branch from 3f4aec5 to b0cc7e3 Compare December 13, 2022 16:04

philn requested changes Feb 20, 2023

View reviewed changes

pgorszkowski-igalia force-pushed the eng/GStreamer-refactor-video-decoder-limit-customization branch from b0cc7e3 to 4477143 Compare April 3, 2023 10:13

pgorszkowski-igalia requested review from philn and eocanha April 12, 2023 10:06

philn reviewed Apr 24, 2023

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp Outdated Show resolved Hide resolved

pgorszkowski-igalia force-pushed the eng/GStreamer-refactor-video-decoder-limit-customization branch from 4477143 to 7e64ee9 Compare April 25, 2023 14:55

philn approved these changes Apr 25, 2023

View reviewed changes

pgorszkowski-igalia added the request-merge-queue Request a pull request to be added to merge-queue once ready label Apr 26, 2023

philn added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Apr 26, 2023

webkit-commit-queue force-pushed the eng/GStreamer-refactor-video-decoder-limit-customization branch from 7e64ee9 to c367583 Compare April 26, 2023 08:18

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

webkit-commit-queue merged commit c367583 into WebKit:main Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GStreamer] refactor video decoder limit customization #7381

[GStreamer] refactor video decoder limit customization #7381

pgorszkowski-igalia commented Dec 9, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Dec 9, 2022 •

edited

eocanha left a comment

eocanha Dec 9, 2022

pgorszkowski-igalia Dec 13, 2022

eocanha Dec 13, 2022

eocanha Dec 9, 2022

webkit-early-warning-system commented Dec 13, 2022 •

edited

philn left a comment

pgorszkowski-igalia commented Mar 3, 2023 •

edited

webkit-early-warning-system commented Apr 3, 2023 •

edited

webkit-early-warning-system commented Apr 25, 2023 •

edited

pgorszkowski-igalia commented Apr 26, 2023

webkit-commit-queue commented Apr 26, 2023

[GStreamer] refactor video decoder limit customization #7381

[GStreamer] refactor video decoder limit customization #7381

Conversation

pgorszkowski-igalia commented Dec 9, 2022 • edited by webkit-early-warning-system

c367583

webkit-early-warning-system commented Dec 9, 2022 • edited

eocanha left a comment

Choose a reason for hiding this comment

eocanha Dec 9, 2022

Choose a reason for hiding this comment

pgorszkowski-igalia Dec 13, 2022

Choose a reason for hiding this comment

eocanha Dec 13, 2022

Choose a reason for hiding this comment

eocanha Dec 9, 2022

Choose a reason for hiding this comment

webkit-early-warning-system commented Dec 13, 2022 • edited

philn left a comment

Choose a reason for hiding this comment

pgorszkowski-igalia commented Mar 3, 2023 • edited

webkit-early-warning-system commented Apr 3, 2023 • edited

webkit-early-warning-system commented Apr 25, 2023 • edited

pgorszkowski-igalia commented Apr 26, 2023

webkit-commit-queue commented Apr 26, 2023

pgorszkowski-igalia commented Dec 9, 2022 •

edited by webkit-early-warning-system

`c367583`

webkit-early-warning-system commented Dec 9, 2022 •

edited

webkit-early-warning-system commented Dec 13, 2022 •

edited

pgorszkowski-igalia commented Mar 3, 2023 •

edited

webkit-early-warning-system commented Apr 3, 2023 •

edited

webkit-early-warning-system commented Apr 25, 2023 •

edited