Open Bug 456448 Opened 16 years ago Updated 2 years ago

Add configure check for FT_Library_SetLcdFilter

Categories

(Core :: Graphics, defect)

x86
Linux
defect

Tracking

()

Tracking Status
firefox7 - ---

People

(Reporter: sylvain.pasche, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attachment #339849 - Flags: superreview?(vladimir)
Attachment #339849 - Flags: review?(mozbugz)
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 <wl@gnu.org> * Version 2.3.0 released. ========================= 2006-11-10 David Turner <david@freetype.org> * 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 <david@freetype.org> * 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 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.
Attachment #339849 - Flags: review?(mozbugz) → review+
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
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).
Blocks: 404637
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
Attachment #339849 - Flags: superreview?(vladimir) → superreview-
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.
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.
It has been over a year since the last comment on this bug. Would it be possible to accept the patch now?
Attachment #339849 - Flags: superreview- → superreview?(vladimir)
Assignee: nobody → 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.
Attachment #339849 - Attachment is obsolete: true
Attachment #339849 - Flags: superreview?(vladimir)
(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.
Heh :-) Unfortunately that change was not enough to bring back the LCD filter in Cairo.
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!
Attachment #339849 - Attachment is obsolete: false
Attachment #339849 - Attachment is obsolete: true
Attachment #536381 - Flags: review?(karlt)
Attachment #536381 - Attachment is obsolete: true
Attachment #536404 - Flags: review?(jmuizelaar)
Attachment #536381 - Flags: review?(karlt)
Comment on attachment 536404 [details] [diff] [review] copy system-headers to js/src/config r=jeff over the airs
Attachment #536404 - Flags: review?(jmuizelaar) → review+
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.
Depends on: 661306
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)
If fonts looked this bad on Windows we would absolutely not release, we should track this.
Whiteboard: nomination in comment 20
We mostly worked around this problem in another way so that most (?) Linux systems are unaffected. So comment 20 is less relevant now.
Based on comment 21, this sounds like we don't need the tracking nomination anymore, right?
Whiteboard: nomination in comment 20
This is the permanent fix to the bug that inspired bug 666732.
Depends on: 795354

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.

Assignee: sylvain.pasche → nobody
Flags: needinfo?(bhood)
Flags: needinfo?(bhood)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: