After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 746147 - compositor: Don't convert or aggregate pads that are completely obscured by a higher zorder pad
compositor: Don't convert or aggregate pads that are completely obscured by a...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-13 12:21 UTC by Nirbheek Chauhan
Modified: 2015-05-01 12:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
compositor: Skip pads that are completely obscured by a higher zorder pad (5.34 KB, patch)
2015-03-13 12:23 UTC, Nirbheek Chauhan
none Details | Review
compositor: Only map the frame from a buffer if it will be used (5.26 KB, patch)
2015-03-13 12:23 UTC, Nirbheek Chauhan
none Details | Review
tests: Add a check for the new compositor pad-is-obscured optimization (6.12 KB, patch)
2015-03-13 12:24 UTC, Nirbheek Chauhan
none Details | Review
compositor: Skip pads that are completely obscured by a higher zorder pad (5.47 KB, patch)
2015-03-17 22:50 UTC, Nirbheek Chauhan
none Details | Review
compositor: Only map the frame from a buffer if it will be used (5.26 KB, patch)
2015-03-17 22:51 UTC, Nirbheek Chauhan
none Details | Review
tests: Add a check for the new compositor pad-is-obscured optimization (9.70 KB, patch)
2015-03-17 22:53 UTC, Nirbheek Chauhan
none Details | Review
Compositor: Skip unnecessary background fill (5.32 KB, patch)
2015-03-27 13:51 UTC, Pablo Anton
needs-work Details | Review
compositor: Skip pads that are completely obscured by a higher zorder pad (8.02 KB, patch)
2015-04-17 22:26 UTC, Nirbheek Chauhan
none Details | Review
compositor: Only map the frame from a buffer if it will be used (4.83 KB, patch)
2015-04-17 22:29 UTC, Nirbheek Chauhan
none Details | Review
tests: Add a check for the new compositor pad-is-obscured optimization (9.73 KB, patch)
2015-04-17 22:30 UTC, Nirbheek Chauhan
none Details | Review
compositor: Skip pads that are completely obscured by a higher zorder pad (7.28 KB, patch)
2015-04-18 09:55 UTC, Nirbheek Chauhan
committed Details | Review
compositor: Use the correct video info for doing frame conversion (3.17 KB, patch)
2015-04-18 09:56 UTC, Nirbheek Chauhan
none Details | Review
compositor: Only map the frame from a buffer if it will be used (3.17 KB, patch)
2015-04-18 09:56 UTC, Nirbheek Chauhan
committed Details | Review
tests: Add a check for the new compositor pad-is-obscured optimization (15.15 KB, patch)
2015-04-18 10:00 UTC, Nirbheek Chauhan
none Details | Review
compositor: Use the correct video info for doing frame conversion (3.18 KB, patch)
2015-04-18 15:06 UTC, Nirbheek Chauhan
committed Details | Review
tests: Add a check for the new compositor pad-is-obscured optimization (9.71 KB, patch)
2015-05-01 10:55 UTC, Nirbheek Chauhan
committed Details | Review

Description Nirbheek Chauhan 2015-03-13 12:21:53 UTC
Very straightforward idea; for each pad, check every higher zorder pad to see if any of them completely obscure this pad, and if they do, set the converted_frame as NULL to skip it. This is particularly useful in the case when you have a "background image" or video on top of which other videos are overlayed, and when you "fullscreen" one of those videos, the background one should just be skipped by the pad. 

Skipping conversion is the major benefit, but avoiding memcpy is also good.

The overall check is O(n^2), but it takes a negligible amount of time to compute unless you have hundreds of pads (in which case there are other, larger, bottlenecks).

