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

[WIP] OpenType Support #20

Merged
merged 49 commits into from
Oct 31, 2018
Merged

[WIP] OpenType Support #20

merged 49 commits into from
Oct 31, 2018

Conversation

shermp
Copy link
Contributor

@shermp shermp commented Oct 26, 2018

Hi NiLuJe

I decided to see if I could figure out this whole OpenType/TrueType thing...

I'm opening this pull request relatively early, so I hopefully don't go too far down the wrong track if I've done stuff you don't agree with and/or want changed. Consider this Not Done Yet.

With that out the way, here's what's working:

  • Printing strings to screen in arbitrary font sizes (I'm using points)
  • Line breaking using the libunibreaks library, which implements the Unicode UAX 14 algorithm. Linebreaking honors hard line breaks (line feeds etc).
  • Users can specify arbitrary margins to set the maximum printable area. Currently using percentages, although I'm mulling the idea of moving to pixels directly.
  • Centered text Oops, this seems a bit broken if I have linebreaks in there.

What's not implemented:

  • Overlays, foreground/background colors
  • Vertical centering
  • Probably others.

I've done away with the rows/columns concept, as it doesn't make much sense when using proportional fonts. I've also currently created a new struct for the OT settings. We may wish to integrate these to the main FBInkConfig struct.

Nothing has been optimized yet. We may want to look at caching glyph bitmaps for example (although, that will increase memory usage).

I also haven't handled any rotation shenanigans, I'm not sure if what I've done requires this.

I look forward to getting your feedback on this. No rush though, you are allowed to sleep!

(Also, if I may make an observation on the current printing code? The rows/columns concept seems to be more trouble than its worth IMHO)

@miiPoP
Copy link

miiPoP commented Oct 26, 2018

Yay!

@shermp
Copy link
Contributor Author

shermp commented Oct 26, 2018

Note, I decided to go the stb_truetype route rather than try and use freetype2. Means we don't have to try and link against freetype (or figure out how to statically compile it), and FBInk already uses other STB libraries, so it was a simple matter of #include "stb/stb_truetype.h".

Quality seems acceptable, probably not as nice as freetype2 would be, but good enough.

Still have to think about how we handle font loading. Current implementation is for the developer to load the font into memory and pass it to FBInk. It might be nicer if FBInk could accept a font file path.

EDIT: And also another thought, which I've got as a TODO in the code, but I'll mention here. We need to figure out how to handle the DPI/PPI value. It is used to calculate the font height in pixels from the size provided in points. The value is currently hard coded for the Aura H2O.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

First of all, thanks, this looks fantastic!

So, in no particular order:

  • Yeah, the whole row/col thing is hellishly messy, so I won't fault anyone for not sticking with it, even were it to make more sense than it actually does with proportional fonts ;). I definitely get that, not an issue here ;).

It made sense for cell-based rendering, both because that's what eips does on Kindle, so I'm used to its quirks, and because it was relatively tame at the beginning, but when you start piling stuff on top of it (centering, dumb line-breaking, whatever, ...), it quickly becomes a giant mess :D.

And even for a user, I get that it might sound a bit opaque to use at first (again, used to eips here ;p).


  • Handling position/layout concerns through a simple "margins" concept sounds like a neat & tidy solution, but, yeah, being able to position this in a pixel-perfect manner might be desirable (i.e., +1 on pixel values ;)).
    I do like the ease of use of percentages, though!

  • Clearly not opposed to a high-level font loading function ;). I'd be inclined to go with filepath over stdio, but that's a rather minor concern in my book.

  • As far as rotation is concerned, put_pixel should handle everything automagically, so you'll just need to pass your region to fxpRotateRegion before the refresh call to cinch the deal (NOTE: the switch to a function pointer for this is a rather new development [as in, err, a few days ago], just so you don't get confused if you see calls to a real rotate_region() function in your WD instead).

  • I didn't catch any other use of it, so I'd just fold the INVERT macro into an on-site comment and a ^ 0xFF, if only to match what the blitting code already does.

  • Regarding DPI, the least terrible way I can think of is adding a deviceQuirk field, and populating that in device_id. We should have a more-or-less accurate db of this info in KOReader.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

And as far a glyph caching is concerned, I'm guessing it'll all depend on use-cases and real-world performance.

As a vaguely related data point, story time:

In the cell renderer, I was originally decoding the font data, then scaling it into a buffer, then rendering that scaled buffer.
Folding the scaling into the rendering pass (and getting rid of a buffer & copy in the process) netted me a noticeable performance increase.


TL;DR: I'm not sure the type of use-cases this'll likely get warrant the effort for a glyph cache, which might make it a net loss instead of a gain.

Keep in mind that this could barely even be called a gut feeling, so, if you want to try stuff in that realm, go ahead ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

I kind of appreciate the OT struct being on its own, as FBInkConfig is already starting to look like a giant mess ;).
Again, no strong feelings on that one, though.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

And I definitely get the STB choice, that'd probably have been my first choice since the image loading code, too ;).

(Although I am rather well versed in compiling FT :D).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

And a final comment in the "not to forget when you're done" box: adding a libunibreak mention to the CREDITS file ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 26, 2018

Hey, thanks for the feedback!

