[Bug 746147] compositor: Don't convert or aggregate pads that are completely obscured by a higher zorder pad

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=746147

--- Comment #20 from Nirbheek Chauhan <nirbheek.chauhan at gmail.com> ---
Thanks for the review!

(In reply to Mathieu Duponchelle from comment #19)
> 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'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 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.
;)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.