Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

font.save() fails when trying to overwrite a loaded font #302

Closed
miguelsousa opened this issue Jun 25, 2015 · 11 comments
Closed

font.save() fails when trying to overwrite a loaded font #302

miguelsousa opened this issue Jun 25, 2015 · 11 comments

Comments

@miguelsousa
Copy link
Collaborator

Take a look at this simple script:

from fontTools import ttLib

fontFilePath = 'MyFont.otf'
font = ttLib.TTFont(fontFilePath)

font.save(fontFilePath[:-4] + '-New.otf') # this works
font.save(fontFilePath) # XXX this produces the traceback below
Traceback (most recent call last):
  File "save.py", line 8, in <module>
    font.save(fontFilePath)
  File "/fonttools/Lib/fontTools/ttLib/__init__.py", line 214, in save
    self._writeTable(tag, writer, done)
  File "/fonttools/Lib/fontTools/ttLib/__init__.py", line 626, in _writeTable
    self._writeTable(masterTable, writer, done)
  File "/fonttools/Lib/fontTools/ttLib/__init__.py", line 629, in _writeTable
    tabledata = self.getTableData(tag)
  File "/fonttools/Lib/fontTools/ttLib/__init__.py", line 646, in getTableData
    return self.reader[tag]
  File "/fonttools/Lib/fontTools/ttLib/sfnt.py", line 92, in __getitem__
    data = entry.loadData (self.file)
  File "/fonttools/Lib/fontTools/ttLib/sfnt.py", line 373, in loadData
    assert len(data) == self.length
AssertionError

Am I doing something wrong?

@anthrotype
Copy link
Member

currently it's not possible to overwrite a TTFont, because the reader's file is already open for reading, and can't be also written from the writer at the same time.
You can work around this by first making a temporary copy of the input file and then using the latter when initializing a TTFont object.
This way you can save to the original input file, since now the reader's file is different from the writer's.
There are many ways to do that. Here's one:

import tempfile
import shutil

tf = tempfile.TemporaryFile()
with open(path, 'rb') as f:
    shutil.copyfileobj(f, tf)
tf.seek(0)

font = TTFont(tf)

@miguelsousa
Copy link
Collaborator Author

Alright, thanks for confirming and for providing a workaround.

@anthrotype
Copy link
Member

if there's demand for it, maybe we could do something like this by default upon loading a TTFont? /cc @behdad

@behdad
Copy link
Member

behdad commented Jun 25, 2015

This is definitely in the right direction. Alternatively we can just right the font into memory and wrap a stream around it. I think I like that even better.

Note that the lazy parameter to TTFont() helps a bit with this, but I never got to implement non-lazy in sfnt.py.

@anthrotype
Copy link
Member

I did that in PR #308.
I read the input file's content into a StringIO stream, which is then passed to the SFNTReader instead of the original file. I also keep a reference to the file's name attribute, in case one wants to know the name of the original file.

@behdad
Copy link
Member

behdad commented Jun 26, 2015

Thanks. The name is indeed used by the bitmaps/ code that you fixed recently.

@behdad
Copy link
Member

behdad commented Jun 26, 2015

Right now we do a lot of conversions between file-like object and actual data. Over time we can change all to keep reference to byte() only.

@behdad
Copy link
Member

behdad commented Jun 26, 2015

Another nice thing about this change is that we can now load font from unseekable streams.

@anthrotype
Copy link
Member

Cool!
I'm leaving the input file open in case it's already a file object instead of a path string.

@miguelsousa
Copy link
Collaborator Author

Nice! Thanks guys.

@twardoch
Copy link
Contributor

This is great.

behdad added a commit that referenced this issue Apr 17, 2016
As discussed here:
#580 (comment)

Before:
$ python -m timeit 'from fontTools.ttLib import TTFont; TTFont("sazanami-gothic.ttf")'
10 loops, best of 3: 66.9 msec per loop

After:
$ python -m timeit 'from fontTools.ttLib import TTFont; TTFont("sazanami-gothic.ttf")'
10000 loops, best of 3: 110 usec per loop

That's a 600x speedup!

Fixes #482

HOWEVER, it reintroduces #302
Or worse, we'll crash when overwriting:

$ cp Lobster.ttf t.ttf
$ ./ttx -o ./t.ttf ./t.ttf
Dumping "./t.ttf" to "./t.ttf"...
Dumping 'GlyphOrder' table...
Bus error (core dumped)

IMO we should fix this by changing both XML and font output routines to,
instead of calling open, use os.tmpfile(), create output in a new file
and then rename it to the final destination.  It has the benefit of not
leaving a half-written output file behind if an exception occurs.  The
tempfile.NamedTemporaryFile also comes handy.

While checking those out, tempfile.SpooledTemporaryFile also comes handy,
when it's available, to replace BytesIO in the following part of TTFont.save():

	# write to a temporary stream to allow saving to unseekable streams
	tmp = BytesIO()

Looks like we need to start misc.fileTools to abstract the details away.
brawer pushed a commit that referenced this issue Feb 13, 2017
As discussed here:
#580 (comment)

Before:
$ python -m timeit 'from fontTools.ttLib import TTFont; TTFont("sazanami-gothic.ttf")'
10 loops, best of 3: 66.9 msec per loop

After:
$ python -m timeit 'from fontTools.ttLib import TTFont; TTFont("sazanami-gothic.ttf")'
10000 loops, best of 3: 110 usec per loop

That's a 600x speedup!

Fixes #482

HOWEVER, it reintroduces #302
Or worse, we'll crash when overwriting:

$ cp Lobster.ttf t.ttf
$ ./ttx -o ./t.ttf ./t.ttf
Dumping "./t.ttf" to "./t.ttf"...
Dumping 'GlyphOrder' table...
Bus error (core dumped)

IMO we should fix this by changing both XML and font output routines to,
instead of calling open, use os.tmpfile(), create output in a new file
and then rename it to the final destination.  It has the benefit of not
leaving a half-written output file behind if an exception occurs.  The
tempfile.NamedTemporaryFile also comes handy.

While checking those out, tempfile.SpooledTemporaryFile also comes handy,
when it's available, to replace BytesIO in the following part of TTFont.save():

	# write to a temporary stream to allow saving to unseekable streams
	tmp = BytesIO()

Looks like we need to start misc.fileTools to abstract the details away.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants