GNOME Bugzilla – Bug 746147
compositor: Don't convert or aggregate pads that are completely obscured by a higher zorder pad
Last modified: 2015-05-01 12:07:36 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.
Created attachment 299306 [details] [review] compositor: Skip pads that are completely obscured by a higher zorder pad
Created attachment 299307 [details] [review] compositor: Only map the frame from a buffer if it will be used
Created attachment 299308 [details] [review] tests: Add a check for the new compositor pad-is-obscured optimization
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.
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.
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.
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).
(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. :)
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()
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 ?
(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!
> (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.
(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.
(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.
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.
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.
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.
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.
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 :)
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. ;)
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.
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.
Created attachment 301893 [details] [review] compositor: Only map the frame from a buffer if it will be used Rebased and split patch.
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.
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.
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 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 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 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 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.