Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the test picture generator #520

Merged
merged 7 commits into from
Jun 28, 2022

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Jun 27, 2022

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 xStrom added piet-coregraphics issue in the CoreGraphics backend piet-cairo issue in the cairo backend piet-direct2d issue in the direct2d backend maintenance cleanup and refactoring blocked labels Jun 27, 2022
@xStrom xStrom removed the blocked label Jun 27, 2022
@xStrom
Copy link
Member Author

xStrom commented Jun 27, 2022

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

Copy link

@Zarenor Zarenor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer to read this way, and leads in the direction of using the common code, which I think is a nice improvement!

@x3ro
Copy link
Contributor

x3ro commented Jun 28, 2022

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
Copy link
Member Author

xStrom commented Jun 28, 2022

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
Copy link
Member Author

xStrom commented Jun 28, 2022

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. 🎉

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a skim, looks good to me. Thanks for digging into it!

@xStrom xStrom merged commit 1976716 into linebender:master Jun 28, 2022
@xStrom xStrom deleted the improved-test-picture-gen branch June 28, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance cleanup and refactoring piet-cairo issue in the cairo backend piet-coregraphics issue in the CoreGraphics backend piet-direct2d issue in the direct2d backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants