Re: [Fwd: Re: (partial patch) Fixing some issues with fontconfig]

Hi Petri,

I had an issue subscribing and had to send my message twice. I suspect
that's why your response didn't go through.

Thank you very much for taking the time to explain what I was doing
wrong. The explanation of how pointers work in these circumstances
(passed by value rather than by reference by default) helps greatly and
(mostly) makes sense. Now I understand why I was confused - I had no
idea about that and was looking for something that wasn't allocated or
freed.

Returning the font or null if nothing is found is a great suggestion. My
line of thinking was that if I did that I would need to allocate font in
osd_find_native_font, and that it would never be freed. I was also
confused about whether it would return NULL or a pointer to an address
holding NULL. Like I said, I'm no C coder. In fact this will be my very
first patch to a C program! :)


Here is another attempt that isn't horribly broken (as far as I can tell)

As suggested, I've split this into 2 patches, attached.

* 001-fontconfig_support_filenames.patch makes it possible to use
filenames in subtitles.separate.font_freetype when fontconfig is
enabled. This is pretty trivial and just involves moving that
functionality from osd_lookup_xdg into osd_set_font_freetype2 so that it
is attempted before trying osd_lookup_fontconfig. I did a bunch of tests
with just this patch applied (you make a good point, splitting it into 2
patches makes testing easier) and can confirm that you can now use
filenames and also freetype font names. However if you put in 'cetus'
you'll get the system default font from fontconfig, same as if you type
something invalid.

* 002-fontconfig_prefer_native.patch makes xine prefer to use a native
font over a freetype font if it can find one with the provided name,
making it possible to use native fonts when fontconfig is enabled and
fixing the OSD play/pause icons.

 - i went with osd_lookup_native rather than osd_find_native_font to be
more consistent with the others (osd_lookup_xdg and osd_lookup_fontconfig)

 - I looked through the code a bit more closely and it looks it's OK to
just set osd->font in osd_lookup_native, so that's what I've done. If it
sets osd->font during the first call (the test to see if there is a
native font) then it means that it's going to return 1 and it will just
drop back out to osd_set_font - the freetype/fontconfig code will never
be called, and osd_lookup_native will be called again and set osd->font
again - no big deal. setting osd->font in the function makes the whole
thing simpler. I don't particularly like doing that because to my mind a
function to lookup the font shouldn't also load it - the pedant in me
thinks these should be named osd_load_font_x rather than osd_lookup_x.
But to be fair it is consistent with the behaviour of osd_lookup_xdg and
osd_lookup_fontconfig, which both load the font rather than just looking
it up.

 - this caused another issue in a few places (osd_render_text,
osd_get_text_size) where the code was assuming that if osd->ft2 was set
then a freetype font has been loaded, and tries to access the
uninitialised osd->ft2->face. I added checks to ensure that
osd->ft2->face is initialised. The easier alternative was to call
osd_free_ft2 in osd_set_font if osd_lookup_native returns 1, but that
would mean creating and destroying osd->ft2 repeatedly - inefficient.
This is probably almost a separate issue again, but it's caused by the
changes made in this patch, so I didn't separate it out.

- I also fixed inconsistent indenting in osd_set_font (inside the if
(ret) block)

- you mention font size being an issue for e.g UHD screens. This is a
good point and this patch will cause that issue because e.g native
xine's sans will be shown rather than the freetype sans font.

My preference would be to make bigger native fonts since they tend to
look nicer anyway (e.g the freetype fonts don't have a border for me). I
haven't looked but I would expect that it shouldn't be too hard to do
this by modifying xine-fontconv.

Another approach which may be the best overall would be to integrate
xine-fontconv's functionality into xine and render native fonts from
truetype on the fly when fonts are first loaded. With that approach, all
font rendering would be 'native', and all fonts would get the nice
border added by xine-fontconv, and be scaleable to any size.

Also to support bigger fonts for arbitrarily large screens it may be
necessary to allow users to set the font size as an integer rather than
the current 'small|large|etc' options.

But you're right, that's a separate issue (and a bit of a can of worms).


- One issue that occurs to me is that it's now mostly redundant to have
the two config variables subtitles.separate.font and
subtitles.separate.font_freetype, because now either can be used to
specify either a freetype or native font (as long as ft2 and fontconfig
are compiled in).

This also sort of makes the subtitles.separate.font_use_freetype config
item redundant/inaccurate because it'll now prefer to use a native font
and use freetype if that doesn't work. But I haven't done any testing or
investigation at all as to how this works if freetype or fontconfig are
not compiled in. Perhaps in these cases it makes sense to keep these
config vars.

Still, I think this is an improvement overall :)

