456448 - Add configure check for FT_Library_SetLcdFilter

Status: NEW

(Core :: Graphics, defect)

Description

[Sylvain Pasche]

Attachment: Check for FT_Library_SetLcdFilter, removes FT_Select_Size

Cairo uses that new unction since 1.7.4. It seems that the FT_Select_Size check is not used. Cairo's configure does not check it and I couldn't find reference to FT_Select_Size or HAVE_FT_SELECT_SIZE in the tree. Karl, can you confirm it can be removed? (introduced in bug 327879).

Comment 1

[Karl Tomlinson (:karlt)]

I'll note here that this change is not expected to be picked up in Mozilla's
current official builds, as I assume they are built against an older version of
FreeType.

FT_Library_SetLcdFilter was introduced between 2.2.1 and 2.3.0.

2007-01-17  Werner Lemberg  <[e-mail]>

        * Version 2.3.0 released.
        =========================

2006-11-10  David Turner  <[e-mail]>

        * src/smooth/ftsmooth.c: API change for the LCD
        filter.  The FT_LcdFilter value is an enumeration describing which
        filter to apply, with new values FT_LCD_FILTER_LIGHT and
        FT_LCD_FILTER_LEGACY (the latter implements the LibXft original
        algorithm which produces strong color fringes for everything
        except very-well hinted text).

        * include/freetype/ftlcdfil.h (FT_Library_SetLcdFilter): Change
        second parameter to an enum type.

        * src/base/ftlcdfil.c (USE_LEGACY): Define.
        (_ft_lcd_filter): Rename to...
        (_ft_lcd_filter_fir): This.
        Update parameters.
        (_ft_lcd_filter_legacy) [USE_LEGACY]: New filter function.
        (FT_Library_Set_LcdFilter): Update parameters.
        Handle new filter modes.

        * include/internal/ftobjs.h: Include FT_LCD_FILTER_H.
        (FT_Bitmap_LcdFilterFunc): Change third argument to `FT_Library'.
        (FT_LibraryRec) [FT_CONFIG_OPTION_SUBPIXEL_RENDERING]: Add filtering
        callback and update other fields.

2006-09-27  David Turner  <[e-mail]>

        * include/freetype/freetype.h (FT_FREETYPE_PATCH): Set to 2.

        Add a new API to support color filtering of subpixel glyph bitmaps.
        In a default build, the function `FT_Library_SetLcdFilter' returns
        `FT_Err_Unimplemented_Feature'; you need to #define
        FT_CONFIG_OPTION_SUBPIXEL_RENDERING in ftoption.h to compile the
        real implementation.

        * include/freetype/ftlcdfil.h, src/base/ftlcdfil.c: New files.

        * include/freetype/internal/ftobjs.h (FT_Bitmap_LcdFilterFunc): New
        typedef.
        (FT_LibraryRec) [FT_CONFIG_OPTION_SUBPIXEL_RENDERING]: New members
        `lcd_filter_weights' and `lcd_filter'.

        * src/smooth/ftsmooth.c (ft_smooth_render_generic): Remove arguments
        `hmul' and `vmul'.

        Handle subpixel rendering.
        Simplify function.
        (ft_smooth_render_lcd): Use `FT_RENDER_MODE_LCD'.
        (ft_smooth_render_lcd_v): Use `FT_RENDER_MODE_LCD_V'.

        * include/freetype/config/ftheader.h (FT_LCD_FILTER_H): New macro,
        pointing to <freetype/ftlcdfil.h>.

        * src/base/Jamfile (_sources), src/base/rules.mk (BASE_SRC),
        vms_make.com: Add `ftlcdfil.c' to the list of compiled source files.

        * modules.cfg (BASE_EXTENSIONS): Add ftlcdfil.c.

I think the API change is safe because the original API was not in any stable version of FreeType.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 339849 [details] [diff] [review]
Check for FT_Library_SetLcdFilter, removes FT_Select_Size

Matching the cairo configure like this is the right thing to do.
You are right that HAVE_FT_SELECT_SIZE is not needed.

Comment 3

[Karl Tomlinson (:karlt)]

http://gitweb.freedesktop.org/?p=cairo;a=commitdiff;h=a0cad6c398f5e30e3d10fb19fe51d4266b1c0b4c

@@ -1657,6 +1694,12 @@ _cairo_ft_options_merge (cairo_ft_option

     if (other->base.hint_style == CAIRO_HINT_STYLE_NONE)

         options->base.hint_style = CAIRO_HINT_STYLE_NONE;

 

+    if (options->base.lcd_filter == CAIRO_LCD_FILTER_DEFAULT)
+        options->base.lcd_filter = other->base.lcd_filter;
+
+    if (other->base.lcd_filter == CAIRO_LCD_FILTER_NONE)
+        options->base.lcd_filter = CAIRO_LCD_FILTER_NONE;
+

Assuming that surface options have already been set on the FcPattern with
cairo_ft_font_options_substitute, if fontconfig settings have changed any options on the pattern then isn't that what the user wants?
i.e. Is there a reason why other->base.lcd_filter shouldn't be preferred here
(when not CAIRO_LCD_FILTER_DEFAULT)?

https://bugs.freedesktop.org/show_bug.cgi?id=11838

Comment 4

[Sylvain Pasche]

Maybe there's no valid reason. I was not sure how to deal with this and we decided to follow the existing surrounding behavior (see http://bugs.freedesktop.org/show_bug.cgi?id=10301#c30).

Comment 5

[Sylvain Pasche]

The lcd filtering stuff in Cairo has been reverted recently (that's what was using the FT_Library_SetLcdFilter function). Cairo's configure still checks for FT_Library_SetLcdFilter, so this shouldn't impact this bug (I guess this is in anticipation of a future relanding, if not a forgotten revert). See thread on http://lists.cairographics.org/archives/cairo/2008-September/015179.html

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 339849 [details] [diff] [review]
Check for FT_Library_SetLcdFilter, removes FT_Select_Size

Hrm, let's not unnecessarily bump this req until/if this comes back into cairo.  This would break some folks currently because the ft version is fairly new that has the LcdFilter stuff, I think.

Comment 7

[Sylvain Pasche]

Fine for me. Note sure that it should break something though. The official builds would not have this enabled according to comment 1, and for distributions or people compiling it themselves it should just set the HAVE_FT_LIBRARY_SETLCDFILTER macro to 0 (or not define it) if they have an old FreeType.

Comment 8

[Micah Gersten]

It has been over a year since the last comment on this bug.  Would it be possible to accept the patch now?

Comment 9

[Sylvain Pasche]

Comment on attachment 339849 [details] [diff] [review]
Check for FT_Library_SetLcdFilter, removes FT_Select_Size

See comment 6, nothing has changed since then.

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> (From update of attachment 339849 [details] [diff] [review])
> See comment 6, nothing has changed since then.

Time has changed... October 2008 versus January 2010.

Comment 11

[Sylvain Pasche]

Heh :-) Unfortunately that change was not enough to bring back the LCD filter in Cairo.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 339849 [details] [diff] [review]
Check for FT_Library_SetLcdFilter, removes FT_Select_Size

Turns out the cairo update (bug 542605) introduces this requirement now. So let's use this!

Comment 14

[Joe Drew (not getting mail)]

Attachment: updated to build now that we actually use FT_Library_SetLcdFilter

Comment 15

[Joe Drew (not getting mail)]

Attachment: copy system-headers to js/src/config

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 536404 [details] [diff] [review]
copy system-headers to js/src/config

r=jeff over the airs

Comment 17

[Karl Tomlinson (:karlt)]

This is the right thing to do and will work in people's local builds, but FT_Library_SetLcdFilter won't be found on our build machines.

It's time we updated our build machines, but we might want a faster solution.
If we could upgrade FreeType to 2.3.x on the build machines that would work.
Does mozilla-central have a different pool of build machines to Firefox 3.6, for example?  (This will increase runtime requirements.)

CentOS 5.6 still has freetype-2.2.1-28.el5_5.1 so we'd probably need to build our own rpm.

From a developer's perspective, it may be easier to change the code to find the symbols at run-time.

Comment 19

[Mike Hommey [:glandium]]

Do we really want to break binary compatibility with older distros? (centos 5.6 being not that old, and we already had a lot of people complain that ff4 wasn't working on rh/centos 5.x, which we fixed in ff6)

Comment 20

[Timothy Nikkel (:tnikkel)]

If fonts looked this bad on Windows we would absolutely not release, we should track this.

Comment 21

[Timothy Nikkel (:tnikkel)]

We mostly worked around this problem in another way so that most (?) Linux systems are unaffected. So comment 20 is less relevant now.

Comment 22

[Johnathan Nightingale [:johnath]]

Based on comment 21, this sounds like we don't need the tracking nomination anymore, right?

Comment 23

[Joe Drew (not getting mail)]

This is the permanent fix to the bug that inspired bug 666732.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.