[systemd-devel] [RFC 05/12] gfx: add sd-gfx library with unifont section

[David Herrmann]

Hi

On Wed, Nov 27, 2013 at 11:11 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Wed, 27.11.13 19:48, David Herrmann (dh.herrmann at gmail.com) wrote:
>
>> This patch introduces sd-gfx, a systemd-internal library dealing with all
>> these things. Note that it is designed to be exported some day, but for
>> now we keep it internal.
>> While the library itself will be kept small and almost self-contained, it
>> is designed to be extensible and can be used in rather complex graphics
>> applications. For systemd, we plan to add a basic emergency-console, an
>> initrd password-query, kernel-log-screen and a fallback login-screen.
>> These allow booting without CONFIG_VT and provide a basic system for
>> seats without VTs (either CONFIT_VT=n or seats != seat0).
>
> BTW, one more use of this I'd like to see: if we boot-up and detect that
> no AC and very low battery we should show a pretty screen and block the
> boot until AC is plugged in or the user hits some magic key...

Yepp, sounds good. Would be like 200 lines to write that, I guess.

>> +static int gfx_glyph_render(gfx_glyph *g, unsigned int ppi, const struct unifont_data *u) {
>> +        g->buf.width = u->cells * unifont_width;
>> +        g->buf.height = unifont_height;
>> +        g->buf.stride = u->cells * unifont_stride;
>> +        g->buf.format = SD_GFX_BUFFER_FORMAT_A1;
>> +
>> +        g->buf.data = calloc(unifont_height, unifont_max_cells * unifont_stride);
>> +        if (!g->buf.data)
>> +                return -ENOMEM;
>
> Nice, this must be the first use of alloc() I have ever seen that
> actually takes benefit of the weird parameters it takes ;-)

Do I get an award or something? :)

>> +
>> +        gfx_glyph_blend(g, ppi, u);
>> +        return 0;
>> +}
>> +
>> +int sd_gfx_font_new(sd_gfx_font **out, unsigned int ppi) {
>
> Hmm, so far we followed the rough rule that parameters we pass out are
> listed last on the function parameter list.
>
> int sd_gfx_font_new(unsigned ppi, sd_gfx_fond **out);
>
>
>
>> +        sd_gfx_font *font;
>> +        int r;
>> +
>> +        ppi = CLAMP(ppi, 10U, 1000U);
>> +
>> +        font = calloc(1, sizeof(*font));
>
> font = new0(sd_gfx_font, 1);

I actually dislike new0() to require the type. I'd prefer:
  font = new0(font);
But this is horrible to read, so I went with calloc(1, sizeof(*xyz));
But the systemd code-base seems to use new0() consistently so I can
adjust all my calloc()s if you want? I'm fine with keeping consistency
here.

>> +        font->glyphs = hashmap_new(trivial_hash_func, trivial_compare_func);
>> +        if (!font->glyphs) {
>> +                free(font);
>> +                return log_oom();
>
> Hmm, "library" code should never log. Please just return -ENOMEM
> here. log_oom() should be used in main programs only really...
>
>> +void sd_gfx_font_free(sd_gfx_font *font) {
>> +        gfx_glyph *g;
>
> For public APIs we are pretty defensive and check all parameters we
> take. Given you kinda made this a public library it might make sense to
> follow that rule here, too. assert_return() is particularly useful for
> this. (Well, not for the instance above, bug if you return a negative
> errno it is super-useful).

I wanna avoid these in fast-paths (the blending/blitting functions)
but for all the other ones, I can add assert()s.

> Most destructors we have tend to accept NULL pointers too. It makes
> clean-up code a lot easier (especially in combination with gcc cleanup
> attributes). libc free() takes NULL pointers too, so this is a natural
> extension.

Oh, I know. This is presumably the only destructor where I somehow
forgot that. Fixed.

>> +void sd_gfx_font_render(sd_gfx_font *font, uint32_t id, const uint32_t *ucs4, size_t len, sd_gfx_buffer **out) {
>> +        const struct unifont_data *u;
>> +        gfx_glyph *g;
>> +
>> +        if (!len)
>> +                goto error;
>> +
>> +        if (!id)
>> +                id = *ucs4;
>> +
>> +        g = hashmap_get(font->glyphs, INT_TO_PTR(id));
>> +        if (g)
>> +                goto out;
>> +
>> +        g = calloc(1, sizeof(*g));
>
> g = new(gfx_glyph, 1);
>
>> +        if (!g) {
>> +                log_oom();
>> +                goto error;
>> +        }
>> +
>> +        u = unifont_get(*ucs4);
>> +        if (gfx_glyph_render(g, font->ppi, u) < 0) {
>> +                log_oom();
>> +                free(g);
>> +                goto error;
>> +        }
>> +
>> +        while (--len) {
>> +                u = unifont_get(*++ucs4);
>> +                gfx_glyph_blend(g, font->ppi, u);
>> +        }
>> +
>> +        if (hashmap_put(font->glyphs, INT_TO_PTR(id), g) < 0) {
>> +                log_oom();
>> +                free(g->buf.data);
>> +                free(g);
>> +                goto error;
>> +        }
>> +
>> +        goto out;
>> +
>> +error:
>> +        g = &font->fallback;
>> +out:
>> +        *out = &g->buf;
>> +}
>
> Hmm, we so far followed to rule to not clobber return parameters on
> failure. i.e. we try hard to fill any return values in until we know
> that nothing can fail anymore. This makes the contract for the caller
> much easier who can refrain from freeing/resetting any parameters if a
> call failed...

Oh, I don't clobber the return value here. This is a "void" function,
it never fails. The error-path just stores a fallback-glyph in the
out-buffer. This way, you get some weird
"this-is-an-unprintable-or-caused-by-error"-glyph instead of..
nothing. The application can assume glyph-rendering always works and
just don't deal with possible errors here.

I actually rely quite often on "out" parameters to not get touched at
all if a function fails. It makes error-paths so much nicer.

Thanks
David