My only concern with passing font paths instead of an already loaded buffer is that I will need to add a function like `fbink_close_ot() to free the allocated memory. On the other hand, it would remove any ambiguity.

I'm leaning towards changing the margins to pixels as well. That means it becomes meaningful to return the Y position of the next row of text that callers can set on the next print call.

Thanks for the heads up on the macro. I never was that fond of it, but my bitwise-fu isn't always the greatest!

As to glyph caching, I'm really not certain. Generating them is certainly more intensive than a simple integer scaling however, as there's floating point math and anti-aliasing going on. On the other hand, even with the current method, text is basically appearing on the screen instantly so...

@shermp
Copy link
Contributor Author

shermp commented Oct 26, 2018

Hmm, maybe we need a "get viewinfo" function to return basic resolution info (without requiring getting the full state)? Because if I allow the user to specify margins as pixels, they probably would like to know how many pixels are available first!

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 26, 2018

Yep, having an open/close pair would probably make things more obvious :).

I'm not opposed to a specific getter for the resolution and/or viewport, but how were you envisioning it: returning a new struct that only contains a subset of the full state, or setting uint pointers?
Or something else? ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 26, 2018

Either method could work I suppose. I've gotten used to setting pointers with the STB libraries.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Yeah, that sounds like the least intrusive way to handle that ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 27, 2018

So... um... erm.... I have a confession to make.

I kind of went and added font variant support (bold, italics, bold italics), as well as a very rudimentary "markdown style" parser.

Ummm... Oops? :D

fontvariants

I'm pushing what I've done so far. Fair warning though, there are Problems. Namely segfaults. But only with certain combinations and lengths of text. In other words, I probably need to return to school, because I'm obviously having trouble with my sums. :p

If you spot any obvious errors, please let me know. My brain has turned to mush...

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

:D

I'll try to take a look at it this afternoon, with my old friend gdb if need be ;).

(If you have a test string that crashes reliably, that might help, although I'm guessing it'll crash somewhere in the md parsing stage ;))

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Speaking of crashes, it also crashes with an empty string ;).

In the render pass:

c = u8_nextchar(string, &ci);
stbtt_GetCodepointHMetrics(curr_font, c, &adv, &lsb);
Max LW: 1080
Line # 0

Program received signal SIGSEGV, Segmentation fault.
stbtt_FindGlyphIndex (info=info@entry=0x0, unicode_codepoint=83, unicode_codepoint@entry=802) at stb/stb_truetype.h:1447
1447    stb/stb_truetype.h: No such file or directory.
(gdb) bt full
#0  stbtt_FindGlyphIndex (info=info@entry=0x0, unicode_codepoint=83, unicode_codepoint@entry=802) at stb/stb_truetype.h:1447
        data = 0x0
        index_map = 1080
        format = <optimized out>
        __PRETTY_FUNCTION__ = "stbtt_FindGlyphIndex"
#1  0x000298ec in stbtt_GetCodepointHMetrics (info=0x0, info@entry=0xffffff3a, codepoint=codepoint@entry=802, advanceWidth=0x7e8f6f60, advanceWidth@entry=0x7e8f6fcc, leftSideBearing=0x7e8f6f5c, leftSideBearing@entry=0x7e8f6fc8) at stb/stb_truetype.h:2559
No locals.
#2  0x0002e176 in fbink_print_ot (fbfd=<optimized out>, fbfd@entry=296, string=0x3d343958 <error: Cannot access memory at address 0x3d343958>, string@entry=0x0, cfg=cfg@entry=0x7e8f70e0) at fbink.c:3107
        ci = 13
        color = {r = 110 'n', g = 0 '\000', b = 0 '\000'}
        region = {top = 124, left = 717192, width = 0, height = 3680}
        keep_fd = true
        rv = 0
        lines = 0xffffff3a
        brk_buff = 0x2ac33000 "\020\257\016"
        fmt_buff = 0x0
        line_buff = 0x322 <error: Cannot access memory at address 0x322>
        glyph_buff = 0x3d343958 <error: Cannot access memory at address 0x3d343958>
        area = {tl = {x = 0, y = 71}, br = {x = 2688, y = 22242}}
        size_pt = <optimized out>
        ppi = 265
        font_size_px = 44
        curr_font = 0xffffff3a
        max_height = <optimized out>
        rgMetrics = {sf = 0.0439999998, asc = 802, desc = -198, lg = 0, baseline = 35}
        itMetrics = {sf = 0.0439999998, asc = 802, desc = -198, lg = 0, baseline = 35}
        bdMetrics = {sf = 0.0439999998, asc = 802, desc = -198, lg = 0, baseline = 35}
        bditMetrics = {sf = 0.0439999998, asc = 802, desc = -198, lg = 0, baseline = 35}
        num_lines = <optimized out>
        str_len_bytes = <optimized out>
        sf = 0
        baseline = 0
        lg = 717192
        chars_in_str = <optimized out>
        c_index = 4294967295
--Type <RET> for more, q to quit, c to continue without paging--
        tmp_c_index = 0
        c = <optimized out>
        max_lw = 0
        line = 3
        adv = 3
        lsb = 717192
        curr_x = <optimized out>
        curr_point = {x = 0, y = 0}
        ins_point = <optimized out>
        paint_point = {x = 0, y = 71}
        x0 = 134784
        y0 = 717434880
        x1 = 124
        y1 = 717436864
        gw = <optimized out>
        gh = <optimized out>
        lw = 0
        tmp_c = <optimized out>
        lnPtr = <optimized out>
        glPtr = <optimized out>
        start_x = <optimized out>
        start_y = <optimized out>
        invert = 255 '\377'
#3  0x00011fac in main (argc=2, argv=0x7e8f7254) at fbink_cmd.c:701
        linecount = -1
        cfg = {size_pt = 0 '\000', margins = {top = 5 '\005', bottom = 0 '\000', left = 0 '\000', right = 0 '\000'}, is_centered = false, is_formatted = false}
        total_lines = 0
        opt = <optimized out>
        opt_index = 296
        opts = {{name = 0x33c6c "row", has_arg = 1, flag = 0x0, val = 121}, {name = 0x33c70 "col", has_arg = 1, flag = 0x0, val = 120}, {name = 0x33c74 "voffset", has_arg = 1, flag = 0x0, val = 89}, {name = 0x33c7c "hoffset", has_arg = 1, flag = 0x0, val = 88}, {
            name = 0x33c84 "invert", has_arg = 0, flag = 0x0, val = 104}, {name = 0x33c8c "flash", has_arg = 0, flag = 0x0, val = 102}, {name = 0x33c94 "clear", has_arg = 0, flag = 0x0, val = 99}, {name = 0x33c9c "centered", has_arg = 0, flag = 0x0, val = 109}, {
            name = 0x33ca8 "halfway", has_arg = 0, flag = 0x0, val = 77}, {name = 0x33cb0 "padded", has_arg = 0, flag = 0x0, val = 112}, {name = 0x33cb8 "refresh", has_arg = 1, flag = 0x0, val = 115}, {name = 0x33cc0 "size", has_arg = 1, flag = 0x0, val = 83}, {
            name = 0x33cc8 "font", has_arg = 1, flag = 0x0, val = 70}, {name = 0x33cd0 "verbose", has_arg = 0, flag = 0x0, val = 118}, {name = 0x33cd8 "quiet", has_arg = 0, flag = 0x0, val = 113}, {name = 0x33ce0 "image", has_arg = 1, flag = 0x0, val = 103}, {
            name = 0x33ce8 "img", has_arg = 1, flag = 0x0, val = 105}, {name = 0x33cec "flatten", has_arg = 0, flag = 0x0, val = 97}, {name = 0x33cf4 "eval", has_arg = 0, flag = 0x0, val = 101}, {name = 0x33cfc "interactive", has_arg = 0, flag = 0x0, val = 73}, {
            name = 0x33d08 "color", has_arg = 1, flag = 0x0, val = 67}, {name = 0x33d10 "background", has_arg = 1, flag = 0x0, val = 66}, {name = 0x33d1c "linecountcode", has_arg = 0, flag = 0x0, val = 76}, {name = 0x33d2c "linecount", has_arg = 0, flag = 0x0, 
--Type <RET> for more, q to quit, c to continue without paging--
            val = 108}, {name = 0x33d38 "progressbar", has_arg = 1, flag = 0x0, val = 80}, {name = 0x33d44 "activitybar", has_arg = 1, flag = 0x0, val = 65}, {name = 0x33d50 "noviewport", has_arg = 0, flag = 0x0, val = 86}, {name = 0x33d5c "overlay", has_arg = 0, 
            flag = 0x0, val = 111}, {name = 0x33d64 "bgless", has_arg = 0, flag = 0x0, val = 79}, {name = 0x33d6c "fgless", has_arg = 0, flag = 0x0, val = 84}, {name = 0x0, has_arg = 0, flag = 0x0, val = 0}}
        fbink_config = {row = 0, col = 0, fontmult = 0 '\000', fontname = 0 '\000', is_inverted = false, is_flashing = false, is_cleared = false, is_centered = false, hoffset = 0, voffset = 0, is_halfway = false, is_padded = false, fg_color = 0 '\000', 
          bg_color = 0 '\000', is_overlay = false, is_bgless = false, is_fgless = false, no_viewport = false, is_verbose = false, is_quiet = false, ignore_alpha = false, halign = 0 '\000', valign = 0 '\000'}
        refresh_token = {0x92aa8 "top", 0x33d94 "left", 0x33d9c "width", 0x33da4 "height", 0x33dac "wfm", 0x0}
        image_token = {0x33d74 "file", 0x33d7c "x", 0x33d80 "y", 0x33d84 "halign", 0x33d8c "valign", 0x0}
        subopts = 0x2aad2fb4 "\374\377\377o\204\006"
        value = 0x2aad2fbc "\375\377\377o\003"
        region_top = <optimized out>
        region_left = <optimized out>
        region_width = <optimized out>
        region_height = <optimized out>
        region_wfm = <optimized out>
        is_refresh = 164
        image_file = 0x0
        image_x_offset = <optimized out>
        image_y_offset = <optimized out>
        is_image = 156
        is_eval = 140
        is_interactive = false
        want_linecode = false
        want_linecount = false
        is_progressbar = 148
        is_activitybar = 168
        is_infinite = 132
        progress = <optimized out>
        errfnd = <optimized out>
        rv = 0
        fbfd = 296
        string = 0x0

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

FWIW, I'm testing with a quick'n dirty piggyback in fbink_cmd:

diff --git a/fbink_cmd.c b/fbink_cmd.c
index fabf953..c2a0193 100644
--- a/fbink_cmd.c
+++ b/fbink_cmd.c
@@ -690,11 +690,23 @@ int
                                    fbink_config.fontname,
                                    fbink_config.fontmult);
                        }
+                       // TTF!
+                       FBInkOTConfig cfg = { 0 };
+                       //cfg.is_formatted = true;
+                       cfg.margins.top = 5;
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Regular.ttf", FNT_REGULAR);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Italic.ttf", FNT_ITALIC);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Bold.ttf", FNT_BOLD);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-BoldItalic.ttf", FNT_BOLD_ITALIC);
+                       fbink_print_ot(fbfd, string, &cfg);
+                       fbink_free_ot_fonts();
+                       /*
                        if ((linecount = fbink_print(fbfd, string, &fbink_config)) < 0) {
                                fprintf(stderr, "Failed to print that string!\n");
                                rv = ERRCODE(EXIT_FAILURE);
                                goto cleanup;
                        }
+                       */
                        // NOTE: Don't clobber previous entries if multiple strings were passed...
                        //       We make sure to trust print's return value,
                        //       because it knows how much space it already took up ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Haven't managed to break anything else after a few quick tests ;).

diff --git a/fbink.c b/fbink.c
index 36f32eb..7d563ea 100644
--- a/fbink.c
+++ b/fbink.c
@@ -2114,8 +2114,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,38 +2712,24 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
-                               break;
-                       }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
                case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
+                       // FIXME: _ handling is problematic, because it'll catch in-word underscores when it shouldn't,
+                       //          since there's no closing tag ;).
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2902,7 +2888,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2907,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;

So, basically, just making sure the result buffer is NULL-terminated, and avoiding code duplication between */_ handling.
With an obvious FIXME for _ (which also applies to basically any kind of unbalanced tag, even if the underscore case is probably the only one that can legitimately happen outside of md snafus).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

And with a pretty crappy workaround to the _ thing:

diff --git a/fbink.c b/fbink.c
index 36f32eb..1d2ecfc 100644
--- a/fbink.c
+++ b/fbink.c
@@ -2114,8 +2114,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,17 +2712,22 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
+               case '_':
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2737,25 +2742,10 @@ void
                                ci += 2;
                                break;
                        }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
-               case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
+                       // Try to avoid flagging a single underscore in the middle of a word.
+                       if (ch == '_' && ci > 0 && string[ci - 1] != ' ' && string[ci + 1] != ' ' && string[ci - 1] != ch && string[ci + 1] != ch) {
+                               result[ci] = CH_REGULAR;
+                               ci++;
                                break;
                        }
                        is_italic = !is_italic;
@@ -2902,7 +2892,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2911,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Given stbtt's native bitmap format, we can get inversion pretty easily, so, here goes:

diff --git a/fbink.c b/fbink.c
index 36f32eb..6b2e4dc 100644
--- a/fbink.c
+++ b/fbink.c
@@ -2114,8 +2114,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,17 +2712,22 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
+               case '_':
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2737,25 +2742,10 @@ void
                                ci += 2;
                                break;
                        }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
-               case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
+                       // Try to avoid flagging a single underscore in the middle of a word.
+                       if (ch == '_' && ci > 0 && string[ci - 1] != ' ' && string[ci + 1] != ' ' && string[ci - 1] != ch && string[ci + 1] != ch) {
+                               result[ci] = CH_REGULAR;
+                               ci++;
                                break;
                        }
                        is_italic = !is_italic;
@@ -2902,7 +2892,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2911,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;
@@ -3064,7 +3054,7 @@ int
        unsigned char *lnPtr, *glPtr = NULL;
        unsigned short start_x, start_y;
        // stb_truetype renders glyphs with color inverted to what our blitting functions expect
-       unsigned char invert = 0xff;
+       unsigned char invert = cfg->is_inverted ? 0x00 : 0xFF;
        // Render!
        for (line = 0; lines[line].line_used; line++) {
                printf("Line # %d\n", line);
diff --git a/fbink.h b/fbink.h
index 3777e19..8dbe3d0 100644
--- a/fbink.h
+++ b/fbink.h
@@ -198,6 +198,7 @@ typedef struct {
        } margins;
        bool is_centered;         // Horizontal text centering
        bool is_formatted;                // Is string "formatted"? Bold/Italic support only, markdown like syntax
+       bool is_inverted;
 } FBInkOTConfig;
 
 // NOTE: Unless otherwise specified,
@@ -271,7 +272,7 @@ FBINK_API int fbink_printf(int fbfd, const FBInkConfig* fbink_config, const char
     __attribute__((format(printf, 3, 4)));
 
 // Print a string using an OpenType font. Note the caller MUST init with fbink_init_ot() FIRST.
-// This function uses margins (as whole number percentages) instead of rows/columns for 
+// This function uses margins (as whole number percentages) instead of rows/columns for
 // positioning and setting the printable area.
 // Returns -(ERANGE) if the provided margins are out of range, or sum to < 100%
 // Returns -(ENOSYS) if compiled with MINIMAL
diff --git a/fbink_cmd.c b/fbink_cmd.c
index fabf953..6eb5df1 100644
--- a/fbink_cmd.c
+++ b/fbink_cmd.c
@@ -690,11 +690,23 @@ int
                                    fbink_config.fontname,
                                    fbink_config.fontmult);
                        }
+                       // TTF!
+                       FBInkOTConfig cfg = { 0 };
+                       cfg.is_formatted = true;
+                       cfg.margins.top = 5;
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Regular.ttf", FNT_REGULAR);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Italic.ttf", FNT_ITALIC);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Bold.ttf", FNT_BOLD);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-BoldItalic.ttf", FNT_BOLD_ITALIC);
+                       fbink_print_ot(fbfd, string, &cfg);
+                       fbink_free_ot_fonts();
+                       /*
                        if ((linecount = fbink_print(fbfd, string, &fbink_config)) < 0) {
                                fprintf(stderr, "Failed to print that string!\n");
                                rv = ERRCODE(EXIT_FAILURE);
                                goto cleanup;
                        }
+                       */
                        // NOTE: Don't clobber previous entries if multiple strings were passed...
                        //       We make sure to trust print's return value,
                        //       because it knows how much space it already took up ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

And that's it for now ;).

I you have a few test strings that crashes, I'm happy to try that on my end. (I'll probably need to know the font/sizes/margins you were using, though).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Okay, I lied :D.

Now with added crappy empty string check!

diff --git a/fbink.c b/fbink.c
index 36f32eb..d412f35 100644
--- a/fbink.c
+++ b/fbink.c
@@ -2114,8 +2114,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,17 +2712,22 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
+               case '_':
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2737,25 +2742,10 @@ void
                                ci += 2;
                                break;
                        }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
-               case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
+                       // Try to avoid flagging a single underscore in the middle of a word.
+                       if (ch == '_' && ci > 0 && string[ci - 1] != ' ' && string[ci + 1] != ' ' && string[ci - 1] != ch && string[ci + 1] != ch) {
+                               result[ci] = CH_REGULAR;
+                               ci++;
                                break;
                        }
                        is_italic = !is_italic;
@@ -2788,6 +2778,11 @@ int
 #      pragma GCC diagnostic ignored "-Wconversion"
 #      pragma GCC diagnostic ignored "-Wbad-function-cast"
 
+       // Abort if we were passed an empty string
+       if (string[0] == '\0') {
+               return ERRCODE(EXIT_FAILURE);
+       }
+
        // Has fbink_init_ot() been called yet?
        if (!otInit) {
                return ERRCODE(ENODATA);
@@ -2902,7 +2897,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2916,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;
@@ -3064,7 +3059,7 @@ int
        unsigned char *lnPtr, *glPtr = NULL;
        unsigned short start_x, start_y;
        // stb_truetype renders glyphs with color inverted to what our blitting functions expect
-       unsigned char invert = 0xff;
+       unsigned char invert = cfg->is_inverted ? 0x00 : 0xFF;
        // Render!
        for (line = 0; lines[line].line_used; line++) {
                printf("Line # %d\n", line);
diff --git a/fbink.h b/fbink.h
index 3777e19..8dbe3d0 100644
--- a/fbink.h
+++ b/fbink.h
@@ -198,6 +198,7 @@ typedef struct {
        } margins;
        bool is_centered;         // Horizontal text centering
        bool is_formatted;                // Is string "formatted"? Bold/Italic support only, markdown like syntax
+       bool is_inverted;
 } FBInkOTConfig;
 
 // NOTE: Unless otherwise specified,
@@ -271,7 +272,7 @@ FBINK_API int fbink_printf(int fbfd, const FBInkConfig* fbink_config, const char
     __attribute__((format(printf, 3, 4)));
 
 // Print a string using an OpenType font. Note the caller MUST init with fbink_init_ot() FIRST.
-// This function uses margins (as whole number percentages) instead of rows/columns for 
+// This function uses margins (as whole number percentages) instead of rows/columns for
 // positioning and setting the printable area.
 // Returns -(ERANGE) if the provided margins are out of range, or sum to < 100%
 // Returns -(ENOSYS) if compiled with MINIMAL
diff --git a/fbink_cmd.c b/fbink_cmd.c
index fabf953..6eb5df1 100644
--- a/fbink_cmd.c
+++ b/fbink_cmd.c
@@ -690,11 +690,23 @@ int
                                    fbink_config.fontname,
                                    fbink_config.fontmult);
                        }
+                       // TTF!
+                       FBInkOTConfig cfg = { 0 };
+                       cfg.is_formatted = true;
+                       cfg.margins.top = 5;
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Regular.ttf", FNT_REGULAR);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Italic.ttf", FNT_ITALIC);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Bold.ttf", FNT_BOLD);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-BoldItalic.ttf", FNT_BOLD_ITALIC);
+                       fbink_print_ot(fbfd, string, &cfg);
+                       fbink_free_ot_fonts();
+                       /*
                        if ((linecount = fbink_print(fbfd, string, &fbink_config)) < 0) {
                                fprintf(stderr, "Failed to print that string!\n");
                                rv = ERRCODE(EXIT_FAILURE);
                                goto cleanup;
                        }
+                       */
                        // NOTE: Don't clobber previous entries if multiple strings were passed...
                        //       We make sure to trust print's return value,
                        //       because it knows how much space it already took up ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

(I refrained from using a pointery syntax for that check, because I find it (if (! *string)) slightly more obscure ;)).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

The extra != ch tests in my crappy underscore workaround might actually be superfluous...

EDIT: Yeah, probably.

diff --git a/fbink.c b/fbink.c
index 36f32eb..7bf4431 100644
--- a/fbink.c
+++ b/fbink.c
@@ -2114,8 +2114,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,17 +2712,22 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
+               case '_':
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2737,25 +2742,10 @@ void
                                ci += 2;
                                break;
                        }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
-               case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
+                       // Try to avoid flagging a single underscore in the middle of a word.
+                       if (ch == '_' && ci > 0 && string[ci - 1] != ' ' && string[ci + 1] != ' ') {
+                               result[ci] = CH_REGULAR;
+                               ci++;
                                break;
                        }
                        is_italic = !is_italic;
@@ -2788,6 +2778,11 @@ int
 #      pragma GCC diagnostic ignored "-Wconversion"
 #      pragma GCC diagnostic ignored "-Wbad-function-cast"
 
+       // Abort if we were passed an empty string
+       if (! *string) {
+               return ERRCODE(EXIT_FAILURE);
+       }
+
        // Has fbink_init_ot() been called yet?
        if (!otInit) {
                return ERRCODE(ENODATA);
@@ -2902,7 +2897,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2916,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;
@@ -3064,7 +3059,7 @@ int
        unsigned char *lnPtr, *glPtr = NULL;
        unsigned short start_x, start_y;
        // stb_truetype renders glyphs with color inverted to what our blitting functions expect
-       unsigned char invert = 0xff;
+       unsigned char invert = cfg->is_inverted ? 0x00 : 0xFF;
        // Render!
        for (line = 0; lines[line].line_used; line++) {
                printf("Line # %d\n", line);
diff --git a/fbink.h b/fbink.h
index 3777e19..8dbe3d0 100644
--- a/fbink.h
+++ b/fbink.h
@@ -198,6 +198,7 @@ typedef struct {
        } margins;
        bool is_centered;         // Horizontal text centering
        bool is_formatted;                // Is string "formatted"? Bold/Italic support only, markdown like syntax
+       bool is_inverted;
 } FBInkOTConfig;
 
 // NOTE: Unless otherwise specified,
@@ -271,7 +272,7 @@ FBINK_API int fbink_printf(int fbfd, const FBInkConfig* fbink_config, const char
     __attribute__((format(printf, 3, 4)));
 
 // Print a string using an OpenType font. Note the caller MUST init with fbink_init_ot() FIRST.
-// This function uses margins (as whole number percentages) instead of rows/columns for 
+// This function uses margins (as whole number percentages) instead of rows/columns for
 // positioning and setting the printable area.
 // Returns -(ERANGE) if the provided margins are out of range, or sum to < 100%
 // Returns -(ENOSYS) if compiled with MINIMAL
diff --git a/fbink_cmd.c b/fbink_cmd.c
index fabf953..6eb5df1 100644
--- a/fbink_cmd.c
+++ b/fbink_cmd.c
@@ -690,11 +690,23 @@ int
                                    fbink_config.fontname,
                                    fbink_config.fontmult);
                        }
+                       // TTF!
+                       FBInkOTConfig cfg = { 0 };
+                       cfg.is_formatted = true;
+                       cfg.margins.top = 5;
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Regular.ttf", FNT_REGULAR);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Italic.ttf", FNT_ITALIC);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Bold.ttf", FNT_BOLD);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-BoldItalic.ttf", FNT_BOLD_ITALIC);
+                       fbink_print_ot(fbfd, string, &cfg);
+                       fbink_free_ot_fonts();
+                       /*
                        if ((linecount = fbink_print(fbfd, string, &fbink_config)) < 0) {
                                fprintf(stderr, "Failed to print that string!\n");
                                rv = ERRCODE(EXIT_FAILURE);
                                goto cleanup;
                        }
+                       */
                        // NOTE: Don't clobber previous entries if multiple strings were passed...
                        //       We make sure to trust print's return value,
                        //       because it knows how much space it already took up ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

And with a private stbtt implementation, which may allow GCC some more optimization opportunities...

diff --git a/fbink.c b/fbink.c
index 36f32eb..7439800 100644
--- a/fbink.c
+++ b/fbink.c
@@ -66,6 +66,8 @@
 // stb_truetype needs maths, and so do we to round to the nearest pixel
 #      include <math.h>
 #      define STB_TRUETYPE_IMPLEMENTATION
+// Make it private, we don't need it anywhere else
+#      define STBTT_STATIC
 // stb_truetype is.... noisy
 #      pragma GCC diagnostic push
 #      pragma GCC diagnostic ignored "-Wunknown-pragmas"
@@ -2114,8 +2116,8 @@ int
 }
 
 // Free an individual OpenType font structure
-void* 
-       free_ot_font(stbtt_fontinfo* font_info) 
+void*
+       free_ot_font(stbtt_fontinfo* font_info)
 {
        if (font_info) {
                free(font_info->data); // This is the font data we loaded
@@ -2712,17 +2714,22 @@ int
 // This is **bold** text.
 // This is ***bold italic*** text.
 // As well as their underscore equivalents
-void 
+void
        parse_simple_md(char* string, int size, unsigned char* result)
 {
        int ci = 0;
+       char ch;
        bool is_italic = false;
        bool is_bold = false;
        while (ci < size) {
-               switch (string[ci]) {
+               printf("ci: %d (< %d) is %c\n", ci, size, string[ci]);
+               switch (ch = string[ci]) {
                case '*':
-                       if (ci + 1 < size && string[ci + 1] == '*') {
-                               if (ci + 2 < size && string[ci + 2] == '*') {
+               case '_':
+                       if (ci + 1 < size && string[ci + 1] == ch) {
+                               printf("ci: %d && ci + 1 == %c\n", ci, ch);
+                               if (ci + 2 < size && string[ci + 2] == ch) {
+                                       printf("ci: %d && ci + 2 == %c\n", ci, ch);
                                        is_bold = !is_bold;
                                        is_italic = !is_italic;
                                        result[ci] = CH_IGNORE;
@@ -2737,25 +2744,10 @@ void
                                ci += 2;
                                break;
                        }
-                       is_italic = !is_italic;
-                       result[ci] = CH_IGNORE;
-                       ci++;
-                       break;
-               case '_':
-                       if (ci + 1 < size && string[ci + 1] == '_') {
-                               if (ci + 2 < size && string[ci + 2] == '_') {
-                                       is_bold = !is_bold;
-                                       is_italic = !is_italic;
-                                       result[ci] = CH_IGNORE;
-                                       result[ci + 1] = CH_IGNORE;
-                                       result[ci + 2] = CH_IGNORE;
-                                       ci += 3;
-                                       break;
-                               }
-                               is_bold = !is_bold;
-                               result[ci] = CH_IGNORE;
-                               result[ci + 1] = CH_IGNORE;
-                               ci += 2;
+                       // Try to avoid flagging a single underscore in the middle of a word.
+                       if (ch == '_' && ci > 0 && string[ci - 1] != ' ' && string[ci + 1] != ' ') {
+                               result[ci] = CH_REGULAR;
+                               ci++;
                                break;
                        }
                        is_italic = !is_italic;
@@ -2788,6 +2780,11 @@ int
 #      pragma GCC diagnostic ignored "-Wconversion"
 #      pragma GCC diagnostic ignored "-Wbad-function-cast"
 
+       // Abort if we were passed an empty string
+       if (! *string) {
+               return ERRCODE(EXIT_FAILURE);
+       }
+
        // Has fbink_init_ot() been called yet?
        if (!otInit) {
                return ERRCODE(ENODATA);
@@ -2902,7 +2899,7 @@ int
        }
        // Calculate the maximum number of lines we may have to deal with
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
-
+
        // And allocate the memory for it...
        lines = calloc(num_lines, sizeof(FBInkOTLine));
 
@@ -2921,7 +2918,7 @@ int
        LOG("Found linebreaks!");
 
        // Parse our string for formatting, if requested
-       fmt_buff = calloc(str_len_bytes, sizeof(char));
+       fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
        if (!fmt_buff) {
                rv = ERRCODE(EXIT_FAILURE);
                goto cleanup;
@@ -3064,7 +3061,7 @@ int
        unsigned char *lnPtr, *glPtr = NULL;
        unsigned short start_x, start_y;
        // stb_truetype renders glyphs with color inverted to what our blitting functions expect
-       unsigned char invert = 0xff;
+       unsigned char invert = cfg->is_inverted ? 0x00 : 0xFF;
        // Render!
        for (line = 0; lines[line].line_used; line++) {
                printf("Line # %d\n", line);
diff --git a/fbink.h b/fbink.h
index 3777e19..8dbe3d0 100644
--- a/fbink.h
+++ b/fbink.h
@@ -198,6 +198,7 @@ typedef struct {
        } margins;
        bool is_centered;         // Horizontal text centering
        bool is_formatted;                // Is string "formatted"? Bold/Italic support only, markdown like syntax
+       bool is_inverted;
 } FBInkOTConfig;
 
 // NOTE: Unless otherwise specified,
@@ -271,7 +272,7 @@ FBINK_API int fbink_printf(int fbfd, const FBInkConfig* fbink_config, const char
     __attribute__((format(printf, 3, 4)));
 
 // Print a string using an OpenType font. Note the caller MUST init with fbink_init_ot() FIRST.
-// This function uses margins (as whole number percentages) instead of rows/columns for 
+// This function uses margins (as whole number percentages) instead of rows/columns for
 // positioning and setting the printable area.
 // Returns -(ERANGE) if the provided margins are out of range, or sum to < 100%
 // Returns -(ENOSYS) if compiled with MINIMAL
diff --git a/fbink_cmd.c b/fbink_cmd.c
index fabf953..6eb5df1 100644
--- a/fbink_cmd.c
+++ b/fbink_cmd.c
@@ -690,11 +690,23 @@ int
                                    fbink_config.fontname,
                                    fbink_config.fontmult);
                        }
+                       // TTF!
+                       FBInkOTConfig cfg = { 0 };
+                       cfg.is_formatted = true;
+                       cfg.margins.top = 5;
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Regular.ttf", FNT_REGULAR);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Italic.ttf", FNT_ITALIC);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-Bold.ttf", FNT_BOLD);
+                       fbink_add_ot_font("/mnt/onboard/fonts/Bookerly-BoldItalic.ttf", FNT_BOLD_ITALIC);
+                       fbink_print_ot(fbfd, string, &cfg);
+                       fbink_free_ot_fonts();
+                       /*
                        if ((linecount = fbink_print(fbfd, string, &fbink_config)) < 0) {
                                fprintf(stderr, "Failed to print that string!\n");
                                rv = ERRCODE(EXIT_FAILURE);
                                goto cleanup;
                        }
+                       */
                        // NOTE: Don't clobber previous entries if multiple strings were passed...
                        //       We make sure to trust print's return value,
                        //       because it knows how much space it already took up ;).
diff --git a/fbink_types.h b/fbink_types.h
index 89d1c5f..42187d2 100644
--- a/fbink_types.h
+++ b/fbink_types.h
@@ -23,6 +23,9 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+// NOTE: We need to import stbtt early because we depend on stbtt_fontinfo here
+//       We'll want it as static/private, so do that here, because we're importing it earlier than fbink.c
+#define STBTT_STATIC
 #include "stb/stb_truetype.h"
 
 // List of flags for device or screen-specific quirks...

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

Okay, ungluing myself from the screen, for real this time ;).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

I'm not sure if that might be an issue here, but I've had weird things happening with libu8 skipping over a single terminating NULL because something looked juicy enough after the end of the buffer ;).
I 'fixed' that by making my buffers terminated by a "wide" NULL (i.e., 4 bytes).

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 27, 2018

And on the topic of possibly unrelated stuff, here's what's Clang's static analyzer had to say:

fbink.c:2901:53: warning: Division by zero
        unsigned int num_lines = (unsigned int)(viewHeight / (uint32_t)max_height);
                                                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
fbink.c:2921:13: warning: Result of 'calloc' is converted to a pointer of type 'unsigned char', which is incompatible with sizeof operand type 'char'
        fmt_buff = calloc(str_len_bytes + 1, sizeof(char));
                   ^~~~~~                    ~~~~~~~~~~~~
fbink.c:2966:9: warning: Assigned value is garbage or undefined
                                        sf = itMetrics.sf;
                                           ^ ~~~~~~~~~~~~
fbink.c:3048:14: warning: Result of 'calloc' is converted to a pointer of type 'unsigned char', which is incompatible with sizeof operand type 'char'
        line_buff = calloc(max_lw * font_size_px, sizeof(char));
                    ^~~~~~                        ~~~~~~~~~~~~
fbink.c:3050:15: warning: Result of 'calloc' is converted to a pointer of type 'unsigned char', which is incompatible with sizeof operand type 'char'
        glyph_buff = calloc(font_size_px * font_size_px * 2, sizeof(char));
                     ^~~~~~                                  ~~~~~~~~~~~~
fbink.c:3084:9: warning: Assigned value is garbage or undefined
                                        sf = itMetrics.sf;
                                           ^ ~~~~~~~~~~~~

fbink.c:2088:18: warning: Potential leak of memory pointed to by 'font_info'
                return ERRCODE(EXIT_FAILURE);
                               ^~~~~~~~~~~~

I tend to take all of that with a grain of salt, but, who knows, sometimes it does catch things ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 30, 2018

There we go. Changed the eink refresh implementation, and I've also implemented support for the is_cleared and is_flashing options.

I think that's about it, apart from the previously discussed DPI matter.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 30, 2018

👍

I'll take a final look at it tomorrow then, and I'll handle the dpi & CLI tweaks once it's merged ;).

Many thanks for working on this!

@shermp
Copy link
Contributor Author

shermp commented Oct 30, 2018

Thanks for the assistance!

I feel a bit bad for opening this PR so early, but your input has been greatly appreciated, and has hopefully resulted in a much more complete and stable solution.

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 31, 2018

As you've probably gathered by now, I'm pretty much in the "throw stuff at the wall, and see what sticks" camp, so, really, not a problem ;).

@NiLuJe NiLuJe merged commit b4eb792 into NiLuJe:master Oct 31, 2018
@NiLuJe
Copy link
Owner

NiLuJe commented Oct 31, 2018

Here we go! :).

Thanks!

@shermp
Copy link
Contributor Author

shermp commented Oct 31, 2018

You're welcome.

Although I just noticed a boo-boo I made that got merged. I forgot to reword the fbink_print_ot() API documentation regarding margins. It is still referring to percentages instead of pixels.

@shermp shermp deleted the opentype branch October 31, 2018 22:33
@NiLuJe
Copy link
Owner

NiLuJe commented Oct 31, 2018

Yeah, I noticed, no worries, I'll take care of it ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 31, 2018

Thanks

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 31, 2018

One other thing I just noticed, which I haven't yet looked into: with (at least) valign CENTER, when re-using the top margin, subsequent print calls won't be edge-to-edge, the second line will start at 3/4 down the screen ;).

@shermp
Copy link
Contributor Author

shermp commented Oct 31, 2018

It's that way by design. Alignment is not to center of the viewport, but rather center to whatever the printable area is, which is defined by the margins. It allows for some very creative positioning.

But now that you've brought it up, I can also see the potential issue with it.

However, as soon as you make multiple print calls for vertical centering, you lose the perfect centering anyway (as previous lines aren't reprinted), so you may as well not use valign, but something like a 50% top margin.

TLDR, vertical centering really only makes sense in the context of a single print call (which honors linefeeds, so you can still do multiple lines in one call, if required)

@NiLuJe
Copy link
Owner

NiLuJe commented Oct 31, 2018

Yep, that makes perfect sense, will just have to jot that down somewhere ;).

@shermp
Copy link
Contributor Author

shermp commented Nov 1, 2018

I feel a bit bad for not doing a better job of cleaning up the type conversions/casting. My apologies for that, and thank you.

Unfortunately, it was the nature of the libraries used that required so much type casting. And in my own code, there was quite a bit of well what type do I use here...? going on.

@NiLuJe
Copy link
Owner

NiLuJe commented Nov 1, 2018

Well, to be fair, I do use a fairly psychotic amount of warnings, so, that doesn't help ;).

And yeah, the fact that stb is engineered by a long-time Win32 games dev relying on a really old VS toolchain means no shiny C99 types (well, no shiny C99 anything, really :D), and a fine balancing act between memory usage and performance when it comes to the choice of types.

Which leaves us with a few questionable choices to deal with (signed codepoints, really? :D).

TL;DR: What was left was mostly making GCC happy one way or another, don't worry about it too much ;). Didn't really hit any snag when it came to the actual underlying types.

@NiLuJe
Copy link
Owner

NiLuJe commented Nov 2, 2018

@shermp: Quick question, since I'll probably attempt to look at the AA stuff in esoteric rendering modes over the next few days: What was the reasoning behind inverting the colors if fg < bg? Was that only for the sake of the layer_diff computation, or am I missing something? (I haven't looked into it that much, so I very well may be ;)).

Because if it's only about layer_diff, I was thinking that abs(fg - bg) should do the trick?

@shermp
Copy link
Contributor Author

shermp commented Nov 2, 2018

Now that I think about it, layer diff could have been made negative and it still would work. The main concern is that when performing the following calculation:

lnPtr[k] = (unsigned char) DIV255( (bgcolor + (glPtr[k] * layer_diff)));

the fg should be darker than bg if "black on grey/white" or lighter if "white on black/grey"

I was trying to get my head around it, and realized that the two situations were the inverse of each other, hence the inversion. But thinking of it now, a simple negative layer_diff where needed would do the same thing. In other words, I over complicated things :p

@shermp
Copy link
Contributor Author

shermp commented Nov 3, 2018

Just some musings on the AA side of things, I've just had an idea.

Currently, each 'line' is prepared with the background colour, and then the glyphs are rendered onto it, baking the final colour information in the process.

What if instead, we treat the line itself as an alpha, and only when painting to the framebuffer do we set the background/foreground colour etc. This would preserve the full AA information in the glyph right up to the final rendering stage.

Thoughts?

@NiLuJe
Copy link
Owner

NiLuJe commented Nov 3, 2018

I think that's what I was thinking of, yeah: keeping the alpha coverage mask we get from stbtt up until the final rendering stage, where we "just" have to alpha blend it (with the usual 0/0xFF shortcuts to avoid the maths).

@NiLuJe
Copy link
Owner

NiLuJe commented Nov 3, 2018

Okay, started experimenting for real in the ot-blend branch, because trying to figure that out without code & testing was starting to break my brain ^^.

So far, so good... (3c7b0e8).

Performance is roughly similar, so I'm not even sure I'll bother adding a fast path for B/W||W/B where we could just paint the mask inverted or not...

@shermp
Copy link
Contributor Author

shermp commented Nov 3, 2018

Looks good!

I probably should have taken this approach originally, but I'm still rather a novice at this whole per-pixel image manipulation business...

@NiLuJe
Copy link
Owner

NiLuJe commented Nov 3, 2018

I nearly threw the Kindle 3 out the window near the end, there... :D.

But apart from the monstrosity that is handling those 4bpp fbs, it went fairly okay, once I realized I'd forgot a multiplication... -_-".

@shermp
Copy link
Contributor Author

shermp commented Nov 3, 2018

Just noticed another potential boo-boo. Where we are painting lines using the loop for (unsigned int j = 0U; j < font_size_px; j++), we probably need to change font_size_px to max_line_height. Might help avoid any potential clipping with your favorite font...

@NiLuJe NiLuJe mentioned this pull request Nov 3, 2018
@NiLuJe
Copy link
Owner

NiLuJe commented Nov 3, 2018

Indeed it does, nice catch!

NiLuJe added a commit that referenced this pull request Nov 3, 2018
NiLuJe added a commit that referenced this pull request Oct 2, 2019
Note that this was initially an idea floated by @shermp in #20
(#20 (comment)) :).
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

Successfully merging this pull request may close these issues.

3 participants