Created attachment 289501 [details] [review]
correct improper logic in video-blend

Have an example pipeline to reproduce this improper logic?

Hi Luis,
  This logic would have been executed before the changes made for 738984

  in 738984, if x/y is less than 0, it is set to 0.
  Hence this logic will never be executed now. It is basically dead code after those changes.

  the pipeline i was testing was 
gst-launch-1.0 filesrc location=../wonder.mp4 ! decodebin ! textoverlay text=text deltax=-2000 ! videoconvert ! ximagesink

  but now in basetextoverlay, xpos will be set to 0. 
  If it was not set as 0,

 then as per the logic present in video-blend 

 if (x < 0) {
    src_width -= -x;
    x = 0;
    xoff = -x;
 }
 src_width would have been calculated as a negative value, which is wrong.


Regards,
Vineeth

Textoverlay isn't the only user of video-blend.c, so this code isn't dead since it is a check for other elements passing strange values as well.

Unless anybody thinks otherwise, I am going to reject this patch.

yes textoverlay is not the only user..

but whoever uses video-blend should take care of this.
as any value of x/y < 0 and x/y > image_width/height is invalid and should be reset to 0.


in case any other user of video-blend does not set this.. 
let us say x is being sent as -2000

now as per the calculation src_width will be width of text, which will be around 61 for "text" - 2000, will become 61 - 2000 = -1939 which is an invalid value
hence the calculation is wrong.

another option to fix this will be(in case the users of video-blend does not take care of resetting to 0) 

if(x < 0 && x > dest_width)
 x = 0;
if(y < 0 && y > dest_height)
 y = 0;

***i meant the alternate fix as below

if(x < 0 !! x > dest_width)
 x = 0;
if(y < 0 !! y > dest_height)
 y = 0;

Created attachment 289516 [details] [review]
updated patch

So you are proposing to have every user to check they aren't passing a bad value instead of centralizing it and having video-blend catch it. Even though the problem negative value creates is a segfault in video-blend.

I'm not sure I agree with you. Will think about it a bit more.

actually i updated the patch with the alternate fix,
where video-blend checks for x/y < 0 and x/y > dest_width/height and resets it to 0
which will be a common fix as suggested by you

This should be checked inside that function IMHO, and if there are values outside the frame I would clip (and not render nothing).

I would really like to see a unit test that demonstrate the problem, and against which any fix can be verified. Something small added to -base/tests/check/libs/video.c

@Sebastian 
in case src width is above the dest width then it does get clipped

This scenario is when x pos is outside the frame dest width. In that case there will no text to be displayed. Hence i thought this fix is valid

@Tim
i will try to add a test case tomorrow 
right now i can't test using textoverlay as the fix is already applied to that. Are you aware of any other element which uses video-blend?

assrender also uses video-blend.

grep around and you will see it.

Most elements will use gst_video_blend*() indirectly via the GstVideoOverlayComposition API where they at runtime decide whether to attach the overlay pixels to video buffers and let downstream do the overlaying somehow, or fall back to using gst_video_overlay_composition_blend() to put the subs on top of raw pixel video right there and then.

another segfault found with gdkpixbufoverlay

gst-launch-1.0 -v videotestsrc ! gdkpixbufoverlay location=../icon.png offset_x=10000 ! autovideosink
This will not crash with the patch attached. Since we are resetting the value to 0, if x < 0 || x > dest_width

In case of negative values it behaves wrongly.
src_width is defined as guint.
so even if we pass negative values for x,
after src_width -= -x;
it gets converted to positive value, which in my opinion is wrong.

IMO src_width, src_height, dest_width, dest_height should be declared as gint, and later we should adjust the same based on calculations.



I am trying to write a test case. But since i am new to writing test cases, it might take some time..

Created attachment 289625 [details] [review]
test case to test the patch

I have created a simple test case to check the crash.

When x/y position is given more than dest frame width/height, then it crashes with the present code, and after applying the attached patch it doesnt.

And the same should ideally happen for negative values.
But in video-blend src_width, src_height, dest_width, dest_height are being declared as guint, hence even if we give negative values, it get wrongly converted to positive, which i think is wrong.

Please verify the test case and the patch.

*** Bug 739516 has been marked as a duplicate of this bug. ***

https://bugzilla.gnome.org/show_bug.cgi?id=739516 <- Is an other testcase.