> Thanks for looking into this and sending the patch. It is nice to
> see xine is still used

I've been using xine for over 15 years and it's played tens (hundreds?)
of thousands of hours of video for me (my PC is my primary means of
watching movies). It's still my preferred player by a big margin, and
I'll keep using it for as long as I can get it to run. It's about time I
gave something back :)

Dale Magee


On 13/10/18 02:14, Petri Hintukainen wrote:
> Hello,
> 
> Looks like my reply never got to the mailing list ...
> 
> ------- Välitetty viesti ---------
> Lähettäjä: Petri Hintukainen <[email protected]>
> Vastaanottaja: [email protected]
> Aihe: Re: [xine-devel] (partial patch) Fixing some issues with
> fontconfig
> Päiväys: Thu, 11 Oct 2018 12:16:07 +0300
> 
> ma, 2018-10-08 kello 15:04 +1100, Dale Magee kirjoitti:
>> Hi
>>
>> I've just been messing around with tracking down the cause and trying
>> to
>> fix a couple of issues I've found WRT the way fontconfig works in
>> xine-lib.
>>
>> The attached patch is what I think it should be doing, but I'm no C
>> coder and this code causes a segfault at the moment. I've tried a
>> bunch
>> of different things to fix it but I obviously don't understand what's
>> going on and I've run out of time and patience today (it's now 2am).
>> If
>> somebody could explain why it causes a segfault and how to fix that
>> I'd
>> be very grateful as this might help me understand C better
>> (particularly
>> memory allocation and freeing) :)
> 
> Hello,
> 
> I think the problem is with splitting osd_find_native_font(). Looks
> like the new function is missing functionality of one line from the
> original code:
>     osd->font = font;
> So, it does not remember nor return the best font.
> 
> 
> The crash itself is caused by variable
>     osd_font_t *font = NULL;
> This is never changed to point to the actual font, leading to crash at
> line
>     ret = osd_renderer_load_font(osd->renderer, osd->font>filename);
> where NULL osd->font is dereferenced.
> 
> 
> There are multiple ways to fix this. Probably simplest would be
> returning pointer to found font. Returning integer "found"/"not found"
> is redundant if we return the actual "font" or "no font" (=NULL).
> 
> 
> Looking at the code I think you're trying to return the font to caller
> in the variable given as last argument.
> 
> C pointers can be tricky. If you pass a pointer to a function, you're
> passing the address that is stored in the pointer variable. Just as if
> it was "normal" integer.
>     osd_font_t *font = NULL;
>     func(font);
> Now func() is given the address stored in variable font (NULL in this
> case). It can't change the value of variable font, it sees only the
> address that was stored in font. To modify the actual variable "font"
> in the above code, you need to take a pointer to it (= pointer to
> pointer). something like:
>   osd_font_t *font = NULL;
>   func(&font);
> Now func() knows the address of "font" and can modify the value of
> variable "font". Something like:
>   func(osd_font_t **pfont) {
>     osd_font_t *f = find_font();
>     /* store pointer "f" to the variable
>        where "pfont" points to */
>     *pfont = f;
>   }
> 
> This could be easier to understand if osd_font_t was not a pointer:
>   int font;
>   /* font passed by value -> func() can't change local variable font */
>   func(font);
> and
>   void func(int i) {
>     /* only the local variable i is changed -
>        caller does not see this change in variable font */
>     i = 10;
>   }
> With pointers:
>   int font;
>   func(&font);
> and
>   func (int *p) { 
>     /* modify caller variable "font" where p points to */
>     *p = 111;
>   }
> 
> 
> This would change the implementation to something like:
> 
>  static int osd_find_native_font(osd_object_t *osd,
>                                  const char *fontname,
>                                  int size,
> -                                osd_font_t *font) {
> +                                osd_font_t **pfont) {
>  {
>    int best = 0;
>    int found = 0;
> +  osd_font_t *font;
> 
>    font = osd->renderer->fonts;
>    while( font ) {
> 
>      if( !strcasecmp(font->name, fontname) && (size>=font->size
>          && (best<font->size)) {
>        found = 1;
>        best = font->size;
>        lprintf ("best: font->name=%s, size=%d\n",
>                 font->name, font->size);
> +      /* "return" the font to caller */
> +      if (pfont)
> +        *pfont = font;
>      }
>      font = font->next;
>    }
>    return found;
>  }
> 
> and you would invoke it like this:
> 
>   /* just look if the font is available */
>   if (osd_find_native_font(osd, fontname, size, NULL))
>     return 0;
> 
> and
> 
>   /* get the actual font too */
>   ret = osd_find_native_font(osd, fontname, size, &font);
> 
> 
> An alternative would be storing the font directly to osd->font like in
> the original code (well, I didn't look to the code to see if that would
> cause some side effects in the first use case).
> 
> 
> If you don't want or can't modify osd->font directly, simpler could be
> returning the best font (or NULL if none was found). Something like:
> 
> static osd_font_t *int osd_find_native_font(osd_object_t *osd,
>        const char *fontname, int size) {
>   osd_font_t *best = NULL, *font;
> 
>   font = osd->renderer->fonts;
>   while( font ) {
>     if( !strcasecmp(font->name, fontname) && (size >= font->size)
>          && (best == NULL || best->size < font->size)) {
>       best = font;
>       lprintf ("best: font->name=%s, size=%d\n",
>                font->name, font->size);
>     }
>     font = font->next;
>   }
>   return best;
> }
> 
> Now, the first use case would work just as-is (check if the function
> returns non-NULL):
> 
>     if (osd_find_native_font(osd, fontname, size))
>       return 0;
> 
> and the second one would be something like:
> 
>    font = osd_find_native_font(osd, fontname, size);
> 	
> 
>> This patch addresses 2 issues that I've discovered when compiling
>> with
>> fontconfig enabled:
>>
>> 1. When using fontconfig it's not possible for xine to use the
>> cetus font for OSD, because osd_set_font tries to load a font using
>> freetype (and hence fontconfig) first. When osd_set_font_freetype2
>> passes 'cetus' as the font name to osd_lookup_fontconfig, fontconfig
>> loads a system default font because it can't find a font named cetus.
>> This means that play/pause icons in the OSD are replaced with the
>> corresponding characters (">" and "<").
>>
>> 2. When compiled with fontconfig you can not use a filename in
>> subtitles.separate.font_freetype, because when this filename is
>> passed
>> to fontconfig it looks by font name rather than filename and returns
>> the
>> system default font.
>>
>> Here's what my patch is trying to do:
>>
>> 0. abstract out the bit of osd_set_font which searches for a matching
>> native font into a new function osd_find_native_font (to be DRY).
>>
>> 1. osd_set_font_freetype2 first tries to use fontname as a filename
>> and
>> load it (to support filenames in subtitles.separate.font_freetype).
>>
>> 2. osd_set_font_freetype2 then looks for a native xine font with the
>> provided name. If it finds it, it immediately returns 0 so that
>> osd_set_font can load the native xine font (i.e xine fonts are now
>> used
>> as preference over freetype fonts. This should be fine because
>> according
>> to the FAQ the native fonts are better. e.g for subtitles they have
>> borders).
>>
>> 3. if the 2 above steps fail, osd_set_font_freetype2 passes the font
>> name to fontconfig, which may find the font or return the system
>> default
>> font (i.e previous behaviour).
>>
>> I'd love to hear any thoughts and feedback.
> 
> Looks OK. I think 1 and 2 should be separate patches (those fix
> unrelated issues).
> 
> With 2, the only issue I have in mind is if the internal fonts support
> large enough sizes. With UHD resolutions even the largest subtitle size
> is _very_ small. Maybe we should generate larger fonts or make font
> selection also based on available sizes (= prefer freetype when ex.
> 100+ pixel font is requested, but internal font is only 64 pixels).
> 
> But that doesn't matter yet (even subtitle size selection doesn't
> support larger sizes). And, as you said, lookup order still needs to be
> reversed.
> 
>> Thanks for your time :)
> 
> Thanks for looking into this and sending the patch. It is nice to see
> xine is still used :)
> 
>> Dale Magee
>>
>> _______________________________________________
>> xine-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/xine-devel

