kyamagu
Bindings are a bit outdated
The bindings weren't touched for some time. I've updated these to the latest version. Two major things changed in the API:
- SamplingOptions
- YUV-API
I'm compiling these bindings by myself. I mean, this merge request does not contain "clean code" (please don't just merge it). but could be the basis for the next version. Using it here already: https://github.com/0lru/p3ui with d3d12 (d3d12 is also the reason for the update)
Thanks for bringing this up. Updating internal skia involves a lot of work other than adapting the binding implementation. The general steps are:
- Identify all of the Skia API changes between versions
- Update bindings
- Update Testing
- Update Documentation
The documentation part is actually the hardest, as the change in documentation is hard to identify, and the current docstring is hard-coded for each API.
I would request completing the above for this PR to continue.
Y, I also realized that (your, quite nice) documentation-style is a lot of effort. On the other hand, the API changes are manageable (I mean, it's already "in use" and working, partly :-) ). The biggest changes were/are the two mentioned in the previous post. I mean: The longer we wait, the more difficult it'll get.
I could work overtime "a bit" on that. But to be honest, I don't have the manpower/time to do this alone.
Concerning the build: This also changed a bit. You have to call some batch-file on windows. I already mentioned that in some other post iirc: I'd prefer building Skia in its own repository, as I already did, but not just for windows: https://github.com/0lru/p3ui_skia. You can check the Rust-Skia-bindings. They're doing more or less the same.
Suggestion: Branch the repo and work together on this with more than a single pull request. (delete this pull and make a few out of it, also)
Alright, I would set up a development branch.
Is there any specific skia branch you are considering? If no, the latest milestone m98 would be the target.
@0lru I just created the m98 branch from the main. Perhaps we can close this PR and start working on the updates?
The CI failure is a bit different issue and might be better to fix in another PR or branch. I understand splitting skia build would make CI efficient. This python package depends on cibuildwheel and the only requirement for setting up a split skia-builder repo is that the build must happen in the same environment; i.e.,manylinux docker images.
Commented in https://github.com/kyamagu/skia-python/issues/192#issuecomment-1626503549 , I need m103+ 😞
I think this pull is rather unsatisfactory, actually - it goes by disabling a hell lot of api's, then gradually re-enabling them.
I have tried a different approach - not doing skia build myself but just reuse the pre-built headers + libraries from the jetbrain folks, and see how far I get. Trying m110 and things were breaking left right and center. So I backtracked and just do the next one - m88 is when the SVGDOM class become non-experimental so the header moved.
The diff between m87 and m88 is close to 1000 lines! I haven't bothered with updating the docstring, just looking at Skia API changes and updating the binding. That's already a whole day's work. Would be a bit painful to do the same 20 times to get to m108. (Ideally I need m103 at least)
@HinTak As you've noticed, this repository is unsuitable for keeping up with the latest Skia development. Presumably, everything should be automatically generated including the docstring via AST, although I don't have any time and resources to do that.
@kyamagu strangely enough, now that I can build current skia as a shared library in about 40 minutes (https://github.com/HinTak/skia-building-fun), and having filed an issue with skia and got some response from skia developers about symbol resolution between the core and optional modules like the SVG one, I am convinced that @0lru did the right thing.
The thing is: (1) skia developers actually mark some routines / classes as SK_API, indicating they are likely to stay, with a second category SK_SPI for recent ones that may go in that direction but not yet, (2) using skia.h and static library is a bad idea - because you have private headers and also private symbols that way. By not using skia.h and switching to individual headers with m116 (and shared library with symbol visibility on), I found that you were using some private classes, and doing some needless work which are removed / unavailable in shared library builds.
So I think actually removing a lot of material (accessing private symbols not part of SK_API) in current skia-python is the correct approach, actually. The problem is that skia python has been around for too long and people are getting used to coding in certain way... anyway, I have a m116 fork of skia-python with a lot of material removed, so it probably is terribly broken for practical use, but I intend to add back stuff as I need it, and hope that eventually it is good as a replacement and you can merge it.
So I have a m116 fork that builds.
My m116 fork gets a bit better now - 115 failed, 1972 passed, 34 skipped, 8 xfailed, 66 errors.
Stock currrent m87 on my computer, 2 failed, 2169 passed, 28 skipped, 8 xfailed.
So My passes about 90% of tests, the non-passed ones are half failed half error.
There is a small complication: when routines have similar purpose but done internally differently, do you keep the old name, switch to new name, or both?
I also found skia-python touches/covers about 1000 of the 2400 routines; about 200 of them changed between m87 & m116. :-(.
Anyway, I intend to put a fork out soon, and put it to "production" use, while fixing the rest. At the moment it passes 90% of tests, so it probably is useful to most already.
@HinTak Great!
There is a small complication: when routines have similar purpose but done internally differently, do you keep the old name, switch to new name, or both?
Any example?
The whole image IO - "encodeToData" is gone. It is recommended to use the actual individual graphic format encoder api directly. Canvas.drawBitmap / drawBitmapRect are gone. They re-implemented with drawImage/drawImageRect in my fork... but I wonder how much effort should I go towards "emulating" old APIs.
@HinTak Got it. I would say those breaking changes should be reflected in the Python API and documented in the release note. In other words, keep a record of which API is gone.
Anyway, this should be a huge effort. Great thanks!
https://github.com/kyamagu/skia-python/pull/196
I'd like to set up CI on that pull (and possibly on my detached mirror too). Is there any help/instruction somewhere on that?
@HinTak I've invited you as a collaborator. Let me see if there is any setup to enable CI
I think I have incorporated all the changes that @0lru did (it is useful to see somebody else's work going half of the way), except I have decided not to expose all the interiors of SamplingOptions - in my m116 fork only the bare no-arg constructor is exposed. My idea is that we shouldn't add API which might change in the future and nobody actually uses :-). And get into a nightmare of having to update it as upstream moves!
If somebody wants to play with the inside of SamplingOptions, they should supply an example use and a test case.
I believe everything in this pull is now superceded by pull #236, and #240 . Closing.