bugThe FreeType Project - Bugs: bug #28395, Incremental interface not working...

 
 

bug #28395: Incremental interface not working properly.

Submitted by:  Kenneth Sharp <kens>
Submitted on:  Thu 24 Dec 2009 01:26:52 PM UTC  
 
Severity: 3 - NormalItem Group: Incorrect behaviour
Status: FixedPrivacy: Public
Assigned to: Werner LEMBERG <wl>Open/Closed: Closed
Planned Release: 2.3.12

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 05 Jan 2010 03:40:26 PM UTC, comment #8:

Thanks Werner, that does the trick nicely and allows me to remove my hack from GS, thanks very much.

I just ran a complete test of GS using FreeType and all my files work to the extent that there are now no crashes or errors. Can't be sure yet that the rendering is entirely 'correct' but a random sampling of the output files is generally pretty good.

FWIW thats 2763 test files including broken files emitted by common applications, test files specifically designed to check boundary conditions, and files which are known to provoke 'interesting' results with PostScript rips.

So, pretty good so far, many thanks for your efforts, much appreciated!

Ken

Kenneth Sharp <kens>
Project Member
Tue 05 Jan 2010 12:39:55 PM UTC, comment #7:

Oops! Fixed, thanks.

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Tue 05 Jan 2010 10:52:09 AM UTC, comment #6:

Sorry Werner, the sense of the test for incremental is inverted. The code says (t1gload.c, line 290):

#ifdef FT_CONFIG_OPTION_INCREMENTAL
if ( glyph_index >= (FT_UInt)face->root.num_glyphs &&
face->root.internal->incremental_interface )
#else

So if we built the incremental interface the glyph index is tested only if the particular face in question is an incremental font. We actually want to perform the test only if the face is not incremental. So the code should read:

#ifdef FT_CONFIG_OPTION_INCREMENTAL
if ( glyph_index >= (FT_UInt)face->root.num_glyphs &&
!face->root.internal->incremental_interface )
#else

Ken

Kenneth Sharp <kens>
Project Member
Tue 05 Jan 2010 10:29:25 AM UTC, comment #5:

I've picked up the latest snapshot from GIT, built new libraries and rebuilt GS. I'm starting a complete test suite run which will take a couple of hours. I'll report back when I have news.

Many thanks.

Ken

Kenneth Sharp <kens>
Project Member
Tue 05 Jan 2010 09:30:33 AM UTC, comment #4:

I've applied fixes now. Please test.

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Sat 26 Dec 2009 10:36:30 AM UTC, comment #3:

Sorry for the delay in replying, Real Life interfering...

We could update the num_glyphs member, but there isn't really a whole lot of point. The callback used by the incremental interface is (as far as I can tell) the sole way to retrieve the glyph data when we are using the incremental configuration. In this case the only time the num_glyphs member is referenced is to see whether the requested glyph_index is beyond that range.

Since the callback is responsible for locating and returning the glyph data, there isn't a whole lot of point in the additional test, when using the incremental interface. Obviously its important in the regular case.

From my POV, rather than do the bookkeeping involved in locating the FT structure and updating the member every time a glyph is added to a PostScript font (whether it is used or not) I would simply set the value to 65534 when the face is created and forget about it. That just seems kludgy to me.

There's no great value in making it a compile-time rather than run-time option, other than the very minor fact that the run-time test that will use a tiny amount of processor time.

It just seemed that most of the configuration involved with the incremental interface was done at compile-time, but I'm completely happy with either.

Kenneth Sharp <kens>
Project Member
Sat 26 Dec 2009 07:03:00 AM UTC, comment #2:

Ken, Ralph's question is a good one. Besides this I wonder
whether a compile-time option is the right solution: I can
imagine that a test like

if ( glyph_index >= (FT_UInt)face->root.num_glyphs &&
face->root.internal->incremental_interface == NULL )
{
error = T1_Err_Invalid_Argument;
goto Exit;
}

is more appropriate.

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Thu 24 Dec 2009 06:27:43 PM UTC, comment #1:

Is there some reason we shouldn't be updating the num_glyphs member?

Ralph Giles <rillian>
Thu 24 Dec 2009 01:26:52 PM UTC, original submission:

This may not be strictly a bug, but related to the way Ghostscript uses FreeType.

Because PostScript files often contain incrementally downloaded Type 1 and TrueType fonts, we must use the incremental option for FreeType.

When doing so GS relies upon the callback to supply the correct glyph data to FreeType for processing. In particular Ghostscript does not alter the 'num_glyphs' member of the FreeType 'face' structure when new glyphs become available.

This means that the num_glyphs field is always at its initial value (0 for Type 1 fonts). Howdver, there is a specific test in T1_Load_Glyph (t1gload.c, ~line 290) :

if ( glyph_index >= (FT_UInt)face->root.num_glyphs )
{
error = T1_Err_Invalid_Argument;
goto Exit;
}

Because we don't update num_glyphs, this test always fails, and we get an invalid argument error return.

I would suggest that this should be guarded with a #ifndef FT_CONFIG_OPTION_INCREMENTAL preprocessor directive. This allows the incremental interface to run as expected and the callback duly returns the correct data.

By inspection there are two other locations where this may be a problem, FNT_Load_Glyph (winfnt.c, line 961) and Load_Glyph (ttdriver.c, line 301). I don't have test cases which cause these to fail, though I'm unsure at the moment why the TrueType case doesn't cause us a problem.

I've attached unified diff files of the patches I've made locally to FreeType, please let me know if you need more details.

Kenneth Sharp <kens>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #19353:  t1gload.diff added by kens (418B - application/octet-stream - proposed patches for t1gload.c, ttdriver.c and winfnt.c)
file #19354:  winfnt.diff added by kens (749B - application/octet-stream - proposed patches for t1gload.c, ttdriver.c and winfnt.c)
file #19355:  ttdriver.diff added by kens (401B - application/octet-stream - proposed patches for t1gload.c, ttdriver.c and winfnt.c)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by wl (Posted a comment)
  • -unavailable- added by rillian (Posted a comment)
  • -unavailable- added by kens (Submitted the item)
  • -unavailable- added by kens
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 05 Jan 2010 09:30:33 AM UTCwlStatusNeed Info=>Fixed
      Open/ClosedOpen=>Closed
      Planned ReleaseNone=>2.3.12
    Sat 26 Dec 2009 07:03:00 AM UTCwlStatusNone=>Need Info
      Assigned toNone=>wl
    Thu 24 Dec 2009 01:26:53 PM UTCkensAttached File-=>Added t1gload.diff, #19353
      Attached File-=>Added winfnt.diff, #19354
      Attached File-=>Added ttdriver.diff, #19355
      Carbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1