_______________________________________________
xine-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/xine-devel

--- a/src/xine-engine/osd.c
+++ b/src/xine-engine/osd.c
@@ -1076,10 +1076,6 @@ static int osd_lookup_fontconfig( osd_object_t *osd, const char *const fontname,
 static int osd_lookup_xdg( osd_object_t *osd, const char *const fontname ) {
   const char *const *data_dirs = xdgSearchableDataDirectories(&osd->renderer->stream->xine->basedir_handle);
 
-  /* try load font from current directory or from an absolute path */
-  if ( FT_New_Face(osd->ft2->library, fontname, 0, &osd->ft2->face) == FT_Err_Ok )
-    return 1;
-
   if ( data_dirs )
     while( (*data_dirs) && *(*data_dirs) ) {
       FT_Error fte = FT_Err_Ok;
@@ -1118,6 +1114,15 @@ static int osd_set_font_freetype2( osd_object_t *osd, const char *fontname, int
   }
 
   do { /* while 0 */
+  
+    /* 
+    try to load font from current directory or from an absolute path 
+      we want to do this before trying osd_lookup_fontconfig
+      (which doesn't handle filenames)
+    */
+    if ( FT_New_Face(osd->ft2->library, fontname, 0, &osd->ft2->face) == FT_Err_Ok )
+      break;
+  
 #ifdef HAVE_FONTCONFIG
     if ( osd_lookup_fontconfig(osd, fontname, size) )
       break;

--- a/src/xine-engine/osd.c
+++ b/src/xine-engine/osd.c
@@ -1013,6 +1013,36 @@ static int osd_renderer_unload_font(osd_renderer_t *this, char *fontname ) {
   return ret;
 }
 
+/*
+ look for a native xine font matching the given name and size and load it 
+ into osd->font.
+ return nonzero if a native font is found, zero if not
+*/
+static int osd_lookup_native( osd_object_t *osd, const char *fontname, int size ) {
+    osd_font_t *font;
+    int best = 0;
+    int ret = 0;
+
+    font = osd->renderer->fonts;
+    while( font ) {
+      if( !strcasecmp(font->name, fontname) && (size>=font->size)
+          && (best<font->size)) {
+        ret = 1;
+        osd->font = font;
+        best = font->size;
+        //lprintf ("best: font->name=%s, size=%d\n", font->name, font->size);
+      }
+      font = font->next;
+    }
+    if (ret)
+      lprintf("native match for %s %1d: %s %1d\n",fontname,size,osd->font->name,osd->font->size);
+    else
+      lprintf("no native font matching %s %1d",fontname,size);
+    
+    return ret;
+}
+
+
 #ifdef HAVE_FT2
 
 # ifdef HAVE_FONTCONFIG
@@ -1123,6 +1153,16 @@ static int osd_set_font_freetype2( osd_object_t *osd, const char *fontname, int
     if ( FT_New_Face(osd->ft2->library, fontname, 0, &osd->ft2->face) == FT_Err_Ok )
       break;
   
+    /*
+	try to find a native xine font and return 0 if it succeeds, 
+	  allowing that to load.
+	  this has to happen before calling osd_lookup_fontconfig
+	  so that you don't get the system default font when trying 
+	  to load e.g Cetus.
+	*/
+    if (osd_lookup_native(osd,fontname,size))
+	  return 0;
+  
 #ifdef HAVE_FONTCONFIG
     if ( osd_lookup_fontconfig(osd, fontname, size) )
       break;
@@ -1160,31 +1200,17 @@ static int osd_set_font( osd_object_t *osd, const char *fontname, int size) {
 #ifdef HAVE_FT2
   if ( ! osd_set_font_freetype2(osd, fontname, size) )
 #endif
-    { /* If the FreeType2 loading failed */
-      osd_font_t *font;
-      int best = 0;
+    { /* If the FreeType2 loading failed
+		  (which can happen if it finds a native xine font)
+		*/
       osd->font = NULL;
-      ret = 0;
-
-      font = osd->renderer->fonts;
-      while( font ) {
-
-	if( !strcasecmp(font->name, fontname) && (size>=font->size)
-	    && (best<font->size)) {
-	  ret = 1;
-	  osd->font = font;
-	  best = font->size;
-	  lprintf ("best: font->name=%s, size=%d\n", font->name, font->size);
-	}
-	font = font->next;
-      }
-
+      ret = osd_lookup_native(osd,fontname,size);
       if( ret ) {
-	/* load font if needed */
-	if( !osd->font->loaded )
-	  ret = osd_renderer_load_font(osd->renderer, osd->font->filename);
-	if(!ret)
-	  osd->font = NULL;
+        /* load font if needed */
+        if( !osd->font->loaded )
+          ret = osd_renderer_load_font(osd->renderer, osd->font->filename);
+        if(!ret)
+          osd->font = NULL;
       }
     }
 
@@ -1343,15 +1369,15 @@ static int osd_render_text (osd_object_t *osd, int x1, int y1,
   const char *inbuf;
   uint16_t unicode;
   size_t inbytesleft;
-
+  
+  lprintf("osd=%p (%d,%d) \"%s\"\n", osd, x1, y1, text);
+  
 #ifdef HAVE_FT2
   FT_UInt previous = 0;
-  FT_Bool use_kerning = osd->ft2 && FT_HAS_KERNING(osd->ft2->face);
+  FT_Bool use_kerning = osd->ft2 && osd->ft2->face && FT_HAS_KERNING(osd->ft2->face);
   int first = 1;
 #endif
 
-  lprintf("osd=%p (%d,%d) \"%s\"\n", osd, x1, y1, text);
-
   /* some sanity checks for the color indices */
   if( color_base < 0 )
     color_base = 0;
@@ -1365,7 +1391,7 @@ static int osd_render_text (osd_object_t *osd, int x1, int y1,
 
     if ((font = osd->font)) proceed = 1;
 #ifdef HAVE_FT2
-    if (osd->ft2) proceed = 1;
+    if (osd->ft2 && osd->ft2->face) proceed = 1;
 #endif
 
     if (proceed == 0) {
@@ -1394,7 +1420,7 @@ static int osd_render_text (osd_object_t *osd, int x1, int y1,
 
 
 #ifdef HAVE_FT2
-    if (osd->ft2) {
+    if (osd->ft2 && osd->ft2->face) {
 
       FT_GlyphSlot slot = osd->ft2->face->glyph;
 
@@ -1496,7 +1522,7 @@ static int osd_render_text (osd_object_t *osd, int x1, int y1,
       }
 
 #ifdef HAVE_FT2
-    } /* !(osd->ft2) */
+    } /* !(osd->ft2 && osd->ft2->face) */
 #endif
 
   }
@@ -1520,7 +1546,7 @@ static int osd_get_text_size(osd_object_t *osd, const char *text, int *width, in
 
 #ifdef HAVE_FT2
   /* not all free type fonts provide kerning */
-  FT_Bool use_kerning = osd->ft2 && FT_HAS_KERNING(osd->ft2->face);
+  FT_Bool use_kerning = osd->ft2 && osd->ft2->face && FT_HAS_KERNING(osd->ft2->face);
   FT_UInt previous = 0;
   int first_glyph = 1;
 #endif
@@ -1534,7 +1560,7 @@ static int osd_get_text_size(osd_object_t *osd, const char *text, int *width, in
 
     if ((font = osd->font)) proceed = 1;
 #ifdef HAVE_FT2
-    if (osd->ft2) proceed = 1;
+    if (osd->ft2 && osd->ft2->face) proceed = 1;
 #endif
 
     if (proceed == 0) {
@@ -1561,7 +1587,7 @@ static int osd_get_text_size(osd_object_t *osd, const char *text, int *width, in
 #endif
 
 #ifdef HAVE_FT2
-    if (osd->ft2) {
+    if (osd->ft2 && osd->ft2->face) {
       FT_GlyphSlot  slot = osd->ft2->face->glyph;
 
       i = FT_Get_Char_Index( osd->ft2->face, unicode);
@@ -1601,12 +1627,12 @@ static int osd_get_text_size(osd_object_t *osd, const char *text, int *width, in
         *width += font->fontchar[i].width - (font->fontchar[i].width * FONT_OVERLAP);
       }
 #ifdef HAVE_FT2
-    } /* !(osd->ft2) */
+    } /* !(osd->ft2 && osd->ft2->face) */
 #endif
   }
 
 #ifdef HAVE_FT2
-  if (osd->ft2) {
+  if (osd->ft2 && osd->ft2->face) {
     /* if we have a true type font we need to do some corrections for the last
      * letter. As this one is still in the gylph slot we can still work with
      * it. For the last letter be must not use advance and width but the real