Bug 173097 – [GTK][WPE] Fix alpha premultiplying when using cairo to draw the video frames

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

GStreamer uses non premultiplied alpha on its video frames. But cairo uses premultiplied alpha. So when we want to paint video frames with cairo, we need to perform a manual premultiply.

With the current code, this is performed by VideoSinkGStreamer, in the webkitVideoSinkRequestRender() function, but this is wrong. This performs the premultiply assuming that the video frames are always going to rendered with cairo, but that's not true. When using accelerated compositing, the frame data is uploaded to a texture and rendered with OpenGL, so in that case premultiplying is an error. Also, premultiplying needs to be done as well when using gstreamer-gl, which is not happening now, and that can't be done inside VideoSinkGStreamer as we are using a different sink.

The appropriate thing to do here is to perform the premultiplying inside ImageGStreamerCairo, as there we know for sure that we are going to use cairo for rendering. Also this is used both with and without gstreamer-gl.

Manually performing the premultiplying has a performance penalty, but luckily this just has to be done when the video has an alpha component, which is a rare case. Usually the video will be using BGRx (or xRGB with big endian), which doesn't require the premultiply.

[Miguel Gomez]

Created attachment 312299 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 312299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312299&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:71
> +        surface = adoptRef(cairo_image_surface_create(cairoFormat, width, height));
> +        unsigned char* surfaceData = cairo_image_surface_get_data(surface.get());

Could we allocate the new data ourselves using bmalloc here? In this case we could assign bufferData = surfaceData after the conversion and create the cairo surface the same way for both cases. We would also need to attach a user data callback to the surface to destroy the data.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:94
> +        cairo_surface_mark_dirty(surface.get());

And we wouldn't need this, I guess

[Zan Dobersek]

Comment on attachment 312299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312299&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:71
>> +        unsigned char* surfaceData = cairo_image_surface_get_data(surface.get());
> 
> Could we allocate the new data ourselves using bmalloc here? In this case we could assign bufferData = surfaceData after the conversion and create the cairo surface the same way for both cases. We would also need to attach a user data callback to the surface to destroy the data.

See ImageBufferCario.cpp for that. Don't forget to allocate zeroed-out memory.

[Miguel Gomez]

Created attachment 312413 [details]
Patch

[Build Bot]

Attachment 312413 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:96:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 312413 [details]
Patch

Clearing flags on attachment: 312413

Committed r217976: <http://trac.webkit.org/changeset/217976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.