We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4346

Summary: check pixmap/bitmap width when it's cached
Product: SDL_ttf Reporter: Sylvain <sylvain.becker>
Component: miscAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2    
Version: unspecified   
Hardware: x86_64   
OS: Linux   
Attachments: patch
patch
an addition to the previous
an addition to the previous patch
patch for non scalable fonts

Description Sylvain 2018-10-31 09:34:27 UTC
This is a little improvement.

Instead of making sure that the glyph width is correct each time it is rendered, 
we can make do it only once when it's cached.
Comment 1 Sylvain 2018-10-31 09:37:05 UTC
Created attachment 3424 [details]
patch

So we update the width when it's cached.
Of course, we don't update the pitch.

Also removing parenthesis of a few returns.
Comment 2 Sylvain 2018-10-31 14:27:02 UTC
Created attachment 3425 [details]
patch

update of the patch: I forgot to update width variable of BlendedWrap function.
Comment 3 Sylvain 2018-10-31 16:48:34 UTC
Created attachment 3427 [details]
an addition to the previous

It's an addition to the previous patch.

Currently the bitmap buffer is cropped in width, but we can also crop in height/rows.
The patch add that: the bitmap will be never more than its bounding box.

Size() function only knows the bounding box to allocate the surface, so it makes sense to use it as a boundary.

And I have removed the bound checking of Render functions. At first, It almost worked and I spot another issue so I re-worked the Size function which is know much more similar in width and height.
The bitmap is blitted at (yoffset, yoffset+maxy-miny) and we clear see it in the size function.


I fixed the BlendWrap() function that was allocating an extra un-needed memory and also fix when using TTF_USE_LINESKIP (added in bug 3679). Which could be enabled by default maybe ?

I didn't remove the bound checking from underline/strikethrough as it's still possible to make it crash, with very big outline/size for instance. (I will do it probably).

I have tested it with many fonts, including FletcherGothicFLF.ttf, which seems just a normal one.
Comment 4 Sylvain 2018-10-31 20:49:43 UTC
I tried the second patch with a non-scalable font and it didn't work :(
Indeed, min/max Y metrics are filled with 0. Glyph are now clamped and so not rendered.

With an old version of SDL_ttf, it wouldn't be rendered correctly (mis aligned). So I am wondering how non-scalable should be supported / used ?
Comment 5 Sylvain 2018-10-31 21:32:11 UTC
Created attachment 3431 [details]
an addition to the previous patch

Here's a new 2nd patch.

It seems to be solved by using font->height, and max(font->height, maxy - miny) as a limit. Which doesn't exceed the surface allocated memory, since we already make sure we are greater that font->height in TTF_Size() function. 

before:
  dst->rows  = SDL_min((int)dst->rows, cached->maxy - cached->miny);

after:
  int rows_max  = SDL_max(font->height,    cached->maxy - cached->miny);
  dst->rows  = SDL_min((int)dst->rows,  rows_max);



I think I also find a reason for reverting this previous commit of bug 4344
https://hg.libsdl.org/SDL_ttf/rev/7489a6f9929b
If I had a non scalable font size with almost null minx/miny, I would use the "advance" value in the same way.

before:
  dst->width = SDL_min((int)dst->width, cached->maxx - cached->minx);

after:
  int width_max = SDL_max(cached->advance, cached->maxx - cached->minx);
  dst->width = SDL_min((int)dst->width, width_max);

This 7489a6f9929b commit was to make sure there is enough memory allocated when we use "advance" as a limit.
Comment 6 Sam Lantinga 2018-10-31 22:12:09 UTC
These patches are in:
https://hg.libsdl.org/SDL_ttf/rev/38b21453a2b4

You realize that at this point the code has entirely changed and you need to do lots and lots of validation to make sure you haven't introduced any regressions, right?
Comment 7 Sylvain 2018-11-01 12:33:04 UTC
I already tried with non-scalable font, but I really hit the empty metric issue with this previous patch. I'll do more test soon.
Comment 8 Sylvain 2018-11-02 23:08:32 UTC
Created attachment 3434 [details]
patch for non scalable fonts

Here's a patch (diff'ed with bug 4351)

I have played more with non scalable fonts and I got them working in a better way 
by selecting the size index with FT_Select_Size. Font metrics appears into face->size->metrics.
Also glyph metrics appears correct now.

In the patch:
- Fix crash when using an invalid (negative) size index. No need to save this index.
- use FT_Select_Size to select the size, and get the size from "face->size->metrics"
- it appears glyph metrics are corrects and same as for scalable fonts. merge both cases.
  => as a consequence, we only crop the pixmap up to the [minx; maxx]x[miny, maxy]

- ttf_handling of bold style now works (before y_ppem was 0).
- lineskip is fixed. (before it was FT_CEIL'ed when it wasn't a 26.6 frac pixel).
- mis aligned (bad yoffset) is now fixed
- underline and strikethrough: face->underline_offset/height were not relevant values (cf freetype doc), 
  so use arbitrary computed values.

cannot do outline and  italic with non-scalable font:
- prevent outline
- prevent ttf_handling of italic style


I've kept in TTF_Size_Internal():
  maxx = SDL_max(maxx, x + glyph->advance);
so it allows to render a string with only one space " ". (bug 4344).
Comment 9 Sylvain 2018-11-02 23:09:29 UTC
re-opening
Comment 10 Sam Lantinga 2018-11-02 23:41:36 UTC
Patch added, thanks!
https://hg.libsdl.org/SDL_ttf/rev/66fe96efd5f6