[Bug 739281] video-blend: correct improper logic calculations

[GStreamer (bugzilla.gnome.org)]

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

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

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

--- Comment #37 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2014-11-05 19:41:47 UTC ---
(From update of attachment 289884)
>Subject: [PATCH] video-blend: correct improper logic calculations

Please update the right hand side of the summary line, it does not describe
what you are fixing here very well (see previous comment) :)

>in gst_video_blend function

The exact function is not so important, and we can easily see that from the
patch.

>The logic to calulate x/y/src_width/src_height based on
>different input values is wrong.
>Hence correcting the logic for the same

It would be better to describe the problem/bug (ie. what happened when one did
what?) and then the fix and what works now that the fix is applied. Most people
who read commit messages are interested in the effect/outcome and want to know
'might this patch be of interest or useful for me?'.

>@@ -263,7 +263,7 @@ gboolean
> gst_video_blend (GstVideoFrame * dest,
>     GstVideoFrame * src, gint x, gint y, gfloat global_alpha)
> {
>-  guint i, j, global_alpha_val, src_width, src_height, dest_width, dest_height;
>+  gint i, j, global_alpha_val, src_width, src_height, dest_width, dest_height;
>   gint xoff;
>   guint8 *tmpdestline = NULL, *tmpsrcline = NULL;
>   gboolean src_premultiplied_alpha, dest_premultiplied_alpha;
>@@ -328,23 +328,32 @@ gst_video_blend (GstVideoFrame * dest,
> 
>   xoff = 0;
> 
>+  /*In case overlay is completely outside the video, dont render */
>+  if (x + src_width < 0 || y + src_height < 0 || x >= dest_width
>+      || y >= dest_height) {
>+    x = 0;
>+    y = 0;
>+    src_width = 0;
>+    src_height = 0;
>+  }

The comment is good (but please add a space after the initial /* ), it
describes exactly what the code's intention is. The comparisons are fine as
well, though shouldn't it be <= 0 for the first two? I would start a new line
after the second comparison like claudiu does in his patch. There are two
things that should be changed here IMHO: the whole block should be moved up as
far as possible. We can check this much much earlier and bail out before we
allocate memory. I would move this after the ensure_debug_category() call. And
then instead of setting these variables to 0 I would just do a goto
nothing_to_do; and the nothing_to_do at the end just prints a GST_LOG message
with the details and returns TRUE.


>   /* adjust src pointers for negative sizes */

While you're at it, I think this comment should be changed to 'src image' and
'negative offsets', which seems to make more sense to me here. Maybe also add
an aditional comment above this with a newline, saying /* If we're here we know
that the overlay image fully or partially overlaps with the video frame */ or
something to that effect.

>-  if (x < 0) {
>+  if (x < 0 && -x < src_width) {

Is the additional check needed after the block above?

>     src_width -= -x;
>-    x = 0;
>     xoff = -x;
>+    x = 0;
>   }

Why move the x = 0 ?


>-  if (y < 0) {
>+  if (y < 0 && -y < src_height) {
>     src_height -= -y;
>     y = 0;
>   }

Is this (existing) block correct? I'm not so sure. The extra condition is again
not needed, is it?

So what seems to happen here, if I read the code correctly, is that if we have
a negative y, that instead of cropping off the top -y lines of the overlay
image we crop off the *bottom* -y lines of the overlay image.

Perhaps we should add a src_yoff for clarity here, and also rename xoff to
src_xoff (or src_crop_left/src_crop_top?)

>   /* adjust width/height if the src is bigger than dest */

--> /* adjust width/height to render if the src image extends
       * beyond the right or bottom border of the video image */

>-  if (x + src_width > dest_width)
>+  if (x < dest_width && (x + src_width) > dest_width)
>     src_width = dest_width - x;

Additional check x < dest_width is superfluous, can't be the case here.

>-  if (y + src_height > dest_height)
>+  if (y < dest_height && (y + src_height) > dest_height)
>     src_height = dest_height - y;

Additional check y < dest_height is superfluous, can't be the case here.

>     dinfo->unpack_func (dinfo, 0, tmpdestline, dest->data, dest->info.stride,
>         0, i, dest_width);
>     sinfo->unpack_func (sinfo, 0, tmpsrcline, src->data, src->info.stride,
>-        xoff, i - y, src_width - xoff);
>+        xoff, i - y, src_width);

Ok. Only one problem: non-0 xoff doesn't work yet, does it?

And we need to also take into account the new src_yoff/src_top_crop I think.

So two observations: I don't think it can actually work properly yet in
practice (has anyone tested the visual outcome with corner cases?), and the
unit test doesn't catch the fact that the code doesn't render the right thing
;)

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