Upstream GStreamer supports VP8/VP9 with transparency, but some videos are not rendered correctly in WebKit WPE with GStreamer backend, while working fine in Firefox and Chromium. Example of such video with incorrect rendering is: https://cdn.streamelements.com/uploads/187c102b-ac08-4843-8ac9-4e6be93688b9.webm Reproduction with GStreamer's wpesrc (this is what I have and use for testing): LIBGL_ALWAYS_SOFTWARE=true gst-launch-1.0 \ wpesrc do-timestamp=true location=https://nextcloud.mokrynskyi.com/s/qcoTsNgeLqmAXtg ! \ video/x-raw,format=BGRA,width=1280,height=720 ! \ queue ! videoconvert ! autovideosink Many other videos are rendered properly, so there is something special about that particular file. NOTE: GStreamer pipeline that takes that video and does some compositing with it also works fine, so it is WPE-specific issue. Initially reported against gst-plugins-bad: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/issues/1594 Tested with: libwpe 1.9.1 wpebackend-fdo 1.9.1 wpewebkit 2.31.1 GStreamer and plugins from master branch
"Rendered correctly" is not totally accurate. So according to Firefox and Chromium implementation, WebM always produces straight colors. Good thing, since GStreamer only support that also. Though, what I notice is that the compositor in WebKit assumes premultiplied colors, that will looks ok-ish, as loing as the transparent pixels have no color information. From quick look, it seems like a knob missing, but that Straight vs Premultiplied is already something supported over-all.
Yeah, we'll try to investigate this soon, thanks for the report :)
Created attachment 429871 [details] Patch
Created attachment 429901 [details] Patch
Comment on attachment 429901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429901&action=review > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:91 > + // These store the source pixel components. > + unsigned short red; > + unsigned short green; > + unsigned short blue; > + unsigned short alpha; > + // These store the component offset inside the pixel for the destination surface. > + unsigned short redIndex; > + unsigned short greenIndex; > + unsigned short blueIndex; > + unsigned short alphaIndex; These could be explicitly using the uint16_t type. Bonus points for using uint8_t in place of unsigned char (or unsigned char*).
Would it be possible to have a test? reftest? Would be skipped until the bots use gst 1.20.
Created attachment 430019 [details] Patch
(In reply to Miguel Gomez from comment #7) > Created attachment 430019 [details] > Patch Please don't review this yet. As I don't have any idea about which platforms support transparent video rendering and will pass the new test, I'll let the EWSs tell me, and I'll update the patch later skipping the platforms that fail.
Created attachment 430025 [details] Patch
(In reply to Miguel Gomez from comment #9) > Created attachment 430025 [details] > Patch This is the correct patch. The new test is skipped for all the platforms initially. After this is landed, I'll open a new gardening bug for the new test, mentioning that it must be enabled for GTK and WPE once GStreamer is bumped to 1.20.
Comment on attachment 430025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430025&action=review > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:91 > + // These store the source pixel components. > + uint8_t red; > + uint8_t green; > + uint8_t blue; > + uint8_t alpha; > + // These store the component offset inside the pixel for the destination surface. > + uint8_t redIndex; > + uint8_t greenIndex; > + uint8_t blueIndex; > + uint8_t alphaIndex; I did mean uint16_t when I wrote that, since down lower you end up conditionally multiplying RGB values with the alpha channel and then dividing by 255. You're on the safe side if you do this through uint16_ts as you definitely avoid any overflow. I don't know whether otherwise the multiplication automagically propagates the product into a uint16_t before it's divided and narrowed back down to uint8_t.
(In reply to Zan Dobersek from comment #11) > Comment on attachment 430025 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430025&action=review > > > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:91 > > + // These store the source pixel components. > > + uint8_t red; > > + uint8_t green; > > + uint8_t blue; > > + uint8_t alpha; > > + // These store the component offset inside the pixel for the destination surface. > > + uint8_t redIndex; > > + uint8_t greenIndex; > > + uint8_t blueIndex; > > + uint8_t alphaIndex; > > I did mean uint16_t when I wrote that, since down lower you end up > conditionally multiplying RGB values with the alpha channel and then > dividing by 255. You're on the safe side if you do this through uint16_ts as > you definitely avoid any overflow. I don't know whether otherwise the > multiplication automagically propagates the product into a uint16_t before > it's divided and narrowed back down to uint8_t. Yeah, you're right, my fault. But the *Index ones can remain as uint8_t as they are just indexes, no operations are done with them.
Created attachment 430189 [details] Patch
zan@falconsigh.net does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 430189 [details] from commit queue.
Committed r278276 (238313@main): <https://commits.webkit.org/238313@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430189 [details].