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

On 19/10/18 21:05, Petri Hintukainen wrote:
> 
> Looks like sourceforge doesn't allow posts from my sourceforge alias
> anymore. Messages just disappear. Maybe they've activated some kind of
> new spam filter...
>

Weird.

>> 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)
> 
> The second iteration looks good. You must have experience with some
> other language(s) :)
> 

Thanks. Yeah, I have a long history of working with a bunch of other
languages (php, python, ruby, javascript, delphi, others), just not very
much C :)
> 
> Both patches look fine.
> Pushed, thanks!
> 

Excellent! Thanks again for your help!

>>
>> 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.
> 
> Yes, it seems easy to do. Generated font sizes are hardcoded in the
> conversion program.
> 
>> 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.
> 
> I was also thinking about this. We could simply improve rendering code
> and install cetus.ttf.
> 
> Maybe native fonts could still be used as fallback when truetype is
> disabled.
>

That seems like a sensible approach. Maybe I'll have a bit more of a
look into the code and see if this might be something I could do, or at
least contribute to. I'm a bit busy at the moment though so it might be
a couple of weeks before I get a chance to take a look.

>> 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).
> 
> I tried adding few new size options. It worked as expected, but I
> wasn't still happy with it. When playing in smaller window text was far
> too large :)
> 
> Maybe subtitle font size should be relative to output size. We could 
> scale those "huge"/"tiny" sizes to match output resolution, or add an
> option to set font size in % of output size.
> 

I think that percentage of output size is a sensible option, but I think
it should be in addition to the fixed sizes.

>> - 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.
> 
> I haven't looked those config items, but if there are useless /
> redundant items, we could deprecate those. Having less config options
> is always a good thing.
> Maybe use_freetype was meant to allow selecting between built-in and
> freetype fonts. With that, different options for freetype and non-
> freetype make sense. But, still, having only single entry (font) seems
> simpler ...
> 

use freetype used to select between native and freetype fonts, i.e when
disabled you would never get a freetype font and when enabled you would
never get a native font. Now when this option is enabled it is possible
to get a native font if you use e.g cetus as the freetype font name. I
guess it's still valid to have it there so that you can disable freetype
fonts, but now having it enabled doesn't guarantee that freetype will be
used.

I do think that subtitles.separate.font_freetype should probably be
deprecated and xine should just use subtitles.separate.font for either
freetype or native fonts. I can almost certainly do that patch in osd.c,
but I'm not familiar with how xine does config items so I don't yet know
how I would go about removing/deprecating
subtitles.separate.font_freetype. I'll have to go and take a look at the
hacker's guide, maybe I'll find some tips there. Or if you had any
advice on where to look it would be appreciated :)

>> 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 :)
> 
> BTW, what GUI are you using ?
> 
> Both xine-ui and gxine are pretty much the same those were over 10
> years ago.
> 

I use xine-ui. I *like* that it hasn't changed in ages - "if it ain't
broke don't fix it"! The configuration window and menu toolkit (tk? or
is it pure xlib?) is a little antiquated but it gets the job done and
configuration isn't something you do often so it's not a big deal.

I like it because it's reliable and lightweight - xine-ui starts in
about 1 second on my machine but something like VLC can take 10 or 20
seconds sometimes. On one of my older machines I used to have to use
xine for playing HD because other players were too CPU-intensive. I also
like having LIRC integration and i think the controls window looks
sleek. And my friends are always impressed when I skip the copyright
warnings and ads on DVDs ;)

-Dale

>> 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