Musings and ideas for a potential v2 API

[shermp]

Carrying on from our discussions in shermp/go-fbink-v2/pull/9, I just wanted to jot down a few ideas rolling around in my skull while they are fresh.

Feel free to ignore, or wait on this.

• Splitting the library into eink refresh/blitting component, and a text/image rendering components. The idea is if one wants to use different libraries for text or images, they can. The hard part in many cases is dealing with all the quirks of eink, and device framebuffer formats etc, and it would be nice to have a standalone library to deal with that.
• Not using a massive FBConfig struct to configure everything under the sun. IMO, it's gotten rather unwieldly as more features have been added. One potential idea:
  • An opaque context struct that contains per session config variables
  • Change options via "methods" on that context struct.
  • Use an init 'method' to create said struct for the user.
  • While we're at it, create a fbfd in the init function and store it in the context. Drop the AUTO fbfd completely
  • There could be an internal function to test the context for any re-init requirements, so the user doesn't have to think about it.
  • Then when finished, close the context to unmap the fbfd etc.

That's one initial thought from me. I'm generally not a fan of getter/setters, coming from a Python/C background, but in this case it might help prevent API breakage by hiding internal structs etc.

[NiLuJe]

Just a quick braindump, too, some of which was detailed a tiny bit in the linked PR ;).

• Modular builds are currently possible, you can basically drop everything but the basic fixed-cell rendering w/ the default font. That's what KFMon uses (i.e., MINIMAL=1).
  It could arguably be extended to drop the basic text rendering, too ;).

That said, a truly modular library model is definitely not a bad idea, and certainly a fairly minor (famous last words ;p) hurdle once the API is taken care of ;).

• Agreed, that struct is a bit of a monster ^^. I'm also not a fan of setters/getters, so whenever I find myself thinking about it, it ends up looking like a Russian doll-like super-struct, nesting smaller structs (i.e, logical units).
  That's still essentially a giant struct, though ^^.
  Ideally, API breaks would be less of an issue, since the idea would be to stabilize the API. But I get what you mean ;).
  It has the advantage of being fairly easy to implement (i.e., stick a pointer to the nested struct in the function prologue, and you can essentially re-use the code as-is).

• KFMon relies on the fbfd AUTO behavior, so, a way to basically have the whole thing constructed & torn down for each and every call would have to be preserved, somehow ;).

[shermp]

If you need to keep the fbfd AUTO behaviour, I suppose you could pass that as a parameter to the context 'constructor', and use the same logic as you already are to test it.

[NiLuJe]

I think I've mentioned this before, but I'm not really a CS person, and I've never managed to get into OOP, so designing a cool API is really not my forte ^^.

So, yeah, please feel free to throw stuff at the wall, and we'll see what sticks ^^.

[shermp]

I'm not an OOP person myself, although I did take a class on it. I tend to take some of the bits I find useful, but I'm hardly one to preach the OOP gospel!

From a bindings perspective, not having to deal with lots of structs (externally) is kind of a nice thought, which is why I'm a little partial to the getter/setter idea here.

[shermp]

Changing tack slightly, from an internal FBInk standpoint, would it be worth investigating using put_pixels variants? I was just looking at the if-ladder in draw_image() and thinking yikes!

As well as simplifying the if-else ladder(s), I'm wondering if the individual calls to get_pixel/put_pixel allows the compiler to auto-vectorize that code at all. From what little I've read about SIMD, and my gut instinct would make me think that blitting code is currently not being SIMD optimized by the compiler.

Thoughts?

[NiLuJe]

Variants in which way, exactly?