Patches to implement this and a test are attached.
Comment 1 Nirbheek Chauhan 2015-03-13 12:23:27 UTC
Created attachment 299306 [details] [review]
compositor: Skip pads that are completely obscured by a higher zorder pad
Comment 2 Nirbheek Chauhan 2015-03-13 12:23:47 UTC
Created attachment 299307 [details] [review]
compositor: Only map the frame from a buffer if it will be used
Comment 3 Nirbheek Chauhan 2015-03-13 12:24:02 UTC
Created attachment 299308 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization
Comment 4 Nirbheek Chauhan 2015-03-17 22:50:14 UTC
Created attachment 299660 [details] [review]
compositor: Skip pads that are completely obscured by a higher zorder pad

Fixed a bug I noticed (wasn't taking into account formats with an alpha channel), and rebased against master.
Comment 5 Nirbheek Chauhan 2015-03-17 22:51:44 UTC
Created attachment 299661 [details] [review]
compositor: Only map the frame from a buffer if it will be used

No changes; just rebased and re-uploading to maintain the order.
Comment 6 Nirbheek Chauhan 2015-03-17 22:53:27 UTC
Created attachment 299662 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization

Overhauled the test, and made it test a whole bunch of scenarios instead of just two.
Comment 7 Nicolas Dufresne (ndufresne) 2015-03-17 23:09:03 UTC
Review of attachment 299660 [details] [review]:

::: gst/compositor/compositor.c
@@ +301,3 @@
+static gboolean
+is_rectangle_obscured (gint x11, gint y11, gint x12, gint y12, gint x21,
+    gint y21, gint x22, gint y22)

The naming is cryptic. Grouping the rectangles in a structure would be much cleaner. I found that outside of the compositor we tend to use rectangle as a point and a size instead of two points. It is not very consistent, fixing it would allow create some utility to eventually implement a proper rectangle based render. See GstVideoRectangle in gstvideosink.h as an example for a structure (we could reuse this one already).
Comment 8 Nirbheek Chauhan 2015-03-17 23:20:14 UTC
(In reply to Nicolas Dufresne (stormer) from comment #7)
> Review of attachment 299660 [details] [review] [review]:
> 
> ::: gst/compositor/compositor.c
> @@ +301,3 @@
> +static gboolean
> +is_rectangle_obscured (gint x11, gint y11, gint x12, gint y12, gint x21,
> +    gint y21, gint x22, gint y22)
> 
> The naming is cryptic. Grouping the rectangles in a structure would be much
> cleaner. I found that outside of the compositor we tend to use rectangle as
> a point and a size instead of two points. It is not very consistent, fixing
> it would allow create some utility to eventually implement a proper
> rectangle based render. See GstVideoRectangle in gstvideosink.h as an
> example for a structure (we could reuse this one already).

I originally wrote that function as (x,y) + (w x h), then tried GstVideoRectangle, but the if statement in those cases was harder to decipher than this one. In addition, all the CLAMP() calls would need to move into the function in that case, making it even uglier, and less likely that the function would be inlined by gcc. Now that I think about it, it might actually make sense to add an inline keyword to the function definition...

It was just personal preference, since the current check is very simple and obvious at a glance, but I can change it if you feel strongly about it. :)
Comment 9 Nicolas Dufresne (ndufresne) 2015-03-17 23:24:33 UTC
Review of attachment 299661 [details] [review]:

This looks good.

::: gst/compositor/compositor.c
@@ +339,3 @@
     width = cpad->width;
   else
+    width = pad->info.width;

I'm not big fan of the accessor macros either, but I would recommend to make sure not using them here is fully concistent. The macro would have been GST_VIDEO_INFO_WIDTH()
Comment 10 Nicolas Dufresne (ndufresne) 2015-03-17 23:30:32 UTC
Review of attachment 299662 [details] [review]:

Test looks good in general, the GstVideoMeta::map() trick is neat.

::: tests/check/elements/compositor.c
@@ +1253,3 @@
+  buffer_mapped = FALSE;
+
+  alpha1 = g_rand_double_range (rand, 0.1, 0.9);

Using unseeded GRand seems like a bad idea. Do you really need random values here ?
Comment 11 Nirbheek Chauhan 2015-03-17 23:34:22 UTC
(In reply to Nicolas Dufresne (stormer) from comment #9)
> Review of attachment 299661 [details] [review] [review]:
> 
> This looks good.
> 
> ::: gst/compositor/compositor.c
> @@ +339,3 @@
>      width = cpad->width;
>    else
> +    width = pad->info.width;
> 
> I'm not big fan of the accessor macros either, but I would recommend to make
> sure not using them here is fully concistent. The macro would have been
> GST_VIDEO_INFO_WIDTH()

I'm tempted to make the other uses not use the macro. :) Will submit an updated patch.

(In reply to Nicolas Dufresne (stormer) from comment #10)
> Review of attachment 299662 [details] [review] [review]:
> 
> Test looks good in general, the GstVideoMeta::map() trick is neat.
> 
> ::: tests/check/elements/compositor.c
> @@ +1253,3 @@
> +  buffer_mapped = FALSE;
> +
> +  alpha1 = g_rand_double_range (rand, 0.1, 0.9);
> 
> Using unseeded GRand seems like a bad idea. Do you really need random values
> here ?

From the GLib documentation:

> g_rand_new()
> 
>Creates a new random number generator initialized with a seed taken either from /dev/urandom (if existing) or from the current time (as a fallback).

So, the seed is probably better than anything we could give it!
Comment 12 Nirbheek Chauhan 2015-03-17 23:36:38 UTC
> (In reply to Nicolas Dufresne (stormer) from comment #10)
> > Review of attachment 299662 [details] [review] [review] [review]:
> > 
> > Test looks good in general, the GstVideoMeta::map() trick is neat.
> > 
> > ::: tests/check/elements/compositor.c
> > @@ +1253,3 @@
> > +  buffer_mapped = FALSE;
> > +
> > +  alpha1 = g_rand_double_range (rand, 0.1, 0.9);
> > 
> > Using unseeded GRand seems like a bad idea. Do you really need random values
> > here ?
> 
> From the GLib documentation:
> 
> > g_rand_new()
> > 
> >Creates a new random number generator initialized with a seed taken either from /dev/urandom (if existing) or from the current time (as a fallback).
> 
> So, the seed is probably better than anything we could give it!

Also, I thought it would be a good idea to test random alpha values just in case one of them causes something stupid sometime in the future. Not really essential.
Comment 13 Nicolas Dufresne (ndufresne) 2015-03-18 00:23:15 UTC
(In reply to Nirbheek Chauhan from comment #11)
> >Creates a new random number generator initialized with a seed taken either from /dev/urandom (if existing) or from the current time (as a fallback).
> 
> So, the seed is probably better than anything we could give it!

Let me rephrase, using random value for a unit test is a bad idea.
Comment 14 Nicolas Dufresne (ndufresne) 2015-03-18 00:26:38 UTC
(In reply to Nirbheek Chauhan from comment #8)
> I originally wrote that function as (x,y) + (w x h), then tried
> GstVideoRectangle, but the if statement in those cases was harder to
> decipher than this one. In addition, all the CLAMP() calls would need to
> move into the function in that case, making it even uglier, and less likely
> that the function would be inlined by gcc. Now that I think about it, it
> might actually make sense to add an inline keyword to the function
> definition...

This is called once per negotiation, so inlining is really unimportant. Two points or a point and a size is up to you of course, the point that you should put them in a structure for readability still holds.

> 
> It was just personal preference, since the current check is very simple and
> obvious at a glance, but I can change it if you feel strongly about it. :)

It's simple and obvious now, but the natural next step is clear, we need a proper rectangle list management, and render from list of rectangle.
Comment 15 Pablo Anton 2015-03-27 13:51:46 UTC
Created attachment 300450 [details] [review]
Compositor: Skip unnecessary background fill

Check into aggregate_frames if any pad convert all outframe, in that case, skip filling background image.
Comment 16 Nirbheek Chauhan 2015-04-17 22:26:19 UTC
Created attachment 301880 [details] [review]
compositor: Skip pads that are completely obscured by a higher zorder pad

This updated patch addresses the issues raised in comment 7 and comment 9.
Comment 17 Nirbheek Chauhan 2015-04-17 22:29:26 UTC
Created attachment 301881 [details] [review]
compositor: Only map the frame from a buffer if it will be used

This patch rebases the old patch, and fixes another bug I saw in the code: it uses pad->info to get the current frame's video info to decide which conversions to do, which is wrong. It should use pad->buffer_vinfo since that's what the current frame's vinfo is.

I didn't make a separate patch for this because it's all the same code.
Comment 18 Nirbheek Chauhan 2015-04-17 22:30:42 UTC
Created attachment 301882 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization

This updated patch addresses the issue raised in comment 10 about using random values in a test. It now tests all alpha values from 0.1 to 0.9.
Comment 19 Mathieu Duponchelle 2015-04-18 09:06:35 UTC
Review of attachment 301880 [details] [review]:

Nice patch, that needed to be done since quite some time. I'll take more time to think about the algorithm, as I think there might be a slightly more elegant solution where you'd maintain a "global opaque rectangle" that you would initialize at aggregate time, it's the weekend though :) I would also like to see some tests for this, we need to think about the form they could take. I'm completely OK with the end goal of the patch though, we need to merge something to that effect ASAP :)

::: gst/compositor/compositor.c
@@ +299,3 @@
+/* Test whether rectangle1 is obscured by rectangle2 */
+static gboolean
+is_rectangle_obscured (GstVideoRectangle rect1, GstVideoRectangle rect2)

That function would be more aptly named is_rectangle_contained, as it makes no alpha checks, just geometry

@@ +350,3 @@
     width = cpad->width;
   else
+    width = GST_VIDEO_INFO_WIDTH (&pad->buffer_vinfo);

I'm confused, you state in the comments above that it is the same as VIDEO_FRAME_WIDTH / HEIGHT, why do you need to change it then?

@@ +355,3 @@
     height = cpad->height;
   else
+    height = GST_VIDEO_INFO_HEIGHT (&pad->buffer_vinfo);

same question here?

@@ -395,2 +422,3 @@
   }
 
+  /* Clamp the x/y coordinates of this frame to the video boundaries to cover

And all this could go in a function that you would indeed name is_frame_obscured :)
Comment 20 Nirbheek Chauhan 2015-04-18 09:36:06 UTC
Thanks for the review!

(In reply to Mathieu Duponchelle from comment #19)
> Review of attachment 301880 [details] [review] [review]:
> 
> Nice patch, that needed to be done since quite some time. I'll take more
> time to think about the algorithm, as I think there might be a slightly more
> elegant solution where you'd maintain a "global opaque rectangle" that you
> would initialize at aggregate time, it's the weekend though :)

I'm not sure what you mean by "global opaque rectangle", since the "opaque rectangle" is different for every pad because it depends on other (higher z-order) pads.

Initializing and updating it at aggregate time would be too late since that's done after pad conversion, and most of the gains from this patch are at conversion time.

> I would also
> like to see some tests for this, we need to think about the form they could
> take.

attachment 301882 [details] [review] contains a test for this.

> I'm completely OK with the end goal of the patch though, we need to
> merge something to that effect ASAP :)
> 
> ::: gst/compositor/compositor.c
> @@ +299,3 @@
> +/* Test whether rectangle1 is obscured by rectangle2 */
> +static gboolean
> +is_rectangle_obscured (GstVideoRectangle rect1, GstVideoRectangle rect2)
> 
> That function would be more aptly named is_rectangle_contained, as it makes
> no alpha checks, just geometry
> 

Sure.

> @@ +350,3 @@
>      width = cpad->width;
>    else
> +    width = GST_VIDEO_INFO_WIDTH (&pad->buffer_vinfo);
> 
> I'm confused, you state in the comments above that it is the same as
> VIDEO_FRAME_WIDTH / HEIGHT, why do you need to change it then?
> 
> @@ +355,3 @@
>      height = cpad->height;
>    else
> +    height = GST_VIDEO_INFO_HEIGHT (&pad->buffer_vinfo);
> 
> same question here?
> 

I do these because in the next patch, I move the gst_video_frame_map() down the function so that it's only called if the buffer will actually be used. I think I accidentally put these changes here while rebasing my patch. Will fix. :)

> @@ -395,2 +422,3 @@
>    }
>  
> +  /* Clamp the x/y coordinates of this frame to the video boundaries to
> cover
> 
> And all this could go in a function that you would indeed name
> is_frame_obscured :)

Not the best idea, since clamping requires information about the visible video region. Doing this in "is_rectangle_obscured" would mean calling it "is_rectangle_obscured_and_visible" and passing the visible video w/h as well. ;)
Comment 21 Nirbheek Chauhan 2015-04-18 09:55:16 UTC
Created attachment 301891 [details] [review]
compositor: Skip pads that are completely obscured by a higher zorder pad

This updated patch addresses the issues raised in comment 19.
Comment 22 Nirbheek Chauhan 2015-04-18 09:56:24 UTC
Created attachment 301892 [details] [review]
compositor: Use the correct video info for doing frame conversion

I split this out from  attachment 301880 [details] [review] and attachment 301881 [details] [review] since it deserves its own commit.
Comment 23 Nirbheek Chauhan 2015-04-18 09:56:57 UTC
Created attachment 301893 [details] [review]
compositor: Only map the frame from a buffer if it will be used

Rebased and split patch.
Comment 24 Nirbheek Chauhan 2015-04-18 10:00:18 UTC
Created attachment 301894 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization

Updated the patch by removing a uselessly verbose logging line.
Comment 25 Nirbheek Chauhan 2015-04-18 15:06:53 UTC
Created attachment 301902 [details] [review]
compositor: Use the correct video info for doing frame conversion

Missed an instance of using pad->info incorrectly; fixed now.
Comment 26 Nirbheek Chauhan 2015-05-01 10:55:48 UTC
Created attachment 302718 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization

Attached the wrong patch! This is the correct patch for the pad-is-obscured test.
Comment 27 Tim-Philipp Müller 2015-05-01 11:50:06 UTC
Comment on attachment 301891 [details] [review]
compositor: Skip pads that are completely obscured by a higher zorder pad

Pushed this, but split out the 'clean-up' changes that just replace info.foo use with accessor macros.

I'm not sure if it's not possible that the pad has been removed by the time we get to "g_list_find (GST_ELEMENT (vagg)->sinkpads", so a check here might be in order.
Comment 28 Tim-Philipp Müller 2015-05-01 11:52:00 UTC
Comment on attachment 301902 [details] [review]
compositor: Use the correct video info for doing frame conversion

The description of this patch is a bit misleading: the frame info is the same as pad->buffer_vinfo, since the frame has been mapped with pad->buffer_vinfo. There's no sign of pad->info here anywhere. What these changes are really for is preparation for the next patch that moves the mapping of the frame downwards and only maps it if needed. So I've squashed this patch with the next patch.
Comment 29 Tim-Philipp Müller 2015-05-01 11:52:33 UTC
Comment on attachment 302718 [details] [review]
tests: Add a check for the new compositor pad-is-obscured optimization

Not 100% valgrind clean, but that looks like an issue in videoconvert/base.
Comment 30 Tim-Philipp Müller 2015-05-01 12:07:10 UTC
Comment on attachment 300450 [details] [review]
Compositor: Skip unnecessary background fill

Thanks for the patch!

I will clone this bug and open a new bug for the background fill optimisation.

It looks like you might be able to re-use the just-introduced is_rectangle_contained() function in your patch, and also I wonder if we could do this as part of the previous loop or if that would be too messy.