Bug 225961 – [GStreamer] Incorrect rendering of VP9 with transparency

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Nazar Mokrynskyi]

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

[Nicolas Dufresne]

"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.

[Philippe Normand]

Yeah, we'll try to investigate this soon, thanks for the report :)

[Miguel Gomez]

Created attachment 429871 [details]
Patch

[Miguel Gomez]

Created attachment 429901 [details]
Patch

[Zan Dobersek]

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*).

[Philippe Normand]

Would it be possible to have a test? reftest? Would be skipped until the bots use gst 1.20.

[Miguel Gomez]

Created attachment 430019 [details]
Patch

[Miguel Gomez]

(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.

[Miguel Gomez]

Created attachment 430025 [details]
Patch

[Miguel Gomez]

(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.

[Zan Dobersek]

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.

[Miguel Gomez]

(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.

[Miguel Gomez]

Created attachment 430189 [details]
Patch

[EWS]

[e-mail] 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.

[EWS]

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].