Bug 30308 – [GStreamer] Add direct support for ARGB videos

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sebastian Dröge (slomo)]

Hi,
attached patch adds direct support for ARGB videos, without loosing the alpha channel. Not that ARGB or videos with alpha channel are very common but this at least makes sure that the alpha channel doesn't get lost :)

[Sebastian Dröge (slomo)]

Created attachment 41048 [details]
0002-Add-support-for-ARGB-videos.patch

[Gustavo Noronha (kov)]

Comment on attachment 41048 [details]
0002-Add-support-for-ARGB-videos.patch

> +        m_buffer = gst_buffer_make_writable (m_buffer);

Space.

> +        int x, y;

You should declare these inside the for loops initialization.

> +        unsigned short a;

Can we make this alpha, for better readability? =)

> +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> +                a = data[3];
> +                data[0] = (a > 0) ? MIN ((data[0] * 255 + a / 2) / a, 255) : 0;
> +                data[1] = (a > 0) ? MIN ((data[1] * 255 + a / 2) / a, 255) : 0;
> +                data[2] = (a > 0) ? MIN ((data[2] * 255 + a / 2) / a, 255) : 0;
> +                data += 4;
> +#else
> +                a = data[0];
> +                data[1] = (a > 0) ? MIN ((data[1] * 255 + a / 2) / a, 255) : 0;
> +                data[2] = (a > 0) ? MIN ((data[2] * 255 + a / 2) / a, 255) : 0;
> +                data[3] = (a > 0) ? MIN ((data[3] * 255 + a / 2) / a, 255) : 0;
> +                data += 4;
> +#endif

I was asking krit about this. He pointed me to code in WebCore/platform/graphics/Color.cpp that does this and is apparently faster. We are wondering if that code could be reused?

unsigned premultipliedARGBFromColor(const Color& color)
{
    unsigned pixelColor;

    if (unsigned alpha = color.alpha()) {
        pixelColor = alpha << 24 |
             ((color.red() * alpha  + 254) / 255) << 16 | 
             ((color.green() * alpha  + 254) / 255) << 8 | 
             ((color.blue() * alpha  + 254) / 255);
    } else
         pixelColor = color.rgb();

    return pixelColor;
}

[Sebastian Dröge (slomo)]

(In reply to comment #2)
> (From update of attachment 41048 [details])
> > +        m_buffer = gst_buffer_make_writable (m_buffer);
> 
> Space.

*sigh* I'm so used to those spaces that I don't even notice them anymore, sorry

> > +        int x, y;
> 
> You should declare these inside the for loops initialization.

Ah right, C++ can do that :)

> > +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> > +                a = data[3];
> > +                data[0] = (a > 0) ? MIN ((data[0] * 255 + a / 2) / a, 255) : 0;
> > +                data[1] = (a > 0) ? MIN ((data[1] * 255 + a / 2) / a, 255) : 0;
> > +                data[2] = (a > 0) ? MIN ((data[2] * 255 + a / 2) / a, 255) : 0;
> > +                data += 4;
> > +#else
> > +                a = data[0];
> > +                data[1] = (a > 0) ? MIN ((data[1] * 255 + a / 2) / a, 255) : 0;
> > +                data[2] = (a > 0) ? MIN ((data[2] * 255 + a / 2) / a, 255) : 0;
> > +                data[3] = (a > 0) ? MIN ((data[3] * 255 + a / 2) / a, 255) : 0;
> > +                data += 4;
> > +#endif
> 
> I was asking krit about this. He pointed me to code in
> WebCore/platform/graphics/Color.cpp that does this and is apparently faster. We
> are wondering if that code could be reused?

Well, the above code was wrong anyway (that was doing un-premultiplication but we want premultiplication). But using the code from Color.cpp really can't be faster... you don't want a function call for every pixel and even if it was inlined the byte array had to be converted to a Color and then from a unsigned int to 4 bytes again...

I'll attach a new patch that does it correctly... and in a more efficient fashion (only a single time for every buffer and not from the main thread).

[Sebastian Dröge (slomo)]

Ok, small update after looking into it a bit more:

The problem is, that we have to change the buffer content in the ARGB case. There are two possible solutions for this:

a) do it in GstBaseSink::render(), the timeout callback, or in MediaPlayerPrivate::paint(). Because GstBaseSink::render() doesn't have reference/write-ownership of the buffer content (because of a GStreamer API bug IMHO), everything else after it doesn't have this either. This means, that in the ARGB case there always needs to be a malloc/memcpy of the buffer content which is not nice.
In the normal RGB case this is no problem because we only need to get an additional reference to the buffer and we only need to read from it.

b) Invent a new "webkitvideotransform" element, that would be plugged into the GStreamer pipeline before the video sink, which then does the conversion. This wouldn't have the problem from a) but has a new problem. First of all, the output caps of this element can't be video/x-raw-rgb anymore because in the ARGB case it's simply something different... this would mean that we need video/x-raw-rgb-webkit or something like that. On this the gst_video_format_parse_caps_*() functions don't work anymore and everything becomes a bit inconvenient.
This solution introduces a lot more complexity just for ARGB.


