GNOME Bugzilla – Bug 610249
[xoverlay] add set_render_rectangle() methods
Last modified: 2010-03-28 20:22:39 UTC
* image positions: xoverlay interface currently fills the associated xwindow to 100%. This is problematic as ui toolkits switch to windowless mode. In gtk with can still force the xwindow, but for qt graphicsview there is no such api. On the other hand the X api XShmPutImage/XPutImage/XvShmPutImage/XvPutImage allows to specify the target rectangle within the associated window. In x{v}imagesinks _put() implementation we set (x,y,w,h)=(0,0,image.w,image.h). I am proposing to add a vmethod to xoverlay interface to specify the target rectangle. * resize vmethod: One can turn of event handling on ximagesink/xvimage sink. This only works as both poll the window size for every frame. Unfortunately the polling is a x roundtrip and this not for free (see bug #610248). I am proposing to add a vmethod to xoverlay interface similar to expose, called resize.
Created attachment 154030 [details] [review] proposed interface changes
Created attachment 154042 [details] [review] proposed interface additions
Looks good. resize method should also help for Qt use cases as there are some situations when Qt recreates X window (resizing from windowed to full screen, changing window attribute, reparenting a window). This change will require some coordinated re-configuration of the x overlay.
Review of attachment 154042 [details] [review]: Bunch of nitpicks: - the commit message could be improved: - the summary should say what vmethods you are adding - please write 'first' and 'second' if you don't just enumerate or list. - more about the *why* would be good, I think - 'windowless toolkits' - an example would be nice ::: gst-libs/gst/interfaces/Makefile.am @@ +91,3 @@ -I$(top_builddir)/gst-libs \ --add-include-path=`$(PKG_CONFIG) --variable=libdir gstreamer-0.10`/gst \ + --add-include-path=$(builddir)/../video \ really builddir? @@ +113,2 @@ %.typelib: %.gir $(INTROSPECTION_COMPILER) + $(INTROSPECTION_COMPILER) --includedir=$(srcdir) --includedir=$(builddir) --includedir=`$(PKG_CONFIG) --variable=libdir gstreamer-0.10`/gst --includedir=$(builddir)/../video $(INTROSPECTION_COMPILER_OPTS) $< -o $(@F) here too: really builddir? ::: gst-libs/gst/interfaces/xoverlay.c @@ +465,3 @@ + * in the drawable even if the pipeline is PAUSED. It is needed if the sink is + * not able to detect resizes. + */ - in what cases would the sink not be able to detect resizes for example? - Since: markers @@ +467,3 @@ + */ +void +gst_x_overlay_resize (GstXOverlay * overlay) Wonder if there's a better name for this that makes it clearer that the function is to notify the sink of the resize after the fact and not to be used to resize/set the size. @@ +482,3 @@ + +/** + * gst_x_overlay_set_render_rect: Can we make this _set_render_rectangle()? @@ +487,3 @@ + * + * Configure a subregion as a video target within the window set by + * gst_x_overlay_set_xwindow_id(). I'm not really sure what this means (configure as a target) - would be nice to flesh this out a little bit more :) ::: gst-libs/gst/interfaces/xoverlay.h @@ +76,3 @@ + + void (* resize) (GstXOverlay *overlay); + Same comments re. naming as for the methods. @@ +79,2 @@ /*< private >*/ + gpointer _gst_reserved[GST_PADDING - 3]; I think we can get rid of the padding block, since this is an interface, no?
Created attachment 154209 [details] [review] proposed interface addition * improve the commit message * ::: gst-libs/gst/interfaces/Makefile.am - yes builddir, its including the built gir files - also this is copied from the other makefiles * gst_x_overlay_resize - removed, we don't need it * Can we make this _set_render_rectangle() - yes, thats better indeed (renamed in the patch) - also the api docs are more detailed * gpointer _gst_reserved[GST_PADDING - 3]; - the other interfaces do that as well - if we really don't need that, I'd prefer to clean that up as a separate patch I forgot the since-markers. Will add them if the patch is otherwise okay.
Created attachment 154210 [details] [review] implement new interface in xvimagesink
Created attachment 154211 [details] [review] example for new interface method
Patches for xvimagesink are applied on top of the one in bug #610248.
(In reply to comment #5) > * gpointer _gst_reserved[GST_PADDING - 3]; > - the other interfaces do that as well > - if we really don't need that, I'd prefer to clean that up as a separate > patch Interfaces don't need padding but a separate patch to fix them all is a good idea. Not that it matters much... ;)
Created attachment 154500 [details] [review] proposed interface addition Add since: tag in doc blob and add API keyword in commit message.
This was apparently committed already... but adds a circular dependency between the interfaces and video library. This *must* be fixed before 0.10.29, otherwise the g-i build at least is broken. GstVideoRectangle should either be moved into the interfaces library or a different struct should be used.
Created attachment 156439 [details] [review] use GstXOverlayRectangle struct instead of GstVideoRectangle How about something like this? Alternatively, we could typedef the struct in both headers guarded by #ifndef _OTHER_HEADER_INCLUDED_, but that seems even uglier..
Hehe, rectangles ftw! What's the problem with making gstvideo depend on gstinterfaces (or vice versa)? people have both installed all the time anyway. So what would be the problem in doing that?
(In reply to comment #12) > Created an attachment (id=156439) [details] [review] > use GstXOverlayRectangle struct instead of GstVideoRectangle > > How about something like this? Alternatively, we could typedef the struct in > both headers guarded by #ifndef _OTHER_HEADER_INCLUDED_, but that seems even > uglier.. I'd prefer to have GstVideoRectangle in xoverlay.h and let videosink depend on it. But apart from that you forgot to add the actual struct definition ;)
Sebastian, should the interface Makefile.am just a -I directive to take the local header?
A small note to precise that there is no circular dependency between interfaces and video, the patch introduces a interfaces -> video dep (interfaces depends on video through GstVideoRectangle) but video -> interfaces is not true. The last mail on the mailing list mentioned an issue when generating introspection data for libgstaudio, that's because there's now the dep. chain audio -> interfaces -> video, thus the need to add $(builddir)/../video to the include path for g-ir-scanner/g-ir-compiler for libgstaudio. I've attached the patch that makes it work here (MeeGo/OBS) with the 0.10.28 tarbal, that patch won't apply to master but is enough to make everything a bit clearer. I don't think it's a good idea to have that audio -> ... -> video dep chain for the introspection data and I guess everyone will agree on that so I won't bother providing an updated patch for master, it was just for the small precision :p
Created attachment 156528 [details] [review] Quick patch to make introspection data generation work with 0.10.28
Created attachment 156957 [details] [review] xoverlay: change new set_render_rectangle() vfunc to take four arguments so we don't depend on libgstvideo New / alternative patch: pass four arguments to _set_render_rectangle() instead of a struct. (Maybe we should then rename the method as well? _set_render_region? _render_area? I guess it's still a rectangle in the end..)
I never got an answer what the problem with the include is. Honestly I don't like the "pass -1 for x,y,w,h to unset" part. Also if x,y is used, we could use w,h instead of width and height.
> I never got an answer what the problem with the include is. There are two problems here: - a build dependency issue: this can be fixed by building the video subdir before the interfaces subdir and adding the video includes to the audio and interfaces gir generation rules - a conceptual issue: the include results in libgstinterfaces conceptually depending on libgstvideo, so now libgstaudio (which depends on libgstinterfaces) ends up indirectly depending on libgstvideo, which is undesirable, to say the least. > Honestly I don't like the "pass -1 for x,y,w,h to unset" part. Well no, me neither. 0/0/0/0 would work for me too. I don't really think it matters much. If you have a better suggestion... > Also if x,y is used, we could use w,h instead of width and height. Yes we could, but why garble argument names for no good reason? (The original patch should've used 'rectangle' instead of 'rect' as well IMHO, but it didn't seem worth bringing it up ;)).
Ahh, the whole cyclic dependency has nothing to do with the c-level. It is just because of gir, right? If that is right, lets get rid of the GstVideoRectangle usage. I still don't like it, whats the point in having those if we cannot use it :/ regarding the x,y,w,h - thats what GstVideoRectangle uses too. So if you prefer "width" and "hight", "x" and "y" should be "left" and "top". Regarding unsetting, lets keep it -1/-1/-1/-1 then. Dunno if we might want to use 0/0/0/0 to "hide" the overlay in the future.
> Regarding unsetting, lets keep it -1/-1/-1/-1 then. Dunno if we might want to > use 0/0/0/0 to "hide" the overlay in the future. Originally I thought one might want to have the possibility to use width/height = -1 for 'as much as available' (in which case 0/0/-1/-1 would be the natural way to reset), but I guess that doesn't really make sense for the use case of this API, since the caller knows the width/height of the display. Anyway, let's leave it at all -1/invalid to reset for now. commit 37d000d175dad226f5eacbefe557f107cb43e6a6 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Mar 24 09:59:42 2010 +0000 xoverlay: change new set_render_rectangle() vfunc to take four arguments so we don't depend on libgstvideo Don't make libgstinterfaces (and thus libgstaudio etc.) indirectly depend on libgstvideo by using the GstVideoRectangle helper structure in the API, which causes undesirable dependencies, esp. with the gobject-introspection (people will point and laugh at us if they find out that libgstaudio depends on libgstvideo). Instead, pass the x, y, width and height parameters directly to the function. Re-fixes #610249.