extmod/modframebuf: FrameBuffer text scaling - #6263

Ugh. qemu-arm port build and tests failing:

CC test_main.c
CC ../../lib/tinytest/tinytest.c
In file included from test_main.c:20:0:
build/genhdr/tests.h: In function 'test_extmod_framebuf1_py_fn':
build/genhdr/tests.h:45181:50: error: trigraph ??' ignored, use -trigraphs to enable [-Werror=trigraphs]
 "bytearray(b'\\x00\\x00\\xff\\xff\\xff\\x00\\x00???')\n"
                                                   
CC build/frozen_content.c
cc1: all warnings being treated as errors
../../py/mkrules.mk:63: recipe for target 'build/test_main.o' failed
make: *** [build/test_main.o] Error 1
make: Leaving directory '/home/travis/build/micropython/micropython/ports/qemu-arm'
The command "make ${MAKEOPTS} -C ports/qemu-arm -f Makefile.test test" exited with 2.

Trigraphs? Seriously? The 1970s are calling and they want to force us to accommodate their crazy keyboards.

Will alter test to not generate output that contains repeated question marks...

If anyone wants to know what the result of this is. It looks like this:

This screen is seriously tiny, so doubled-up text allows it to be read from a distance instead of having to squint at it up close.

Made this patch more flexible by switching to specifying the font size (in pixels) instead of an integer scaling factor. This both allows for drawing text at effective non-integer scales, but also means that a future version could select an alternative built-in font for drawing larger text. Drawing text will be slightly slower with this version due to increased bit twiddling and less scope for short-cutting out of the Y loop.

Related PR: #3583 Simple font size scaling for framebuf

How is this PR coming along? We love the idea of increasing accessibility and usability for small displays. If we're taking votes, I'd vote for a keyword size argument.

As an aside, we've received interest in our forums (thread) at Core Electronics for variable font sizes.

At the moment this PR doesn't seem to have generated sufficient interest to get mainline attention.

Since I always build my own MicroPython these days, I just roll this patch into my production branch; so I'm committed to continuing to support it for the foreseeable future.

We featured this PR in this week's episode of The Factory - our fortnightly engineering catchup.

+1 to see this implemented. Great work @jonathanhogg and hopefully @dpgeorge can assist,

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.55%. Comparing base (7a61d7c) to head (056fa29).
⚠️ Report is 198 commits behind head on master.

@@            Coverage Diff             @@
##           master    #6263      +/-   ##
==========================================
- Coverage   98.59%   98.55%   -0.04%     
==========================================
  Files         179      182       +3     
  Lines       23244    23311      +67     
  Branches        0        5       +5     
==========================================
+ Hits        22917    22975      +58     
- Misses        327      335       +8     
- Partials        0        1       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

See also #8987. It would be good to see some updates to framebuf.

this PR has just celebrated its 6th anniversary

I was deliberately looking through PRs in order of age to incite some movement- for better or worse.

Might be prudent just to close the PR?

Don't get me wrong, it's always lovely to get comments

You are absolutely right

Thanks @Gadgetoid for waking up the discussion on this PR, which indeed has been neglected for a long time (sorry @jonathanhogg!)

