sammycage / lunasvg

SVG rendering and manipulation library in C++
MIT License
866 stars 124 forks source link

Color channels swapped and unpremultiplied #73

Closed fdwr closed 2 years ago

fdwr commented 2 years ago

Sammy, your library was very easy to integrate πŸ‘, building and displaying something in less than 20 minutes.

image

Though at first, it didn't look so good πŸ˜…, when attempting to draw icons with transparency as I got swapped color channels and ugly fringes around the borders back from lunasvg::Document::render() πŸ˜₯.

image

So I wrote a function to swap the returned channels to BGRA order (and by BGRA, I mean logical memory order, where byte 0=blue, 1=green, 2=red, and 3=alpha), which is compatible with many (if not more?..) graphics APIs, including Windows GDI SetDIBitsToDevice / CreateDIBSection and Direct2D DrawBitmap and Cairo CAIRO_FORMAT_ARGB32. I also premultiplied the colors to be directly compatible with most rendering API's, but after stepping through the code, I observed that lunasvg was actually undoing what plutovg was already appropriately returning in the first place πŸ™ƒ. So I just commented out that one line, and then everything worked beautifully πŸŽ¨πŸ–Ό (and I could delete my helper function too).

image

void Document::render(Bitmap bitmap, const Matrix& matrix, std::uint32_t backgroundColor) const
{
    RenderState state(nullptr, RenderMode::Display);
    state.canvas = Canvas::create(bitmap.data(), bitmap.width(), bitmap.height(), bitmap.stride());
    state.transform = Transform(matrix.a, matrix.b, matrix.c, matrix.d, matrix.e, matrix.f);
    state.canvas->clear(backgroundColor);
    root->render(state);
    // LunaSvg is unpremultiplying all the BGRA values /:-/, which makes them
    // useless for callers to composite icons against a background.
    // Additionally it's swapping all the color channels, which leads to blue and red
    // being backwards when trying to draw them to GDI SetDIBitsToDevice and D2D DrawBitmap.
    // So disable this line and return the true original pixel values.
    // state.canvas->rgba();
}

Shall we add a bool parameter to avoid unpremultiplying the values? Otherwise we end up unpremultiplying them only to re-premultiply them again in common rendering scenarios - lots of extra divisions. Obviously straight alpha would still need to be an option too for some other libraries, like libpng which only accepts unassociated alpha, but I feel like straight alpha is less and less useful over the decades in the general case. Thanks from Seattle.

sammycage commented 2 years ago

Added : 0d3e6a2897a76c907becbe48c1726685531ab455

You can now convert bitmap to any format after rendering.

Example :

auto document = Document::loadFromFile("hello.svg");

Bitmap bitmap(48, 48);
bitmap.clear(0x00FF00FF); // Green background
document->render(bitmap);

bitmap.convert(0, 1, 2, 3, true); // convert To RGBA unpremulitied
bitmap.convert(0, 1, 2, 3, false); // convert To RGBA premultiplied

bitmap.convert(3, 2, 1, 0, true); // convert To ABGR unpremultiplied
bitmap.convert(3, 2, 1, 0, false); // convert To ABGR premultiplied

bitmap.convert(2, 1, 0, 3, true); // convert To BGRA unpremultiplied
bitmap.convert(2, 1, 0, 3, false); // convert To BGRA premultiplied
fdwr commented 2 years ago

Great - nice approach too. Thanks.

So in my case, to avoid extra work, I simply don't call convert :b.

fdwr commented 2 years ago

@sammycage : Btw, I published my little sample app now: https://github.com/fdwr/LunaSvgSampleTest

sammycage commented 2 years ago

@fdwr Interesting πŸ‘πŸ‘πŸ‘