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

la, 2018-10-13 kello 09:31 +1100, Dale Magee kirjoitti:
> 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.

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

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

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

Both patches look fine.
Pushed, thanks!

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

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.

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

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

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

> 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