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 610249 - [xoverlay] add set_render_rectangle() methods
[xoverlay] add set_render_rectangle() methods
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal blocker
: 0.10.29
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-02-17 11:37 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2010-03-28 20:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed interface changes (6.09 KB, patch)
2010-02-17 13:01 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
proposed interface additions (6.11 KB, patch)
2010-02-17 15:05 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
reviewed Details | Review
proposed interface addition (6.61 KB, patch)
2010-02-19 14:33 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
implement new interface in xvimagesink (11.40 KB, patch)
2010-02-19 14:33 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
example for new interface method (8.81 KB, patch)
2010-02-19 14:35 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
proposed interface addition (6.67 KB, patch)
2010-02-23 15:04 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
use GstXOverlayRectangle struct instead of GstVideoRectangle (3.01 KB, patch)
2010-03-18 09:48 UTC, Tim-Philipp Müller
none Details | Review
Quick patch to make introspection data generation work with 0.10.28 (3.86 KB, patch)
2010-03-19 03:09 UTC, Damien Lespiau
none Details | Review
xoverlay: change new set_render_rectangle() vfunc to take four arguments so we don't depend on libgstvideo (7.92 KB, patch)
2010-03-24 10:04 UTC, Tim-Philipp Müller
none Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-17 11:37:43 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.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-17 13:01:21 UTC
Created attachment 154030 [details] [review]
proposed interface changes
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-17 15:05:06 UTC
Created attachment 154042 [details] [review]
proposed interface additions
Comment 3 Alexander Bokovoy 2010-02-17 19:40:38 UTC
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.
Comment 4 Tim-Philipp Müller 2010-02-18 11:02:27 UTC
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?
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-19 14:33:19 UTC
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.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-19 14:33:51 UTC
Created attachment 154210 [details] [review]
implement new interface in xvimagesink
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-19 14:35:40 UTC
Created attachment 154211 [details] [review]
example for new interface method
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-19 14:38:43 UTC
Patches for xvimagesink are applied on top of the one in bug #610248.
Comment 9 Sebastian Dröge (slomo) 2010-02-19 14:41:15 UTC
(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... ;)
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2010-02-23 15:04:30 UTC
Created attachment 154500 [details] [review]
proposed interface addition

Add since: tag in doc blob and add API keyword in commit message.
Comment 11 Sebastian Dröge (slomo) 2010-03-18 09:25:18 UTC
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.
Comment 12 Tim-Philipp Müller 2010-03-18 09:48:55 UTC
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..
Comment 13 Benjamin Otte (Company) 2010-03-18 09:54:14 UTC
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?
Comment 14 Sebastian Dröge (slomo) 2010-03-18 12:04:00 UTC
(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 ;)
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-18 13:47:47 UTC
Sebastian, should the interface Makefile.am just a -I directive to take the local header?
Comment 16 Damien Lespiau 2010-03-19 03:07:41 UTC
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
Comment 17 Damien Lespiau 2010-03-19 03:09:20 UTC
Created attachment 156528 [details] [review]
Quick patch to make introspection data generation work with 0.10.28
Comment 18 Tim-Philipp Müller 2010-03-24 10:04:51 UTC
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..)
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-24 12:49:55 UTC
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.
Comment 20 Tim-Philipp Müller 2010-03-24 18:10:47 UTC
> 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 ;)).
Comment 21 Stefan Sauer (gstreamer, gtkdoc dev) 2010-03-25 09:01:29 UTC
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.
Comment 22 Tim-Philipp Müller 2010-03-28 20:22:39 UTC
> 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.