search

linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.24k stars 93 forks source link

Refactor the test picture generator #520

Closed xStrom closed 2 years ago

xStrom commented 2 years ago

The problem

Currently the test-picuture example projects for Direct2D/CoreGraphics/Cairo are very bespoke.

The code is brittle and not as widely tested. My motivation for doing this refactoring comes from finding out that the CoreGraphics test picture code has no understanding of stride. Yet it gives 0 (which means automatic) as the stride to CGBitmapContextCreate, so macOS decides.

Running tests on my macOS 10.15.4 with sample picture 0 (which has a width of 200dp) gave me the following results:

Scale Expected Actual
1.0 800 832
1.5 1200 1216
2.0 1600 1600
2.5 2000 2048

As you can see, the faulty logic is invisible at a scale ratio of 2.0 but causes an assert failure with other scales, as macOS seems to automatically choose a stride that is divisible by 64.

The solution

Nowadays we have the functionality in piet-common to provide us with a bitmap render target and the ability to save the results as a PNG. Indeed the piet-common code path even already has stride support thanks to #196.

This PR thus removes most of the platform specific code from test-picture and replaces it with simpler code that uses piet-common to achieve a more robust result.

xStrom commented 2 years ago

I created piet-snapshots#29 to update all the Cairo snapshots to RGBA PNGs. This PR-combo is now ready for review.

x3ro commented 2 years ago

Not sure why, but this seems to lead to some sort of transparency difference in in example picture 15, if I'm not mistaken.

Screenshot 2022-06-28 at 10 08 17
xStrom commented 2 years ago

Yeah I noticed it too and it's interesting. I'll note that not all three platforms have the same transparency even before my PR here, so for that reason I didn't worry about it.

The old Cairo 15 looks like the CoreGraphics 15, and the new Cairo 15 looks like the Direct2D 15.

Something related to premultiplied alpha perhaps. Which one is the correct target? Not sure. I think all platforms should look the same. I may take a closer look at this later today.

xStrom commented 2 years ago

Okay, I have solved the alpha issue now as well.

The PNG spec states that PNG does not use premultiplied alpha so the correct choice was to have all the PNGs with straight alpha.

The CoreGraphics BitmapTarget::save_to_file method already did the conversion correctly, while the Cairo and Direct2D versions of that method did not. The reason that the Cairo test images were still correct previously was because the test picture code used Cairo's own PNG export method which did the correct thing.

I updated the Cairo and Direct2D versions of BitmapTarget::save_to_file to correctly do the conversion.

Also, this PR isn't even merged yet but it's already proving valuable by highlighting bugs in the public Piet API. :tada: