GNOME Bugzilla – Bug 761251
textoverlay: Expose text rendering dimensions to applications and remove absolute positioning limit
Last modified: 2016-03-11 15:56:11 UTC
Created attachment 319937 [details] [review] textoverlay: Do not limit positioning to video area. These two patches allows applications like Pitivi to modify the text position in a visual editor. The first patch removes the limit of the posx / posy parameters be inside the video canvas. If the feature of the "snap to border" behavior is required, it should be exposed in another alignment mode. "position" should be the absolute position without limits. The second patch allows to acquire the rendering dimensions (x, y, width, height) of the pango text, so hover listeners can be implemented. Related Pitivi issue: https://phabricator.freedesktop.org/T3523
Created attachment 319938 [details] [review] textoverlay: Expose rendering dimensions as properties.
Review of attachment 319938 [details] [review]: Apart from that it looks good. ::: ext/pango/gstbasetextoverlay.c @@ +400,3 @@ + + /** + * GstBaseTextOverlay:image-x: Should be `text-x` now
Review of attachment 319937 [details] [review]: Sounds good to me, I am not sure why there were those arbitrary limitations?
Created attachment 320422 [details] [review] textoverlay: Expose rendering dimensions as properties. * Fix property name in comment.
Review of attachment 320422 [details] [review]: OK
Attachment 319937 [details] pushed as a48daf6 - textoverlay: Do not limit positioning to video area. Attachment 320422 [details] pushed as 8e07203 - textoverlay: Expose rendering dimensions as properties.
Comment on attachment 319937 [details] [review] textoverlay: Do not limit positioning to video area. Just a comment on this: this breaks applications that have been using this property to position text via the GstController API, since the property range has changed now, and GstController maps 0-1.0 to the property range. Do you need this change for pitivi/ges? I don't think you should be using these relative positioning properties at all in your case.
Yes we need this for Pitivi/GES, and we need to be able to use the controller API on them too. From the namings, halignment type *position* and the name of the prop, posx, those are supposed to be absolute values, though they have a weird semantic. If you want to use relative one should use another alignment type. I understand this might break code and I am not sure what to do with that if anything?
> I understand this might break code and I am not sure what to do with that if > anything? Re-opening then for discussion.
We could add a new property to switch between old and new mode, that way we wouldn't break anything at least.
Any progress here? Otherwise we should revert these commits again for 1.8
Ah, no please don't :)
So what should we do here then instead? :) Add a property?
> Ah, no please don't :) A slightly more pro-active approach on how to move forward here might be advisable then :) The problem is that the GstController API is weird to begin with, it should really have defaulted to using absolute values, but that's unfortunately not how it works by default and we have to deal with that. That means that anyone who was using GstController with xpos/ypos before would have been able to map the values passed to controller to the interface now. Since these properties were controllable before, and their controllability has been requested specifically (e.g. bug #638859) and it also comes up on irc and the mailing list from time to time there's a reasonable chance that people are actually using this. Sebastian: the change of property range is the problem here in itself if I'm not mistaken. Thibault: why do you need relative positioning in pitivi? Wouldn't it make more sense to make text-x/y writable as well, or add other properties for setting absolute position in pixel units instead? Or you could just use deltax/deltay really in combination with relative position 0.0/0.0 ?
I discussed on IRC about that with Sebastian, we are writting a patch where we had a new absolute position mode. The problem here is not the range, but the clamped value for X/Y.
> The problem here is not the range, but the clamped value for X/Y. Maybe we're talking about different problems then?
So I came up with a patch that introduces a new alignment without the clamp, so the old alignment mode will behave like before: https://git.collabora.com/cgit/user/lubosz/gst-plugins-base.git/commit/?h=textoverlay-pitivi2&id=caa677447abe18d2a93a2e5477459ab5afe48cd1 But what I understand now is that the property range is not compatible with how GstController uses it. Can't GstController handle +/-MAX_DOUBLE values at all, or is just changing the range of an existing property breaking compatibility? == About the current mapping + clamping: == I can't understand why the property gets clamped and not mapped to the range it wants to achieve: For example the current upper left position for an example text is ~ [0.17, 0.02] http://i.imgur.com/RZmu7nB.png This is dependend on canvas and text aspect ratios. The LEGAL properties between [0,0] and [0.17, 0.02] are clamped to the same position (upper left corner). This is a hack to achieve that 0,0 is the upper left corner. The same problem occurs at the bottom left, where [0.83, 0.98] is bottom right, and legal values between [0.83, 0.98] - [1.0,1.0] are useless. This mapping will lead to weird animations with GstController. I don't know why this mapping is used, maybe because of the padding, but a valid range [0,0]-[1,1] for inside the canvas could be mapped like this without clamping: http://i.imgur.com/uhMrj11.jpg And without restriction it could actually be moved out of canvas, to make animations where text slides in etc. TL;DR: The current mapping does not make sense to me. == About how to solve the incompatibility of the new range: == I think thibault's solution to introduce 2 new properties (btw, are there vector / list properties?) for positioning with infinite range. Should this property be only used when the alignment mode is changed to absolute, like in the patch above? Will an infinite range be usable in GstController?
Infinity is probably not going to work well, you can't do calculations with that. If you really mean infinity, and not just the maximum double value before infinity. In any case, what is the simplest solution at this point to keep compatibility?
GstController should be able to handle a [ -MAXDOUBLE, +MAXDOUBLE ] range in principle, it's just that it maps a 0...1 range in the controller values to the property range, so if you change the property range of an existing property then a controller value of e.g. 0.5 suddenly changes its meaning and results in a different position.
Created attachment 323609 [details] [review] basetextoverlay: Add new alignment type for unclamped positions Maintains original behavior of GST_BASE_TEXT_OVERLAY_*ALIGN_POS and introduces GST_BASE_TEXT_OVERLAY_*ALIGN_ABSOLUTE for unclamped positioning. Maps absolute coordinates to be exactly inside of canvas for [0,0] - [1,1]. This means values [0.8,0.8] - [1.0,1.0] do not map to the same position anymore. [0,0]: Text Top Left is aligned to Video Top Left [0.5,0.5]: Both centers are aligned [1,1]: Bottom rights are aligned
Created attachment 323610 [details] [review] basetextoverlay: Add new position properties for absolute alignment Restore old range of xpos / ypos and add a new property to write position with +/- MAX_DOUBLE range.
Comment on attachment 323610 [details] [review] basetextoverlay: Add new position properties for absolute alignment >+ /** >+ * GstBaseTextOverlay:xabsolute: >+ * >+ * Horizontal position of the rendered text when using positioned alignment. >+ */ >+ g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_X_ABSOLUTE, >+ g_param_spec_double ("xabsolute", "horizontal position", >+ "Horizontal position when using absolute alignment", -G_MAXDOUBLE, >+ G_MAXDOUBLE, DEFAULT_PROP_XPOS, >+ G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); - "xabsolute" should be "x-absolute" IMHO (I know it's also "xpos" etc., but no need to continue that bad tradition :)) - could you add a note what the units are - also please add a 'Since: 1.8' at the end - gtk-doc chunk text doesn't match GParamSpec text (positioned alignment vs. absolute alignment) >+ /** >+ * GstBaseTextOverlay:yabsolute: >+ * >+ * Vertical position of the rendered text when using positioned alignment. >+ */ >+ g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_Y_ABSOLUTE, >+ g_param_spec_double ("yabsolute", "vertical position", >+ "Vertical position when using absolute alignment", -G_MAXDOUBLE, > G_MAXDOUBLE, DEFAULT_PROP_YPOS, > G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); See above
Thanks! I think we should revert a48daf6dd8cb69b4260a03aa7f3cdf227d4f1602 and re-do these patches on top of that reverted commit then.
Created attachment 323693 [details] [review] basetextoverlay: Add new alignment type for unclamped absolute positions. On top of reverted a48daf6dd8cb69b4260a03aa7f3cdf227d4f1602.
Created attachment 323694 [details] [review] basetextoverlay: Add new alignment type for unclamped absolute positions. Add "Since 1.8"
Created attachment 323695 [details] [review] basetextoverlay: Add new alignment type for unclamped absolute positions. Adds mapping description to doc string.
Great, thanks! commit 8a443784c47431f41fe8dc5726cb2977e33fea9b Author: Lubosz Sarnecki <lubosz.sarnecki@collabora.co.uk> Date: Tue Mar 8 19:22:34 2016 +0100 basetextoverlay: Add new properties and alignment type for unclamped absolute positions Introduces [x-absolute, y-absolute] properties for positioning in +/- MAX_DOUBLE range. Adds new (h/v)alignment type "absolute" where coordinates map the text area to be exactly inside of video canvas for [0, 0] - [1, 1]: [0, 0]: Top-Lefts of video and text are aligned [0.5, 0.5]: Centers are aligned [1, 1]: Bottom-Rights are aligned https://bugzilla.gnome.org/show_bug.cgi?id=761251 commit 241fcaa64567489ab592f391da66af71528ce373 Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Mar 11 13:15:03 2016 +0000 Revert "textoverlay: Do not limit positioning to video area." This reverts commit a48daf6dd8cb69b4260a03aa7f3cdf227d4f1602. This changed behaviour in a way that's not always backwards-compatible. https://bugzilla.gnome.org/show_bug.cgi?id=761251
Comment on attachment 323695 [details] [review] basetextoverlay: Add new alignment type for unclamped absolute positions. >From cb506405eeaac657f2673f5f194c0a6ddd1ed4ff Mon Sep 17 00:00:00 2001 >From: Lubosz Sarnecki <lubosz.sarnecki@collabora.co.uk> >Date: Tue, 8 Mar 2016 19:22:34 +0100 >Subject: [PATCH 2/2] basetextoverlay: Add new alignment type for unclamped > absolute positions. > >Introduces [x-absolute, y-absolute] properties >for positioning in +/- MAX_DOUBLE range. > >Adds new (h/v)alignment type "absolute" where coordinates >map the text area to be exactly inside of video canvas for [0, 0] - [1, 1]: > >[0, 0]: Top-Lefts of video and text are aligned >[0.5, 0.5]: Centers are aligned >[1, 1]: Bottom-Rights are aligned > >https://bugzilla.gnome.org/show_bug.cgi?id=761251 >--- > common | 2 +- > ext/pango/gstbasetextoverlay.c | 68 +++++++++++++++++++++++++++++++++++++++--- > ext/pango/gstbasetextoverlay.h | 6 ++-- > 3 files changed, 69 insertions(+), 7 deletions(-) > >diff --git a/common b/common >index 6f2d209..b64f03f 160000 >--- a/common >+++ b/common >@@ -1 +1 @@ >-Subproject commit 6f2d2093e84cc0eb99b634fa281822ebb9507285 >+Subproject commit b64f03f6090245624608beb5d2fff335e23a01c0 >diff --git a/ext/pango/gstbasetextoverlay.c b/ext/pango/gstbasetextoverlay.c >index 669b0c2..e0bb169 100644 >--- a/ext/pango/gstbasetextoverlay.c >+++ b/ext/pango/gstbasetextoverlay.c >@@ -90,6 +90,8 @@ enum > PROP_DELTAY, > PROP_XPOS, > PROP_YPOS, >+ PROP_X_ABSOLUTE, >+ PROP_Y_ABSOLUTE, > PROP_WRAP_MODE, > PROP_FONT_DESC, > PROP_SILENT, >@@ -141,8 +143,10 @@ gst_base_text_overlay_valign_get_type (void) > {GST_BASE_TEXT_OVERLAY_VALIGN_BASELINE, "baseline", "baseline"}, > {GST_BASE_TEXT_OVERLAY_VALIGN_BOTTOM, "bottom", "bottom"}, > {GST_BASE_TEXT_OVERLAY_VALIGN_TOP, "top", "top"}, >- {GST_BASE_TEXT_OVERLAY_VALIGN_POS, "position", "position"}, >+ {GST_BASE_TEXT_OVERLAY_VALIGN_POS, "position", >+ "Absolute position clamped to canvas"}, > {GST_BASE_TEXT_OVERLAY_VALIGN_CENTER, "center", "center"}, >+ {GST_BASE_TEXT_OVERLAY_VALIGN_ABSOLUTE, "absolute", "Absolute position"}, > {0, NULL, NULL}, > }; > >@@ -163,7 +167,9 @@ gst_base_text_overlay_halign_get_type (void) > {GST_BASE_TEXT_OVERLAY_HALIGN_LEFT, "left", "left"}, > {GST_BASE_TEXT_OVERLAY_HALIGN_CENTER, "center", "center"}, > {GST_BASE_TEXT_OVERLAY_HALIGN_RIGHT, "right", "right"}, >- {GST_BASE_TEXT_OVERLAY_HALIGN_POS, "position", "position"}, >+ {GST_BASE_TEXT_OVERLAY_HALIGN_POS, "position", >+ "Absolute position clamped to canvas"}, >+ {GST_BASE_TEXT_OVERLAY_HALIGN_ABSOLUTE, "absolute", "Absolute position"}, > {0, NULL, NULL}, > }; > >@@ -445,7 +451,7 @@ gst_base_text_overlay_class_init (GstBaseTextOverlayClass * klass) > */ > g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_XPOS, > g_param_spec_double ("xpos", "horizontal position", >- "Horizontal position when using position alignment", 0, 1.0, >+ "Horizontal position when using clamped position alignment", 0, 1.0, > DEFAULT_PROP_XPOS, > G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); > /** >@@ -455,9 +461,45 @@ gst_base_text_overlay_class_init (GstBaseTextOverlayClass * klass) > */ > g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_YPOS, > g_param_spec_double ("ypos", "vertical position", >- "Vertical position when using position alignment", 0, 1.0, >+ "Vertical position when using clamped position alignment", 0, 1.0, > DEFAULT_PROP_YPOS, > G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); >+ >+ /** >+ * GstBaseTextOverlay:x-absolute: >+ * >+ * Horizontal position of the rendered text when using absolute alignment. >+ * >+ * Maps the text area to be exactly inside of video canvas for [0, 0] - [1, 1]: >+ * >+ * [0, 0]: Top-Lefts of video and text are aligned >+ * [0.5, 0.5]: Centers are aligned >+ * [1, 1]: Bottom-Rights are aligned >+ * >+ * Values beyond [0, 0] - [1, 1] place the text outside of the video canvas. >+ * >+ * Since: 1.8 >+ */ >+ g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_X_ABSOLUTE, >+ g_param_spec_double ("x-absolute", "horizontal position", >+ "Horizontal position when using absolute alignment", -G_MAXDOUBLE, >+ G_MAXDOUBLE, DEFAULT_PROP_XPOS, >+ G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); >+ /** >+ * GstBaseTextOverlay:y-absolute: >+ * >+ * See x-absolute. >+ * >+ * Vertical position of the rendered text when using absolute alignment. >+ * >+ * Since: 1.8 >+ */ >+ g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_Y_ABSOLUTE, >+ g_param_spec_double ("y-absolute", "vertical position", >+ "Vertical position when using absolute alignment", -G_MAXDOUBLE, >+ G_MAXDOUBLE, DEFAULT_PROP_YPOS, >+ G_PARAM_READWRITE | GST_PARAM_CONTROLLABLE | G_PARAM_STATIC_STRINGS)); >+ > g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_WRAP_MODE, > g_param_spec_enum ("wrap-mode", "wrap mode", > "Whether to wrap the text and if so how.", >@@ -978,6 +1020,12 @@ gst_base_text_overlay_set_property (GObject * object, guint prop_id, > case PROP_YPOS: > overlay->ypos = g_value_get_double (value); > break; >+ case PROP_X_ABSOLUTE: >+ overlay->xpos = g_value_get_double (value); >+ break; >+ case PROP_Y_ABSOLUTE: >+ overlay->ypos = g_value_get_double (value); >+ break; > case PROP_VALIGNMENT: > overlay->valign = g_value_get_enum (value); > break; >@@ -1091,6 +1139,12 @@ gst_base_text_overlay_get_property (GObject * object, guint prop_id, > case PROP_YPOS: > g_value_set_double (value, overlay->ypos); > break; >+ case PROP_X_ABSOLUTE: >+ g_value_set_double (value, overlay->xpos); >+ break; >+ case PROP_Y_ABSOLUTE: >+ g_value_set_double (value, overlay->ypos); >+ break; > case PROP_VALIGNMENT: > g_value_set_enum (value, overlay->valign); > break; >@@ -1496,6 +1550,9 @@ gst_base_text_overlay_get_pos (GstBaseTextOverlay * overlay, > if (*xpos < 0) > *xpos = 0; > break; >+ case GST_BASE_TEXT_OVERLAY_HALIGN_ABSOLUTE: >+ *xpos = (overlay->width - overlay->text_width) * overlay->xpos; >+ break; > default: > *xpos = 0; > } >@@ -1521,6 +1578,9 @@ gst_base_text_overlay_get_pos (GstBaseTextOverlay * overlay, > *ypos = (gint) (overlay->height * overlay->ypos) - height / 2; > *ypos = CLAMP (*ypos, 0, overlay->height - overlay->ink_rect.height); > break; >+ case GST_BASE_TEXT_OVERLAY_VALIGN_ABSOLUTE: >+ *ypos = (overlay->height - overlay->text_height) * overlay->ypos; >+ break; > case GST_BASE_TEXT_OVERLAY_VALIGN_CENTER: > *ypos = (overlay->height - height) / 2; > break; >diff --git a/ext/pango/gstbasetextoverlay.h b/ext/pango/gstbasetextoverlay.h >index 76fb10d..a078fb7 100644 >--- a/ext/pango/gstbasetextoverlay.h >+++ b/ext/pango/gstbasetextoverlay.h >@@ -62,7 +62,8 @@ typedef enum { > GST_BASE_TEXT_OVERLAY_VALIGN_BOTTOM, > GST_BASE_TEXT_OVERLAY_VALIGN_TOP, > GST_BASE_TEXT_OVERLAY_VALIGN_POS, >- GST_BASE_TEXT_OVERLAY_VALIGN_CENTER >+ GST_BASE_TEXT_OVERLAY_VALIGN_CENTER, >+ GST_BASE_TEXT_OVERLAY_VALIGN_ABSOLUTE > } GstBaseTextOverlayVAlign; > > /** >@@ -80,7 +81,8 @@ typedef enum { > GST_BASE_TEXT_OVERLAY_HALIGN_CENTER, > GST_BASE_TEXT_OVERLAY_HALIGN_RIGHT, > GST_BASE_TEXT_OVERLAY_HALIGN_UNUSED, >- GST_BASE_TEXT_OVERLAY_HALIGN_POS >+ GST_BASE_TEXT_OVERLAY_HALIGN_POS, >+ GST_BASE_TEXT_OVERLAY_HALIGN_ABSOLUTE > } GstBaseTextOverlayHAlign; > > /** >-- >2.7.2 >