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);

[NiLuJe]

And at a very very quick glance, enforcing 'em nets a noticeable win.

Now I have to rememebr when I last checked and why it wasn't so at the time... :D

[NiLuJe]

Chiefly affects print_ot, across all bitdepths.

[shermp]

Back to the hypothetical v2 API, just had a quick thought, maybe consider dumping the whole rows/columns thing? Seems a simple offset/dimensions system would cause a lot less implementation headache.

[NiLuJe]

But I luuuuurrve it ;p.

(Also, it's inherited from eips, and I just find it practical for the fixed-cell rendering codepath. Practical to use, say, certainly not to make it work more or less properly inside FBInk :D).

[shermp]

I guess from my perspective, most of the complexities appear to come from trying to mix the fixed cell layout with pixel based adjustments for centering, padding etc. Seems a bit counter-intuitive to me, but hey, I never used eips, so what do I know...?

[NiLuJe]

Yeah, I definitely get that.

I say "it makes sense as a user", but then I also remember when I was wondering WTF were the lab126 guys on when I first played with it, soooooo :D.

I do like it, once you wrap your head around it. I do agree that it only makes sense in the fixed-cell codepath, which means the whole thing clashes a bit once you start mixing the two together ;).

[NiLuJe]

IIRC, I wasn't mixing the two together (inside the fixed-cell codepath, I mean) at the start. Then the whole "let's handle the weird H2O viewport" thing happen, and here I was with a huge pixel shift grain of sand in my shiny row/column machinery ^^...

And then image support happen, where you probably definitely expected pixels...

Then I went, hey, might be fun to align images to text...

And that's how you end up with a baby monster :D.

[NiLuJe]

Incidentally, once the two started getting mixed together, the fact that you could essentially ignore one whole side of it altogether was kinda neat (i.e., you can use the fixed-cell codepath entirely with pixel values, or entirely w/ row/col).

That doesn't help the code, but at least that insanity doesn't bleed though too much to the user (except in the "there's seventy billion different settings" kind of way ^^).

[NiLuJe]

Whee, found the traces of my previous inline tests:

• Jul 22 2018.
• Apr 18 2019.

The first once I can understand, results were poor because stuff was still going through function pointers; but the second one is interesting: that's what made me switch away from function pointers in put_pixel (at least during the experiment).
I don't remember why I didn't keep the inlining at the time, though :D.

That was in the middle of the qimagescale stuff, so, err, probably a lot of stuff going on...

[shermp]

Just for the record, I was asleep during this current bout of nonsense.

[NiLuJe]

Hey, there's now a fancy new? Report/Block button, kudos GitHub! :).

(If it isn't new, I failed to notice it during the last round of trolling last year or so).

[pgaskin]

Yes, that's new (it was added around halfway through last year). Unfortunately, it still says this issue was "Fixed by hdhbdh/Kobo-Reader#1", though.

[NiLuJe]

Huh. The fact that this is picked up in the title comments (or whatever that area is called) across unconnected repos in the first place is... weird?

(I mean, it's great when it works for actually connected repos, which comes in handy with the various KOReader submodules for cross-references, for instance), but across unconnected (not the same owner/organization/commit rights) feels like an oversight? One that's ripe for the taking for trolling opportunities...

[NiLuJe]

Note to self before I entirely forget about it:

On the subject of setters/getters. Having recently played with ZSTD, I kinda like the approach of a single setter/getter function, which takes a pointer to an opaque struct (e.g., it's just an empty forward declaration in the public header), and you choose what to get/set via a list of constants from an enum as the actual argument.

That is all ;p.

[NiLuJe]

Note to self, again:

• Make fbink_refresh take a pointer to an FBInkRect.
• And as for the internal refresh, ideally, it should just take a pointer to an FBInkConfig instead of splitting a few things off of it, but doing this is currently mildly helpful because of fbink_print_ot's FBInkConfig-less feature...