(There's already one variant per bitdepth, which the insane ladder in draw_image takes advantage of. Speaking of that ladder, most/all of it gets hoisted properly, so it's fugly as hell, but it doesn't perform too badly, as far as branching is concerned at least).

But, yeah, none of that gets vectorized, mostly because the loops are too complex, and/or the bounds are unclear/dynamic IIRC.

(Which is why anything than can go through a memcpy or fill_rect instead of put_pixel* is a happy camper ^^).

(The vectorization diagnostics are much more readable since GCC 9).

[shermp]

put_pixels with an s. a function for each bit depth that takes an input buffer, coordinates, dimensions, stride etc, and plots pixels with inversions/alpha as necessary, without needing to use individual function calls to put/get pixels.

I'm also a bit surprised you haven't made put/get_pixel macros or inline functions to be honest. Whether that would make a measurable improvment or not I don't know...

[NiLuJe]

IIRC, every *_pixel_* functions gets inlined properly (except when going through a function pointer, which I think is no longer the case anywhere, precisely to allow inlining to kick-in, IIRC).

(The inline keyword is more of a suggestion than anything, the compiler generally figures these things out on its own just fine, especially for static functions. Which is everything in FBInk, since it's built as an amalgam. What it does is make the compiler emit a diagnostic when it thinks inlining would be a loss. As for always_inline, it will enforce inlining, and slightly tweak that diagnostic's wording accordingly ;p).

[shermp]

Huh. Ok. Fair enough.

[NiLuJe]

KOReader's C blitter does use a few macros, but I found it a bit of a PITA to work with in practice ;p.

Granted, it has to bend its back to follow the legacy Lua API of the Lua blitter, which doesn't help ^^.

[shermp]

I don't blame you on the macro's, I avoid 'em like the plague myself generally.

[NiLuJe]

Okay, apparently they're not inlined everywhere (at a glance, specifically in print_ot). It'd been a while since I checked, I'll look into it ;).

[NiLuJe]

Oh, or when I last checked, having them inlined where the compiler wasn't already doing so was a loss. Can't recall -_-".

Will test again ^^.

[NiLuJe]

For ref.

diff --git a/fbink_internal.h b/fbink_internal.h
index 87a9f69..d167b56 100644
--- a/fbink_internal.h
+++ b/fbink_internal.h
@@ -450,24 +450,24 @@ static void rotate_coordinates_nop(FBInkCoordinates* restrict __attribute__((unu
 
 static inline uint16_t pack_rgb565(uint8_t, uint8_t, uint8_t);
 
-static void put_pixel_Gray4(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
-static void put_pixel_Gray8(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
-static void put_pixel_RGB24(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
-static void put_pixel_RGB32(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
-static void put_pixel_RGB565(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
+static inline void put_pixel_Gray4(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
+static inline void put_pixel_Gray8(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
+static inline void put_pixel_RGB24(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
+static inline void put_pixel_RGB32(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
+static inline void put_pixel_RGB565(const FBInkCoordinates* restrict, const FBInkPixel* restrict);
 // NOTE: We pass coordinates by value here, because a rotation transformation *may* be applied to them,
 //       and that's a rotation that the caller will *never* care about.
-static void put_pixel(FBInkCoordinates, const FBInkPixel* restrict, bool);
+static inline void put_pixel(FBInkCoordinates, const FBInkPixel* restrict, bool);
 // NOTE: On the other hand, if you happen to be calling function pointers directly,
 //       it's left to you to not do anything stupid ;)
 
-static void get_pixel_Gray4(const FBInkCoordinates* restrict, FBInkPixel* restrict);
-static void get_pixel_Gray8(const FBInkCoordinates* restrict, FBInkPixel* restrict);
-static void get_pixel_RGB24(const FBInkCoordinates* restrict, FBInkPixel* restrict);
-static void get_pixel_RGB32(const FBInkCoordinates* restrict, FBInkPixel* restrict);
-static void get_pixel_RGB565(const FBInkCoordinates* restrict, FBInkPixel* restrict);
+static inline void get_pixel_Gray4(const FBInkCoordinates* restrict, FBInkPixel* restrict);
+static inline void get_pixel_Gray8(const FBInkCoordinates* restrict, FBInkPixel* restrict);
+static inline void get_pixel_RGB24(const FBInkCoordinates* restrict, FBInkPixel* restrict);
+static inline void get_pixel_RGB32(const FBInkCoordinates* restrict, FBInkPixel* restrict);
+static inline void get_pixel_RGB565(const FBInkCoordinates* restrict, FBInkPixel* restrict);
 // NOTE: Same as put_pixel ;)
-static void get_pixel(FBInkCoordinates, FBInkPixel* restrict);
+static inline void get_pixel(FBInkCoordinates, FBInkPixel* restrict);
 
 #if defined(FBINK_WITH_IMAGE) || defined(FBINK_WITH_OPENTYPE)
 // This is only needed for alpha blending in the image or OpenType codepath ;).

fbink.c: In function 'fbink_print_ot':
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4845:7: note: called from here
       put_pixel(paint_point, &pixel, false);
       ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4858:8: note: called from here
        put_pixel(paint_point, &bgP, true);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4861:8: note: called from here
        put_pixel(paint_point, &fgP, true);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4866:8: note: called from here
        put_pixel(paint_point, &pixel, false);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4885:8: note: called from here
        put_pixel(paint_point, &bgP, true);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4896:8: note: called from here
        put_pixel(paint_point, &pixel, false);
        ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4889:8: note: called from here
        get_pixel(paint_point, &fb_px);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4912:7: note: called from here
       put_pixel(paint_point, &pixel, false);
       ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4909:7: note: called from here
       get_pixel(paint_point, &fb_px);
       ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4931:8: note: called from here
        put_pixel(paint_point, &pixel, false);
        ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4928:8: note: called from here
        get_pixel(paint_point, &fb_px);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4949:8: note: called from here
        put_pixel(paint_point, &pixel, false);
        ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4936:8: note: called from here
        get_pixel(paint_point, &fb_px);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4967:7: note: called from here
       put_pixel(paint_point, &pixel, false);
       ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4963:7: note: called from here
       get_pixel(paint_point, &fb_px);
       ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4983:8: note: called from here
        put_pixel(paint_point, &fgP, true);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:4997:8: note: called from here
        put_pixel(paint_point, &pixel, false);
        ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:4987:8: note: called from here
        get_pixel(paint_point, &fb_px);
        ^
fbink.c:373:5: warning: inlining failed in call to 'put_pixel.constprop': call is unlikely and code size would grow [-Winline]
     put_pixel(FBInkCoordinates coords, const FBInkPixel* restrict px, bool is_rgb565)
     ^
fbink.c:5013:7: note: called from here
       put_pixel(paint_point, &pixel, false);
       ^
fbink.c:525:5: warning: inlining failed in call to 'get_pixel': call is unlikely and code size would grow [-Winline]
     get_pixel(FBInkCoordinates coords, FBInkPixel* restrict px)
     ^
fbink.c:5010:7: note: called from here
       get_pixel(paint_point, &fb_px);
       ^
fbink.c: In function 'draw_image':
fbink.c:496:5: warning: inlining failed in call to 'get_pixel_RGB565': call is unlikely and code size would grow [-Winline]
     get_pixel_RGB565(const FBInkCoordinates* restrict coords, FBInkPixel* restrict px)
     ^
fbink.c:7019:7: note: called from here
       get_pixel_RGB565(&coords, &bg_px);