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

[GStreamer] Rewrite inner decoder leveraging the GStreamerElementHarness #8971

Merged
merged 1 commit into from
Jan 26, 2023
Merged

[GStreamer] Rewrite inner decoder leveraging the GStreamerElementHarness #8971

merged 1 commit into from
Jan 26, 2023

Conversation

philn
Copy link
Member

@philn philn commented Jan 23, 2023 •
edited by webkit-early-warning-system

fa3503d

[GStreamer] Rewrite inner decoder leveraging the GStreamerElementHarness
https://bugs.webkit.org/show_bug.cgi?id=250934

Reviewed by Xabier Rodriguez-Calvar.

The previous decoder implementation relied on a GStreamer pipeline involving giostreamsrc,
decodebin3 and appsinks. Due to the highly asynchronous behavior of that pipeline the decoder
heavily relied on Locks and Conditions making the code hard to follow and debug.

The new implementation relies on two element harnesses, the first one is in charge of parsing (with
parsebin), the first parsed video output stream is then plugged to a decode harness, directly hooked
to the platform video decoder, so no queue is involved. The decoding no longer involves additional
threads, so we can simplify the code and entirely get rid of InnerDecoder.

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

50b6913

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
✅ 🧪 webkitpy ✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
❌ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🧪 services ✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 🧪 merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@philn philn self-assigned this Jan 23, 2023
@philn philn added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Jan 23, 2023
@philn philn requested a review from calvaris January 23, 2023 14:06
@philn
Copy link
Member Author

philn commented Jan 23, 2023

The decoder diff won't be easy to read... I'd advise to read the final code instead.

@webkit-early-warning-system
Copy link
Collaborator

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

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

calvaris
calvaris requested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@calvaris calvaris left a comment

Choose a reason for hiding this comment

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

Other than the changes I also commented, I think it could be a good idea to change the interface of pushBuffer and pushBufferFull for them to take a GRefPtr&& that gives an explicit view of ownership. Maybe even all push* functions could use a similar interface.

The downside of this is that you have to WTFMove objects and adopt the ones with [transfer full] before passing them down, or we'd be leaking.

About this I don't have a strong opinion so I just leave it here as "food for thought".

Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h Outdated Show resolved Hide resolved
Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp Outdated Show resolved Hide resolved
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 50b6913)

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
✅ 🧪 webkitpy ✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
❌ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🧪 services ✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 🧪 merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@philn philn requested a review from calvaris January 25, 2023 16:58
calvaris
calvaris approved these changes Jan 26, 2023
View reviewed changes
Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp
@@ -1041,6 +1041,19 @@ void fillVideoInfoColorimetryFromColorSpace(GstVideoInfo* info, const PlatformVi
GST_VIDEO_INFO_COLORIMETRY(info).range = GST_VIDEO_COLOR_RANGE_UNKNOWN;
}

void configureVideoDecoderForHarnessing(const GRefPtr<GstElement>& element)
{
auto elementHasProperty = [&](const char* name) -> bool {
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 not the first time I see this, please strongly consider moving this to GStreamerCommon and refactor all appearances. It does not have to be in this PR though.

@philn philn added the merge-queue Applied to send a pull request to merge-queue label Jan 26, 2023
@philn
[GStreamer] Rewrite inner decoder leveraging the GStreamerElementHarness
fa3503d 
https://bugs.webkit.org/show_bug.cgi?id=250934

Reviewed by Xabier Rodriguez-Calvar.

The previous decoder implementation relied on a GStreamer pipeline involving giostreamsrc,
decodebin3 and appsinks. Due to the highly asynchronous behavior of that pipeline the decoder
heavily relied on Locks and Conditions making the code hard to follow and debug.

The new implementation relies on two element harnesses, the first one is in charge of parsing (with
parsebin), the first parsed video output stream is then plugged to a decode harness, directly hooked
to the platform video decoder, so no queue is involved. The decoding no longer involves additional
threads, so we can simplify the code and entirely get rid of InnerDecoder.

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

Committed 259430@main (fa3503d): https://commits.webkit.org/259430@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit fa3503d into WebKit:main Jan 26, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 26, 2023
@philn philn deleted the eng/250934 branch January 26, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calvaris calvaris calvaris approved these changes

Assignees

@philn philn

Labels
Platform Portability improvements and other general platform improvements not driven directly by site bugs.
Projects
None yet
Milestone
No milestone
4 participants
@philn @webkit-early-warning-system @webkit-commit-queue @calvaris