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

Improve "convert to PIL" example #250

Merged
merged 1 commit into from
Jul 27, 2024
Merged

Conversation

lucach
Copy link
Contributor

@lucach lucach commented Jun 1, 2024

The default color type of a skia.Image is platform dependent. On some systems (e.g., GitHub CI, and most Linux systems) the default color type is BGR (for endianness purposes).

See https://github.com/google/skia/blob/6ffe89f9b4cc8c1dae9c4a916f16f9c463e3fa6d/include/core/SkColorType.h#L58-L62

Thus, in general, not only one has to convert the alphaType to create a PIL image, but also the colorType.

The default color type of a `skia.Image` is platform dependent. On some systems (e.g., GitHub CI, and most Linux systems) the default color type is BGR (for endianness purposes).

See https://github.com/google/skia/blob/6ffe89f9b4cc8c1dae9c4a916f16f9c463e3fa6d/include/core/SkColorType.h#L58-L62

Thus, in general, not only one has to convert the `alphaType` to create a PIL image, but also the `colorType`.
@HinTak
Copy link
Collaborator

HinTak commented Jun 1, 2024

I am wondering if it is possible to detect what skia's image color type is (such as a class static method), and also adds a few more tests to the pytests.

@HinTak
Copy link
Collaborator

HinTak commented Jun 14, 2024

Oh, we do expose the kN32... type directly:

.value("kN32_ColorType", SkColorType::kN32_SkColorType)
so it is possible to test if it is equal to either of the two, to detect endian.

Copy link
Collaborator

@HinTak HinTak left a comment

Choose a reason for hiding this comment

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

Looking at the diff again, I don't have a strong opinion against having the change as is, so let's just put it in. If somebody (me or else) wants more, they can always add more in a new pull.

@HinTak HinTak merged commit ec5d357 into kyamagu:main Jul 27, 2024
13 checks passed
@HinTak
Copy link
Collaborator

HinTak commented Jul 27, 2024

The merge commit failed CI because of the Centos EOL July 2024 issue - that's fixed by the m127/m128 pulls. we can ignore the failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants