Icon bitmaps with over 1024 bytes of uncompressed 1-bit data unintentionally wrap buffer data on decode

[mikepangel]

Icon by Design

First off I'd like to preface an understanding I have come up with, and have a guess on probable intention of the use of Icons:

• I understand the device is working under tight memory constraints and needs to minimize it as much as possible
• Images are designed to be the size of the screen (128x64@1bpp) or smaller, aka 1024 bytes.
• They can be split up into Animations with expectation of up to screen-sized frames - although I am not aware of how this process is meant to be used yet and I'm unlikely to personally do so
• From what I can observe, Icons are normally loaded in as compressed XMP - and decompressed on-draw only - to save on memory during runtime as a standard to allow more flexible asset usage
• Icons are imported with the expectation of width and height being set as uint8_t values, so images above 255x255 cannot even be included in a project as the generated code will not compile due to the assigned values being too large to assign to a 1-byte unsigned integer - but UFBT will generate 'bad data' in such a scenario regardless of this (this may be a separate Issue to address, as well...)
• Canvas drawing of Icons are limited to uint8_t screenspace x/y drawing positions as well, meaning they cannot be drawn using negative values to be drawn out of bounds, but when drawn past screen bounds they are properly cut off at the bottom and right edges of the Canvas - meaning no Icon bitmap's contents beyond 128x64 can ever be drawn outside of the 128x64 screen size anyways.

Discovery of the issue

My discovery of this came about when working on refactoring another project's game engine and implementing a custom tilemap renderer using a 128x128 bitmap once I ran into the uint8_t width/height limitation mentioned above. It took me a day or two of combing over my code to understand why the output wasn't coming out quite right, thinking it was an error in my buffer alignment math, but it turns out my code is fine - and (as later discovered) I will likely utilize a custom Compress decoder instead of the default CompressIcon one for my needs. However, I do think this is a legitimate problem that should be addressed at some point.

I'm not sure I understand exactly where the bitmap import is looping back on itself in the buffer, but my testing has proven that any image size that exceeds 1024 bytes in its uncompressed 1-bit bitmap form will begin writing back into the buffer's 0 index past 1024 bytes of data instead of discarding the extra data. This causes unexpected behavior when using images for application development.

Examples

To demonstrate this behavior more clearly I have produced test images of varying sizes that (except for the last one) are drawn starting at the topleft of the Canvas at 0,0 using a 128x128 wrap_test image:

Using the method call canvas_draw_icon(canvas, 0, 0, (Icon*)&I_wrap_test); this is the output:

What happens here is the first 64 rows are overwritten by the next 64 rows of the 128x128 image. The expected behavior would be to only draw the first 64 rows and ignore the following 64.

Another example using a truncated part that makes this much clearer with 128x72, where the last 8 rows overlap the first 8 rows:

canvas_draw_icon(canvas, 0, 0, (Icon*)&I_wrap_test_128x72); produces this output where the text "WONT SEE THIS" is overwitten by "THIS WRAPS THIS WRAPS" from the image's last 8 pixel rows:

A thought was perhaps it's not wrapping on asset import, but on drawing into the screen and wrapping in the screen buffer. So I tested using another cut version but this time at 96x128 which is also above 1024 bytes, but still taller than the screen while not being as wide

canvas_draw_icon(canvas, 0, 0, (Icon*)&I_wrap_test_96x128); produces this output:

So this not only correctly draws within the bounds of the screen's buffer but it wraps the imported uncompressed bitmap onto itself without regard for width/height, only the raw buffer itself. Offsetting the icon draw by 32 pixels (at 0,32) produces this using the same above 96x128 image:

---

The root of the problem

How did I discover it was 1024 specifically and not just off an assumption based on the screen size, even though that seems like likely use-case and design? Well, when I looked into how the Icon is actually loaded and drawn I dug up compress.c and discovered the hard-coded #define'd constant buffer size value COMPRESS_ICON_DECODED_BUFF_SIZE (1024u)

This is a constant array size for the buffer defined in the CompressIcon struct which an instance is returned from the compress_icon_alloc() call that has no parameters for size - and this is where I think some kind of alternative or soft-overload (I know C cannot do literal method overloads, but an alternative call would be useful) could be utilized for custom CompressIcon needs. Again, I am not 100% certain this is the cause, but when I look at the ufbt-generated Icon data generated in my project those buffers definitely, while compressed XMP, are above 1024 bytes in size and it seems likely the CompressIcon's strict buffer limitation is what is causing this overlapping-buffer behavior.

