Created attachment 203475 [details] [review]
gstvideo: overlays may now have premultiplied alpha

Created attachment 203476 [details] [review]
gstvideo: fix a RGB ordering mixup in colorspace conversion code

Created attachment 203491 [details] [review]
gstvideo: overlays may now have premultiplied alpha

Now allows the user of _get_argb* to request conversion.
Hopefully the bit that caches that (now merged with the scaled list cache) is correct, not sure how to test this without a lot of code...

Comment on attachment 203476 [details] [review]
gstvideo: fix a RGB ordering mixup in colorspace conversion code

> video: fix a RGB ordering mixup in colorspace conversion code

This looks right (not directly related to this bug though, is it?)

Sorry, it could be committed separately, but it seemed to small to open a new bug for it and I was still unaware of the file-local reordering or color components, so I wasn't sure enough it was correct to just push it.

commit ceeff69bc192488ab494f24dd36e0f555ca9efd0
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Dec 14 16:34:39 2011 +0000

    gstvideo: fix a RGB ordering mixup in colorspace conversion code

Comment on attachment 203491 [details] [review]
gstvideo: overlays may now have premultiplied alpha

>Subject: [PATCH] gstvideo: overlays may now have premultiplied alpha

Looks good to me in general, just a few nitpicks below.

There are multiple ways of doing all of this, but I think we should not overcomplicate things and just get on with it and see how it works out. Almost everything here is an implementation detail anyway, so can be improved if needed. Also, in practice it is likely that the overlay producer will not have a choice of what to produce anyway (premultiplied or not), and if it did, we would need to add a query or so anyway (and can then still use this API just fine).

Btw, I think 'video: foobar' is sufficient ;-)


>+#define BLEND10(ret, alpha, v0, v1) \
>+do { \
>+  ret = v0 + (v1 * (255 - alpha)) / 255; \
>+} while(0)

There are also GLib macros for that: G_STMT_START { ... } G_STMT_END

 
>@@ -1310,10 +1343,21 @@ video_blend (GstBlendVideoFormatInfo * dest,
>+ ...
>+  g_return_val_if_fail (dest, FALSE);
>+  g_return_val_if_fail (src, FALSE);

g_return_val_if_fail() is usually only for public API to catch programming mistakes. Internal API should just be g_assert()s really.

 
>diff --git a/gst-libs/gst/video/video-overlay-composition.c b/gst-libs/gst/video/video-overlay-composition.c
>index 3bfbad1..55927db 100644
>@@ -748,7 +752,6 @@ gst_video_overlay_rectangle_new_argb (GstBuffer * pixels,
>   g_return_val_if_fail (stride >= (4 * width), NULL);
>   g_return_val_if_fail (height > 0 && width > 0, NULL);
>   g_return_val_if_fail (render_height > 0 && render_width > 0, NULL);
>-  g_return_val_if_fail (flags == 0, NULL);

FWIW, the intention here was to error out if we get flags we don't understand / don't know how to handle.


>@@ -846,37 +852,60 @@ gst_video_overlay_rectangle_set_render_rectangle (GstVideoOverlayRectangle *
>   /* This assumes we don't need to adjust the format */
>-  if (rectangle->render_width == rectangle->width &&
>-      rectangle->render_height == rectangle->height) {
>+  if (wanted_width == rectangle->width &&
>+      wanted_height == rectangle->height &&
>+      !(rectangle->flags ^ flags) &
>+      GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA) {

Would be nice if the bit ops here could be simplified a little for better readability (and if not, some additional bracketing would be nice).


>@@ -886,8 +915,10 @@ gst_video_overlay_rectangle_get_pixels_argb (GstVideoOverlayRectangle *
>+    if (r->width == wanted_width &&
>+        r->height == wanted_height &&
>+        !(rectangle->flags ^ flags) &
>+        GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA) {

bit ops, see above

>@@ -901,10 +932,21 @@ gst_video_overlay_rectangle_get_pixels_argb (GstVideoOverlayRectangle *
>+  if ((rectangle->flags & flags) &
>+      GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA) {
>+    if (rectangle->flags & GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA) {
>+      gst_video_overlay_rectangle_unpremultiply (&info);
>+    } else {
>+      gst_video_overlay_rectangle_premultiply (&info);
>+    }
>+  }

bit ops again

Created attachment 204029 [details] [review]
video: overlays may now have premultiplied alpha

Thanks. I kept the bitops but placed them in a separate routine named after the semantics, so I guess that's alright ?
One of them had a bug, heh.

Comment on attachment 204029 [details] [review]
video: overlays may now have premultiplied alpha

Looks good to me, thanks for working on this.

Just one minor suggestion:

>+static gboolean
>+gst_video_overlay_rectangle_check_flags (GstVideoOverlayFormatFlags flags)
>+{
>+  /* Check flags only contains flags we know about */
>+  return (flags & ~(GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA)) == 0;
>+}
> ...
>-  g_return_val_if_fail (flags == 0, NULL);
>+  g_return_val_if_fail (gst_video_overlay_rectangle_check_flags (flags), NULL);

Make this either a static inline function or a macro, so we don't get a compiler warning about the static function being unused if people compile gstreamer without the function guards (as common on embedded systems).

>+/**
>+ * gst_video_overlay_rectangle_get_flags:
>..
>+ * Since: 0.10.36
>+ */

More 0.10.37 probably.

>diff --git a/gst-libs/gst/video/video-overlay-composition.h b/gst-libs/gst/video/video-overlay-composition.h
>index 5ff7ce3..11108be 100644
>--- a/gst-libs/gst/video/video-overlay-composition.h
>+++ b/gst-libs/gst/video/video-overlay-composition.h
>@@ -95,13 +95,15 @@ gst_video_overlay_rectangle_unref (GstVideoOverlayRectangle * comp)
> /**
>  * GstVideoOverlayFormatFlags:
>  * @GST_VIDEO_OVERLAY_FORMAT_FLAG_NONE: no flags
>+ * @GST_VIDEO_OVERLAY_FORMAT_FLAG_PREMULTIPLIED_ALPHA: RGB are premultiplied by A/255

Add a (Since 0.10.37) at the end of the description for the new flag (which is not official markup, but the best we can do, gtk-doc doesn't allow since-markers for individual enum values).

Created attachment 204676 [details] [review]
video: overlays may now have premultiplied alpha

commit 5cf87eedc44d9dd252aba4156b2eaf235370f9bf
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Dec 14 14:14:47 2011 +0000

    video: overlays may now have premultiplied alpha
    
    https://bugzilla.gnome.org/show_bug.cgi?id=666177