[Bug 666177] gstvideo: overlays may now have premultiplied alpha

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=666177
  GStreamer | gst-plugins-base | unspecified

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203491|none                        |reviewed
             status|                            |

--- Comment #7 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2011-12-20 20:45:28 UTC ---
(From update of attachment 203491)
>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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.