Even when I utilize my own CompressIcon and grab the temporary decompressed buffer via compress_icon_decode(...) this overlap behavior persists, so I do believe it has to do with the decompression process being limited by this 1024-byte fixed buffer size that is baked into the firmware, and access to change that is hidden without compiling a modified firmware.

Proposal on how to address this issue

I would propose one or more of the following:

1. UFBT should not import bitmap Icons that exceed any intended size limitation, or at least some kind of warning should be generated when they do. It should also not import/generate uncompilable width/height values if Icon width/height is going to stay uint8.
2. The overlapping buffer scenario should be fixed and excess data beyond the buffer should be disposed in a sensible and understandable behavior. I tried looking more deeply into the exact part of the decompressor code but I have a hard time understanding the process and would leave that up to those who have implemented this process with more knowledge than myself.

Alternative use-case approach

Upon writing this Issue and double-checking my work with a clearer mind disconnected from my prior debugging and analysis I finally noticed what was I going to propose to be added for application developers staring me in the face, so I'll snippet this section for now and test out using a custom Compress (instead of CompressIcon) object using a custom buffer size and see what that yields me. The following block of text is what I was writing up before looking through and finally realizing this apparently already exists and that I should be using this instead:

3. An alternative or updated CompressIcon allocator with a parameter-defined buffer size should be accessible for application developers with 'at your own risk' warnings commented around the use of it, as from what I have understood it's designed to be used with a Canvas only but also seems to be the most direct path for importing images that is available. Otherwise I think default should remain to 1024 bytes to keep the decoder light and screen-size friendly.

As I continue working on my game engine framework that I'll soon release I would prefer not to have to do unsafe hacky alternatives (I have ideas, but they're under assumption this design and struct layout never changes - very unsafe!) to get around this nor have to implement a duplicate library just to properly import assets as I want them, so I'd love to see a custom decoder buffer allocator being made available for anyone who knows what they're doing. If there's something I missed here that I could currently use to get around this that would be absolutely wonderful news to find - otherwise I will in the meantime have to chop up separate Icon imports and stitch them together in my own custom image buffers for game application development which isn't anywhere near intuitive.

I do think keeping the constraints of memory usage low is smart and a good basis for the firmware to continue working with, but this behavior for Icon bitmap decoding needs a fix without strange data issues like this to ensure healthy and stable ongoing software development for the Flipper.

Reproduction

1. Include any monochrome bitmap that is (width*height/8) > 1024 bytes as an Icon
2. Attempt to draw the Icon with the expectation of it drawing the original image (limited to screen's bounds)
3. Observe the image appearing to overwrite itself

Extra: Images imported by UFBT larger than 255 in width and/or height cause compilation errors due to 8-bit width/height value assignment limit

Target

No response

Logs

No response

Anything else?

No response

[skotopes]

So 3 set of problems(ascending significance oder) and how I see them fixed:

• u8g, canvas, icon issues. Main problem is u8g2 itself: it got some type size legacy from avr times, it's unable to handle negative offsets in rendering. Then canvas that uses it for rendering and inherit its problems. Ideally we must get rid of u8g2 and refactor canvas API. I've been thinking about it for some time, but there are other high priority tasks that we'd like to focus on first.
• canvas icon decompressor. It was designed to hold 1 screen. Its memory is statically allocated at start to guarantee that we are not going to fragment heap or run out of memory. Ideally I don't want to change this part. Also you don't have to use compressor. Icons can be uncompressed, in that case there will be no problem.
• fbt and scripts: yes, they should report and stop if size exceeds expected values. That we can fix.

So really complex topic, which we already started to unravel, but there will be no solution in short time.

[mikepangel]

No worries, and thanks for the comprehensive and quick response. I have since implemented a working implementation using a Compress decoder with my own decoder buffer and it is working just fine for my current use case now, but definitely wanted to raise the issue to address the points I made for the long term and goal to 1.0 firmware.

There are alternative ways I can import bitmaps of course - and uncompressed is more ideal to import with - but I wanted to see if I could utilize the current automated Icon format that Flipper has baked in for a tighter integration with my engine and have my own dynamic per-asset compressed memory support able to using the existing FBT imports for quicker iteration time - and it seems I found my workaround already.

Cheers!