Created attachment 371743 [details] [review]
patch

Review of attachment 371743 [details] [review]:

Looks good to me.

Created attachment 371757 [details] [review]
0001-subtitleoverlay-Add-background-color-property.patch

Created attachment 371758 [details] [review]
0002-playsink-Add-subtitle-background-color-property.patch

Created attachment 371759 [details] [review]
0003-playbin-Add-subtitle-background-color-property.patch

Created attachment 371760 [details] [review]
0004-playbin3-Add-subtitle-background-color-property.patch

Ideally though, I would also like to be able to configure the font color from GstPlayer, that would mean more properties duplicated :(

Perhaps textoverlay could handle a special configuration GstEvent? That event could be sent by GstPlayer and thus reduce the property churn... Any thoughts on this?

(In reply to Philippe Normand from comment #7)
> Ideally though, I would also like to be able to configure the font color
> from GstPlayer, that would mean more properties duplicated :(
> 
> Perhaps textoverlay could handle a special configuration GstEvent? That
> event could be sent by GstPlayer and thus reduce the property churn... Any
> thoughts on this?

Thibault suggested to have a new "subtitle format" property in GstPlayer and playbin(3) that would describe:

- fg color
- bg color
- font
- what else? :)

Any thoughts about that idea?

Regardless, can someone please review the textoverlay patch?

I wonder what happen when the subtitles themselves defines their fg/bg color

Another subtitle properties I'd like to see is position and subtitle delay.

(In reply to Víctor Manuel Jáquez Leal from comment #9)
> I wonder what happen when the subtitles themselves defines their fg/bg color
> 

Right now subparse doesn't support this, afaik.

*** Bug 797112 has been marked as a duplicate of this bug. ***

Review of attachment 371743 [details] [review]:

::: ext/pango/gstbasetextoverlay.c
@@ +2020,3 @@
+  cairo_set_source_rgba (cr, r / 255.0, g / 255.0, b / 255.0, a / 255.0);
+  cairo_rectangle (cr, ink_rect.x, ink_rect.y, ink_rect.width, ink_rect.height);
+  cairo_fill (cr);

It would be better to just do this as part of the clearing of the surface in the very beginning. Use the SOURCE instead of CLEAR operator for that then.

Also the ink rectangle is suboptimal. See the docs of pango_layout_get_extents():

> Computes the logical and ink extents of layout . Logical extents are usually what you want for
> positioning things. Note that both extents may have non-zero x and y. You may want to use those to
> offset where you render the layout. Not doing that is a very typical bug that shows up as right-to-
> left layouts not being correctly positioned in a layout with a set width.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/450.