Both Vineeth and my patch (in that bug) resolve the segfault. Vineeth catches
the negative values that will be problematic and sets the variables to 0. I
check for the value about to become negative and I go to fail.

What bothers me though is that neither of the two patches create a video where
the off frame overlay is not blended in, and all what is outputed is the
original. In both cases the frames are just dropped.

Won't it be user's problem to send proper values based on video frame width/height
 Instead of crashing we are just not displaying

As said above, you should just clip the subtitles instead of not displaying them above :)

Hi sebastian

Lets consider the below 2 cases
for both cases video width is 240

1) x pos of overlay is -300
2) x pos of overlay is 300

Right now the solution is in both cases x pos is reset to 0 as per my patch..

If width of overlay is more than video width then it does clip overlay.

Ps: actually my previous post is misleading 
in case of gdkpixbufoverlay it shows the overlay but the xpos is reset to 0
in case of basetextoverlay it is not being shown in these cases. (Probably we hv to reset to position of overlay to 0 even in this case rather than not displaying? )

Let's do this systematically :)

I would like to see the existing test extended into a full set of unit tests for the following situations:

1) for an overlay rectangle that is smaller in width+height than the video surface it is rendered on:
1a) if rendered completely left of video surface (x + overlay_width <= 0), no blending takes place
1b) if rendered completely right of video surface (x >= video_width), no blending takes place
1c) if rendered completely above video surface (y + overlay_height <= 0), no blending takes place
1d) if rendered completely below video surface (y >= video_height), no blending takes place
1e) if rendered partially left of video surface (x < 0 && -x < overlay_width), overlay pixels overlay_width+x to overlay_width are blended.
1f) if rendered partially right of video surface, ...
1g) if rendered partially above video surface ...
1h) if rendered partially below video surface..

2) for an overlay rectangle that is wider than video_width, make sure things still work properly with negative x and x < video_width

3) for an overlay rectangle that is higher than video_height, ...

(sorry!) :)

Ideally as a separate test. Should be less code than it sounds hopefully, just a bit of copy'n'paste. We need to fill up the overlay with appropriate values that allow us to test if the right bits are blittet at the right spot later (where stuff is blitted).

:)
Okey first let me write test cases for all the scenarios mentioned above..

Then probably everything will clear u guess

Created attachment 289864 [details] [review]
blend test cases

Added test cases as mentioned by Tim

for 1 the cases are handled in
/*Overlay width & height smaller than video width & height*/

for 2 and 3, it will handled together in
/*Overlay width & height bigger than video width & height*/ 


I have to start working on video-blend now with respect to the above cases.

Please check if this is how it should be or the expectations of test case are different.

Created attachment 289871 [details] [review]
video-blend update logic calculations

After checking the existing logic and based on Tim's comment on how to render based on different values of x,y,overlay_width and overlay_height

and as per the test cases attached in the other patch, have made changes in video-blend to handle these different cases. Please check if they are valid.

Created attachment 289872 [details] [review]
textoverlay remove silent setting

In case of basetextoverlay, we were setting silent to TRUE in case x/y is beyond the video dimensions.

But this is not exactly correct.
The actual logic is updated in video-blend based on correct calculations.

Created attachment 289876 [details] [review]
video-blend update logic calculations

Created attachment 289884 [details] [review]
video-blend update logic calculations

updating src_width - xpos to src_width as per discussion in IRC

Created attachment 289886 [details] [review]
Added x parameter

Comment on attachment 289864 [details] [review]
blend test cases

Thanks for writing these Vinneth! Definitely goes in the right direction, but is not quite complete yet.

One question: you create overlays of 200x50 pixels, but then in gst_video_overlay_rectangle_new_raw() scale them to target width/height 50x50 - is that on purpose? (It's fine of course, was just wondering if it was intentional)

Currently the tests just make sure that we don't crash with the various parameter combinations given. What's missing in the test is then that we fill the overlay rectangles with actual data (ideally with alpha=1 and not alpha=0), and then later check that blending actually took place, in the way that we expected, by looking at some of the "edge pixels" in the video surface we blend the rectangle onto.

This requires two things: (a) filling the overlay rectangle strategically with values, and (b) using a clean background image for every blend operation so we start with a new slate.

I think perhaps a small helper function that creates the background surface, and the overlay rectangle, and blends it onto the x/y/width/height provided would be useful, then the checks can just call that and afterwards check if the video surface was changed or not changed as expected, and then unmap/free it.

Comment on attachment 289872 [details] [review]
textoverlay remove silent setting

I agree with this.

The first chunk is superfluous because the above CLAMP already makes sure *xpos is not < 0.

The rest reverts commit 1cc311156. I think that should rather be reverted explicitly via 'git revert'.

Thanks for the review Tim

I think i got what you are expecting..
Will start working on the same from tomorrow :)