Which one do you prefer? :) I prefer a), we need to touch every bit of the video buffer anyway in the ARGB case and the additional malloc doesn't hurt that much I guess.



Oh and one sidenote: ARGB is *not* supported by any video codec I know. Only things like MNG, animated GIFs, "motion" PNG support it. But Dirac for example supports AYUV, which would be converted to ARGB by GStreamer. So I think it makes sense to support ARGB, even if it's a bit annoying :)

[Sebastian Dröge (slomo)]

Created attachment 41093 [details]
0001-Add-support-for-ARGB-videos.patch

New patch that does this, fixes code style at some places and adds some comments everywhere.
There's also an explanation why I don't use the code from Color.

It's now all done from the streaming thread and not from the main thread, to only do the necessary bits from the main thread and not block it just because of the ARGB conversion.

Note: the premultiplication in Color is not optimal IMO, instead of always rounding upwards it should round to nearest. See also http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n408
My code rounds to nearest too.

[Sebastian Dröge (slomo)]

Created attachment 41094 [details]
0001-Add-support-for-ARGB-videos.patch

And now fix a very stupid bug :)

[Gustavo Noronha (kov)]

Comment on attachment 41094 [details]
0001-Add-support-for-ARGB-videos.patch

 
> +    // Calculate the display width/height from the storage width/height and the pixel aspect ratio
>      displayWidth *= doublePixelAspectRatioNumerator / doublePixelAspectRatioDenominator;
>      displayHeight *= doublePixelAspectRatioDenominator / doublePixelAspectRatioNumerator;
>  
> -    scale = MIN (rect.width () / displayWidth, rect.height () / displayHeight);
> +    // Calculate the largest scale factor that would fill the target surface
> +    scale = MIN(rect.width() / displayWidth, rect.height() / displayHeight);
> +    // And calculate the new display width/height
>      displayWidth *= scale;
>      displayHeight *= scale;
>  
> -    // Calculate gap between border an picture
> +    // Calculate gap between border an picture on every side
>      gapWidth = (rect.width() - displayWidth) / 2.0;
>      gapHeight = (rect.height() - displayHeight) / 2.0;
>  
> -    // paint the rectangle on the context and draw the surface inside.
> +    // paint the rectangle on the context and draw the buffer inside

You may want to make this a full sentence.

> +    // Go to the new origin and center the video frame.
>      cairo_translate(cr, rect.x() + gapWidth, rect.y() + gapHeight);
>      cairo_rectangle(cr, 0, 0, rect.width(), rect.height());
> +    // Scale the video frame according to the pixel aspect ratio.
>      cairo_scale(cr, doublePixelAspectRatioNumerator / doublePixelAspectRatioDenominator,
>                  doublePixelAspectRatioDenominator / doublePixelAspectRatioNumerator);
> +    // Scale the video frame to fill the target surface as good as possible.
>      cairo_scale(cr, scale, scale);
> +    // And paint it.
>      cairo_set_source_surface(cr, src, 0, 0);
>      cairo_fill(cr);
>      cairo_restore(cr);

I like the comments, and style fix, but I think they deserve a separate commit.

>      // Use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations.
>      priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0,
>                                            webkit_video_sink_timeout_func,
>                                            gst_object_ref(sink),
> -                                          (GDestroyNotify)gst_object_unref);
> +                                          (GDestroyNotify) gst_object_unref);

rogue space addition. Fix these, and split the patches, and they look good to go.

[Sebastian Dröge (slomo)]

Created attachment 42093 [details]
0001-Add-support-for-ARGB-videos.patch

[Sebastian Dröge (slomo)]

Created attachment 42094 [details]
0002-Add-some-comments.patch

[WebKit Commit Bot]

Comment on attachment 42093 [details]
0001-Add-support-for-ARGB-videos.patch

Clearing flags on attachment: 42093

Committed r50284: <http://trac.webkit.org/changeset/50284>

[WebKit Commit Bot]

Comment on attachment 42094 [details]
0002-Add-some-comments.patch

Rejecting patch 42094 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
//svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469

[Eric Seidel (no email)]

Until bug 30683 is fixed, svn-apply can't handle diffs which don't start at line 1. :(

[Eric Seidel (no email)]

Which is why the second patch wasn't handled correctly by the commit-queue, because svn-apply didn't know how to translate the patch correctly so that it could set the reviewer.

[Sebastian Dröge (slomo)]

Created attachment 42202 [details]
0001-Add-some-comments.patch

The ARGB patch was apparently applied cleanly, the other one was dropped. Here is it again, rebased to apply

[Sebastian Dröge (slomo)]

Comment on attachment 42093 [details]
0001-Add-support-for-ARGB-videos.patch

This one is committed already

[WebKit Commit Bot]

Comment on attachment 42202 [details]
0001-Add-some-comments.patch

Clearing flags on attachment: 42202

Committed r50343: <http://trac.webkit.org/changeset/50343>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.