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

Ensure we use only frozen Skia APIs #1144

Closed
3 tasks
pavpanchekha opened this issue Mar 26, 2024 · 11 comments
Closed
3 tasks

Ensure we use only frozen Skia APIs #1144

pavpanchekha opened this issue Mar 26, 2024 · 11 comments
Assignees

Comments

@pavpanchekha
Copy link
Collaborator

pavpanchekha commented Mar 26, 2024

Skia seems to somewhat-regularly change its API. In theory, some classes are annotated SK_API, which indicates that they are frozen, while others are not frozen. We should ideally use only frozen APIs. That said, even "frozen" APIs can change; for example, SkPaint::getBlendMode was replaced with SkPaint::getBlendMode_or recently.

Still, perhaps we can assume that the SK_API classes and methods will not change much, and that skia-python can maybe paper over changes if they do (as they did with getBlendMode_or). Then the question is: what else do we need to avoid using? I did an exhaustive audit of our source code and here's what I got:

Definitely needs changing:

  • Everything related to FilterQuality, namely its use in Paint, its use in drawImageRect, and then the actual values of that enum, were all removed way back, like in m90 or something.

Maybe will change in the future:

  • BlendMode is a public enum but isn't SK_API. (I'm not even sure if enums can be SK_API but in any case BlendMode seems to have the same status as FilterQuality.) The fact that getBlendMode changed doesn't make me feel better—looks like Skia is moving toward a new Blender abstraction.
  • A lot of the GPU-related APIs are from the "legacy GL API" and so are on their way out. There's now I think a new GPU driver? Maybe using Vulkan? I found this code pretty confusing.
@chrishtr
Copy link
Collaborator

  • Am I correct that this changed in Skia? Do the original high/medium/low constants exist somewhere or do we need to do all the steps above to construct it?

Not sure, I'll check with the Skia team.

  • Does Skia regularly make breaking changes? Is there any way to avoid? (This is already the second, after they removed getBlendMode in favor of getBlendMode_or.

They do sometimes make breaking changes. Did the Skia Python module already update and break out app? It seems to work locally.

@pavpanchekha
Copy link
Collaborator Author

It's a little tricky. The Skia-Python module has not put out a new "official" release in a while, but the last "official" release doesn't work with Python 3.12 so I've been installing their "beta" releases and those track API changes, including the breaking ones.

@HinTak
Copy link

HinTak commented Apr 21, 2024

The SamplingOptions change is the first item in the release note: https://github.com/kyamagu/skia-python/blob/main/relnotes/README.m116.md#general-overview-of-changes-between-m87-and-m116

@HinTak
Copy link

HinTak commented Apr 22, 2024

SkFilterQuality was removed June 2021, a bit under 3 years ago, in m94 : google/skia@aebe248

Anyway, the migration guide from FilterQuality to SamplingOptions is in:
kyamagu/skia-python#240

You'll need the upcoming skia-python m124 kyamagu/skia-python#236 to migrate FilterQuality related python code.

@HinTak
Copy link

HinTak commented Apr 22, 2024

To be pedantic, it is actually 4 settings - high, medium, low and none. None is even lower than low, as far as I see, but it is the default (instead of medium, which one might assume is the case).

@pavpanchekha
Copy link
Collaborator Author

Hi @HinTak, apologies for the long delay—the semester just ended so it's been hectic. I'm working on getting skia-python compiled from source and I'll first work on filing bugs for all of our uses of old APIs. Then I'll work on converting them to tests.

@pavpanchekha
Copy link
Collaborator Author

I can confirm that, on m124, our browser test suite passes without issue. However, actually running the browser doesn't work due to (maybe?) kyamagu/skia-python#241, because all the text disappears due to the font metrics all returning zero:

image

@HinTak
Copy link

HinTak commented Apr 30, 2024

@pavpanchekha yes, you are on mac os? I tried "Times" and it doesn't work either (reset the push after the failed try). If you can see what incantation that gives a usable font on mac os, that would be nice.

@pavpanchekha
Copy link
Collaborator Author

Ok, I found the issue: HinTak/skia-python#1

With this fix, I can successfully run the browser and it looks bit-for-bit identical to 87.5:

Screenshot 2024-04-29 at 8 53 16 PM

I'm going to test images with SamplingOptions and then think about what remains to do on this.

@HinTak
Copy link

HinTak commented Apr 30, 2024

@pavpanchekha brilliant! That code is a bit confusing - how could it returns 5 zeros instead of none... I'll add that to the m124 pile now.

@pavpanchekha pavpanchekha changed the title Skia removed FilterQuality Ensure we use only frozen Skia APIs Apr 30, 2024
@pavpanchekha
Copy link
Collaborator Author

Closing this since we have now pinned the Skia version.

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

No branches or pull requests

3 participants