Created attachment 289889 [details] [review]
Implement x for ARGB related unpack functions

Created attachment 289892 [details] [review]
Minor changes to vineeths' video-blend patch

Created attachment 289971 [details] [review]
blend test cases

Hi Tim

Updated the test cases as suggested by you,
Can you please check it once..


please give suggestions if it can be improved upon :)...

Comment on attachment 289971 [details] [review]
blend test cases

Thanks, that looks very good, almost ready to commit. Just a few more minor niggles, thanks for your patience :)

- in test_overlay_blend_rect() does the video frame need to be unmapped at the very end?

 - typo in commit message: hegiht -> height

 - the commit message describes what the patch does (tests different x/y overlay positions etc.), but does not really make the *intention* of this all explicit, which is that we want to test rendering of overlays that are outside or partially outside of the video boundaries. People can look at the patch and see *what* it does, what's more difficult to figure out is *why* something is done. This applies to commit messages in general btw, "fix logic error" conveys very little information, the reader knows that this commit fixed something, but what exactly it fixes the reader will have to 'reverse-engineer' from the patch (as another example).

- no need to use glong here, just use normal gints (glong is more awkward to printf in debug logs and doesn't really buy you anything since it will likely be same size as int on 32-bit systems anyway)

- capitalise the #define, i.e. VIDEO_WIDTH and VIDEO_HEIGHT (this is just convention, makes it easier to see in code that this is a #define and not a variable name)

-  gst_video_frame_map() needs to be READWRITE, doesn't it?

- in test_overlay_blend() perhaps describe the test case in natural language rather than in equations (I know I did like that too, sorry). That makes it easier for anyone reading the code to figure out if the code does what you want it to do.

I'll look at the actual patch/fix later.

Comment on attachment 289884 [details] [review]
video-blend update logic calculations

>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 ;)

I lost track of all the patches and not sure if I caused this or if it's a real problem but can you guys try to overlay some text so that it exceeds the frame width? 

Like this pipeline:

gst-launch-1.0 videotestsrc num-buffers=100 ! textoverlay text=TEST font-desc='Sans 40' halignment=4 valignment=3 xpos=0 ypos=0 deltax=300 ! x264enc ! mp4mux ! filesink location=out.mp4


I've checked in basetextoverlay, there's no condition left to silence the element when x+width > dest_width

video-blend.c gets called appropriately and src_width is clipped correctly before it enters the blend loop.

Comment on attachment 289889 [details] [review]
Implement x for ARGB related unpack functions

Thanks for the patch, but I'm going to reject this for now, in the context of this bug. I think we can easily correct for this problem in the unpack function inside our blend function, since the source format is always ARGB of some sort.

Please file a separate bug for this issue. When we fix it, it should be fixed for all formats, and we want a unit test that makes sure it actually works properly for all formats :)

I don't want to block this bug on this being implemented. If it happens, that's great of course, but if not we can work around it, so do that for starters.

Comment on attachment 289892 [details] [review]
Minor changes to vineeths' video-blend patch

Thanks for the patch, but I think it should just be incorporated into the original patch before it lands, to minimise the overall changeset and keep it simple. I've also addressed all of these in my review comments, so let's just mark this patch as obsolete to minimise the number of patches "in play".

Thanks for the detailed reviews Tim :)

I won't hv access to system till Monday 
I will incorporate all the changes and submit the patch on Monday..

Created attachment 290305 [details] [review]
blend test cases

Updated the test case and the commit message as per the review comments.

Have started working on the video-blend logic. Will update the patch as soon as possible.

Please review the patch and give your feedback. Thanks

Created attachment 290329 [details] [review]
video-blend update logic calculations

Made changes as per review and added a new src_yoff variable which makes sure the clipping of the text is proper with respect to y pos.

Right now even though src_xoff is being provided, it is not being used in unpack function. This will workout with Lazars' patch for adding x variable in unpack.