I think the underlying issue here is - as always - the size/functionality trade-off. Having a way to scale the built-in ASCII-only font is undeniably useful, but the question is whether we can justify the code size change in every build when there are other ways to get arbitrary font size rendering from the Python side (i.e. https://github.com/Gadgetoid/fbppf/ as mentioned, also https://github.com/peterhinch/micropython-font-to-py).

Also noting that this seems to be very popular idea, judging by the number of comments over the years.

The code size report isn't showing for this PR, and that's the most useful thing to help decide if the size/function trade-off is worth it. So I've taken the liberty of rebasing one last time to generate that report.

The code size report isn't showing for this PR, and that's the most useful thing to help decide if the size/function trade-off is worth it. So I've taken the liberty of rebasing one last time to generate that report.

It is showing. 🤦 #6263 (comment)

64 bytes (showing on most ports right now) honestly seems like a pretty reasonable price to pay for this functionality, but I'll wait for the report to update in case something has changed in the past four years...

EDIT: After rebase it's still 64 bytes on the microcontroller ports.

This is a neat, minimal way to add very useful functionality, making the .text() method much more usable.

But it introduces an integer division for each pixel. I benchmarked it, comparing this PR to master. I used fb.text("Test", 0, 0, 0xff) to render 4 characters, in a loop 100 times and got the average time for that call:

• on stm32/PYBV10: 48.9us on master, 67.8us with this PR --> 39% increase in time taken
• on rp2/RPI_PICO: 70us on master, 131us with this PR --> 87% increase in time taken (no hardware division)

So, it's significantly slower with the scaling.

To try and optimise it, I did a simple thing where it used the original inner-y loop for size=8. That still cost about 20% in time taken.

Probably the best way to optimise out the division would be to use fractions with integers (like Bresenham's algorithm does -- it probably has a better name).

Yeah, I knew when I wrote this that it would be substantially slower than the old version, which is a super neat and concise implementation. I originally had a version that only supported integer scaling which could be implemented more efficiently, though it added another inner loop.

I guess it depends on what you're optimising for. In my case, I wanted the code to be simple and to add as few bytes as possible. The very act of doubling the size of the text meant 4 times the pixels and, even then, the screen refresh was a tiny fraction of the code runtime.

I'd think harder about this, but my original use case has now long since faded into obscurity and I've not really got the spare time. You can feel free to close this PR unmerged – I only ever opened it in case it proved useful to other people, which I guess it did for at least a while.

@jonathanhogg thanks for the reply. There's definitely no expectation for you to update this PR. It is very old.

(The reason I'm going through this and other PRs now is to fully clean out the PR backlog, so that good PRs like this one don't sit around forgotten by me!)

I'll have a further think about this, and how it could interact with #16470 and maybe other font ideas.

Goddamn you @dpgeorge. You have successfully nerdsniped me into doing a version of this that uses only integer addition/comparisons.

It is likely still slower and, while it passes the tests, I do not have a board to hand on which to do visual checks. I've written this just be looking at the code and thinking about it. It should be correct for both size > 8 and < 8. I may dig out a board with a screen later and test it.

Goddamn you @dpgeorge. You have successfully nerdsniped me into doing a version of this that uses only integer addition/comparisons.

!!

I've tested your latest code with the fb.text("Test", 0, 0, 0xff) test described above:

So it definitely makes a good improvement to the speed (compared to using division).

Visually it looks OK based on my limited testing.

Code size increase with the latest change is also only a tiny bit bigger than before.

[Again, no expectation for you to do any further work here 😄 ]

I'm kinda surprised that the scaling support is only 30–40% slower than the original code, honestly. I've just tweaked again to optimise loop exits on screen overflow.

I guess whether this is worth merging depends on whether the benefit of simple scaling is greater than the performance cost. Perhaps someone with a more demanding use case (like logging to screen) needs to test it.

I am tempted to do another pass at this that draws the characters upwards from the bottom and shifts vline_data in the other direction as that'd allow for quicker inner loop exit on short characters…

Maybe it's just me overengineering things as usual, but won't it be faster to compute the glyph bounding box before starting to draw? Depending on the compiler you may still have to perform a check on j but you'll have enough information to not even enter the y loop in the first place.

Also, for that 0.0001% speed boost people crave so much: if the font is not encoded "the wrong way", inverting the order of the indices (as in, for y in height { for x in width { } }) should result in smaller (and maybe faster) code. Plus you will hit memory linearly and won't risk having to flush small cache lines on lower end processors :)

Edit: oh, and for cheaper speedups, if you skip the drawing loop altogether if the character is not printable and make it just advance the cursor by (width, height) pixels, you can also change the comparison to draw characters if they're in the 33..127 range, since 32 (0x20) is also an empty character (space)?

Reversed the y drawing direction for fun. Performance change depends on how much you use short letters: maximum speed increase is if your output consisted of only underscore characters 😉

I've also done some renaming and improved commenting.

@agatti: Computing bounding boxes on the fly would slow it down rather than speed it up I think. Pre-computing them would maybe lead to some small improvement, but you'd have to add some 384 bytes of table to the binary which feels like a poor cost/benefit. In general, the looping is already very efficient – e.g., exiting the inner loop immediately for an empty column or early for a short one (see this latest commit). Yes, one could shortcut spaces, which may add a small benefit.

Computing bounding boxes on the fly would slow it down rather than speed it up I think.

Probably I've expressed myself incorrectly (sorry!). What I meant was to compute the x and y span lengths before entering the double loop. Something like this (not handling scaling in the drawing loop, everything else should stay the same):

int xspan, yspan;
xspan = MIN(screen_width - x_position, character_width);
yspan = MIN(screen_height - y_position, character_height);
if (xspan && yspan && char > 32 && char < 128) {
	uint8_t *rom_glyph = &font[char * 8];
	for (int y = y_position; y < yspan; ++y) {
		uint8_t glyph = *rom_glyph;
		for (int x = x_position; x < xspan; ++x) {
			if (glyph & 1) {
				set_pixel(x, y, colour);
			}
			glyph >>= 1;
		}
		++rom_glyph;
	}
}
x_position = MIN(screen_width, x_position + character_width);
y_position = MIN(screen_height, y_position + character_height);

Considering you're handling strings you can actually compute the final bounding box by iterating over all characters and stop rendering at the right time and so on. There's plenty of tricks you can use, but again, those take up space :)

xspan = MIN(screen_width - x_position, character_width);
yspan = MIN(screen_height - y_position, character_height);

The key question is where these character_width and character_height values come from? Which takes us back to my point about computing vs space.

The entire drawing routine is only some 30 lines of code even with my scaling tricks, so feel free to take a crack at it.

Well, you already have it - the font is a 8x8 square and the scaling factor is uniform across axes, isn't it? :)

Anyway, I'll take a closer look at this once it's merged as this has spent enough time in the review queue.

I also wonder what impact hoisting formats[fb->format].setpixel out of setpixel might have. It's inline but formats[fb->format] is looked up every pixel where it could be a single cached pointer for the whole run.

I also wonder what impact hoisting formats[fb->format].setpixel out of setpixel might have. It's inline but formats[fb->format] is looked up every pixel where it could be a single cached pointer for the whole run.

As far as I can make out, it makes zero difference (at least on ARM) – so I guess the compiler or instruction set optimises this away.

@agatti, if you're talking about just optimising for characters that are off-screen, then that would seem to be a small use case and one that is already reasonably and cheaply covered in the code. Otherwise, I've no idea what you mean.

I benchmarked the latest version here, with commit "extmod/modframebuf: Draw bottom to top.".

It's a bit slower: PYBV10 up to 65us (was 63.5us) and RPI_PICO up to 109us (was 99us).

Please note that this file is compiled with -Os! Changing that to -O2 for PYBV10 adds 1456 bytes to the firmware and gets the benchmark down to 52us (with latest commit, so down from 65us). So there's easy wins in terms of compiler optimisations, but they cost space.

As usual, MicroPython is multi-faceted tradeoff: minimalism, efficiency, hardware restrictions, usability (a point in favour of being able to easily scale text), and in some cases performance (relates to efficiency). But that's what makes it fun 😄

It's a bit slower: PYBV10 up to 65us (was 63.5us) and RPI_PICO up to 109us (was 99us).

Interesting. Working bottom to top requires an 8-bit left shift and that may require additional mask instructions that offset any early-exit gain. It might be more efficient to store the font the other way up, but we're into vanishing gains territory likely and who knows how much any micro-benchmark reflects real-world usage.

I can reverse out the bottom-to-top part if you want the simplest/clearest version of the code. How likely is it to be merged is the question I guess?

I would go for the version which has the smallest code size impact with ballpark performance. And I think that might be the version as it stands now? Just looking at the stm32 report above, +48 bytes is the lowest it ever got, and that's what it's at now.