I tested corner cases of y position and am able to see that the clipping works as expected. But x position still cant be tested, without the actual implementation in unpack.

Please review the patch and give your inputs on the same.


Is it required to write test cases to check exact clipped overlay?
If so, i can think about how to do the same, but in a separate thread/bug :)...

Well, that patch to make xoff work for only those two formats is not going to land like this, see comment #39, and it doesn't appear work to fix this properly is in progress, so I don't think we should wait for that if we can work around it trivially.

So I've fixed up the cropping on the left, and some other bits, made the test valgrind clean, and added a visual test for all this.

I note that the unit test doesn't actually catch the fact that we're rendering nonsense when the image is partially outside the video on the left hand side.

Oh well, let's just get this in:

commit 7f5aa9af04044373bf54536793c88aab3f7c8516
Author: Vineeth T M <vineeth.tm@samsung.com>
Date:   Mon Nov 10 12:12:42 2014 +0530

    tests: video: add video blend test
    
    Add test to check rendering of overlays of different sizes
    that are completely or partially outside the video surface.
    Once the overlay is blended to the video, verify if the
    position of the blended overlay is as expected, by comparing
    the pixels of the blended video with the expected values.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739281

commit 9032c12e690665a9b1f29ad014073261e7e6e799
Author: Vineeth T M <vineeth.tm@samsung.com>
Date:   Mon Nov 10 16:36:35 2014 +0530

    video-blend: fix blending of rectangles partially or fully outside of the video
    
    In case of overlay being completely or partially outside
    the video frame, the offset calculations are not right,
    which resulted in the overlay not being displayed as
    expected, or crashes due to invalid memory access.
    
    When the overlay rectangle is completely outside,
    we need not render the overlay at all.
    
    For partial display of overlay rectangles, src_yoff
    was not being calculated, hence it was always clipping
    the bottom half of the overlay, By calculating the
    src_yoff, now the overlay is clipped properly.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739281

commit c61604fd9bff3b61870227d4ccd1a17683f4fa1d
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 01:32:55 2014 +0000

    tests: fix leak in video unit test

commit 07f254e4b2a049d02fca97810be5fd95bd39a662
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 01:34:09 2014 +0000

    tests: add visual overlay composition blending test
    
    Shows visual result of blending a logo on top of
    a video surface, esp. when the logo is partially
    outside of the video surface and needs to be
    clipped.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739281

commit 2fae23c3180edc77f08a57ad92f6c929624745cf
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 16:31:45 2014 +0000

    video-blend: fix allocation of temp src line for wide sources
    
    Fix allocation of temporary source line buffers for source
    images that are wider than the video overlay surface.

commit 9396d843d619560d85d6cf0f0e8f7c289c1d79b2
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 16:34:31 2014 +0000

    video-blend: fix clipping of overlay images on the left
    
    Fix clipping of images that are partially left of the video
    surface, they would get clipped on the right side instead of
    the left side, because the video unpack functions currently
    ignore the x offset parameter. Work around that until that
    is implemented.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739281

commit dc3d68d9c2873ccd69fe55f9cb576150e5752f10
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 19:28:54 2014 +0000

    tests: make overlay blending test slightly less boring

commit 5339e4507a430066bb2f497150a802aea3e8ca60
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sun Nov 16 23:26:45 2014 +0000

    video-blend: minor optimisation
    
    Only need to run matrix on those pixels which
    will actually be used.

Created attachment 290832 [details] [review]
textoverlay revert unnecessary patches

reverting the patches

a5e2d1d0869f4dec8cd32bbd417d838f94fda5e9
8d5e8afa8e5f7491d1e198f39ebe7e4a0ba64059

which is not proper, since actual logic is modified in video-blend

I am reopening this issue just to revert the patches in textoverlay

wrong patch ID mentioned in the above msg..

1cc311156cc3908d1d9888fbcda67305fc647337 and 900d0267d511e9553eec44d948d7e33ead7dc903

are the patches reverted.

Please commit the same so that we can close for good. Thanks :)

I have pushed my reverts of those patches (which I had locally already), because I wrote commit messages outlining what's wrong with those patches and you didn't :) ("wrong condition" does not actually add any information or explain anything).

Comment on attachment 290832 [details] [review]
textoverlay revert unnecessary patches

Committed in different form.