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

M120 public #210

Merged
merged 46 commits into from
Dec 5, 2023
Merged

M120 public #210

merged 46 commits into from
Dec 5, 2023

Conversation

HinTak
Copy link
Collaborator

@HinTak HinTak commented Nov 3, 2023

The code changes from m119 to m120 are all obvious, most of them actually mentioned in the top-level release notes. So almost all the changes in the tests are obvious too, just skipping tests involving withdrawn APIs. I don't think those need revisiting, as they are all explicitly mentioned as withdrawn in the release note. Whether we want to emulate them is another matter.

However, somehow there are two new segfaults. I think they are indirectly related / dependent on those withdrawn ones, so they needs re-visiting, either enable them or mark them as indirectly withdrawn.

So this should pass CI, but I'd say those two segfaults need to be either properly skip as withdrawn or re-enabled, before this is merged.

Milestone 120
-------------
    `SkTypeface::UniqueID()` has been removed - clients should use the method instead of this static
    function.
… removed

Milestone 120
-------------
  * `SkFont::refTypefaceOrDefault` and `SkFont::getTypefaceOrDefault()` have been removed from the
    public API.
Milestone 120
-------------
  * `GrBackendSemaphore::initGL` and `GrBackendSemaphore::glSync` have been removed
    from the public API.
Milestone 120
-------------
  * `GrDirectContext::MakeVulkan...` has been moved to `GrDirectContexts::MakeVulkan...` which are defined
    in `include/gpu/ganesh/vk/GrVkDirectContext.h`
Header diff:

$ diff -u skia-m119/src/core/SkMipmapBuilder.h skia-m120/src/core/SkMipmapBuilder.h
--- skia-m119/src/core/SkMipmapBuilder.h	2023-10-06 23:22:04.000000000 +0100
+++ skia-m120/src/core/SkMipmapBuilder.h	2023-11-01 22:34:27.000000000 +0000
@@ -27,7 +27,7 @@
      *  If these levels are compatible with src, return a new Image that combines src's base level
      *  with these levels as mip levels. If not compatible, this returns nullptr.
      */
-    sk_sp<SkImage> attachTo(sk_sp<const SkImage> src);
+    sk_sp<SkImage> attachTo(const sk_sp<const SkImage>& src);

 private:
     sk_sp<SkMipmap> fMM;
The circular dependencies are only in Windows and Linux, from the OT-SVG hook.
Comments on SVG test, and make it pass
Revert "Patch to Hook up SkSVGOpenTypeSVGDecoder::Make to enable OT-SVG"

This reverts commit 9039fbd.

Revert "Solving circular dependencies by listing twice"

This reverts commit 55f6d10.
… defaults to skia_use_freetype &&)

skia_use_freetype=true is not automatic with skia_enable_fontmgr_custom_empty=true

Apparently it still tries to read headers from system locations on non-linux! Need skia_use_system_freetype2=false also.
Increase tolerance to make test_fontmgr_custom_svg_blob_bounds pass on windows
… flag = 10.13.

This is a follow-up to an earlier change - we should keep compile-flag and link-flag
minimum version in sync.

commit ea0e2cd
Author: Hin-Tak Leung <[email protected]>
Date:   Fri Aug 4 06:50:32 2023 +0100

    CI failure log says Skia internals start to require Mac OS X 10.13.
…sh/SkImageGanesh.h

These should have been part of the earlier commit
(adding a misssing new header):

commit deebf369ba0cdc1b2bc825e3ebbdbe0499377a3f
Author: Jonathan Hogg <[email protected]>
Date:   Mon Aug 14 14:50:21 2023 +0100

    Get `Image.MakeFromTexture` working again
Conflicts:
	tests/test_shader.py
@HinTak
Copy link
Collaborator Author

HinTak commented Nov 29, 2023

The segfaults were caused by the withdrawn GL-related API, and actually GrBackendSemaphore only got SK_API status with m120, and GL-backend with this wiil never work. so having the GL-based methods earlier was a mistake. That said, apparently it should be possible if vulkan build is enabled, and should work if the GrBackendSemaphore is initialized with a Vulkan backend: #219 .

@kyamagu I think this is missing a README.m120 but code-wise okay to go, as m121 should be out soon. I'll get on with a README summary soon. Not sure if we should include #219 in this?

…texts::MakeVulkan...`

Milestone 120
-------------

  * `GrDirectContext::MakeVulkan...` has been moved to `GrDirectContexts::MakeVulkan...` which are defined
    in `include/gpu/ganesh/vk/GrVkDirectContext.h`
Adjust for recent m118/120 vulkan changes

Milestone 118
-------------

  * Vulkan-specific calls are being removed from GrBackendSurface.h. Clients should use the
    equivalents found in `include/gpu/ganesh/vk/GrVkBackendSurface.h"`
GrBackendFormats::MakeVk now has an optional argument "willUseDRMFormatModifiers".
…kend is compiled in

Milestone 98
------------

  * GrBackendSemaphore only includes methods that match the GPU backend that Skia was compiled for.
    For example, initVulkan and vkSemaphore are not defined unless the Vulkan backend is compiled
    into Skia.
@HinTak
Copy link
Collaborator Author

HinTak commented Dec 1, 2023

@kyamagu for vulkan related tests, it is mostly just re-enabling them. However, before m98 they were available for mac os (but don't work) , now they are not available on mac os x. HinTak/skia-m1xx-python@3d69f9b so I expect those should pass on Linux (and it does on my machine) , and Windows (let's see), but fails on mac os. So those REVISIT would get converted to "if (mac os) skip/xfail" eventually - hence running the tests on CI on my playground repo.

@HinTak HinTak mentioned this pull request Dec 1, 2023
@HinTak HinTak requested a review from kyamagu December 4, 2023 02:09
@HinTak
Copy link
Collaborator Author

HinTak commented Dec 4, 2023

@kyamagu added README.120 , and ready to go. I have an extra 14 tests in my m120-try branch but the extra tests require extra test fonts not in skia/resources/fonts (AFAIC - I could check that again eventually) so I don't want to add them here. Let me know if you want them (one is a 10MB font file...) - otherwise I'll just keep them as my own private addition. Ie. The new API are still tested, but not as often as the other tests officially added here.

A bit curious about the state of vulkan support at m87 - mostly I am just re-enabling routines and tests already in m87 which became optional/ dependent on actual skia compiled option in m98. So I think they were present but not necessarily functional, and the tests themselves weren't/aren’t really testing the correct things, as in how people use those practically.

I'd say add more vulkan tests but also occasionally backport the tests to m87 and run them on CI and see how they did in m87.

Upstream has a sentence saying vulkan build is feature comparable to GL build but somewhat buggy - as mentioned in the other issue, Linux has both, Windows seems to be vulkan only (at least on Github CI), mac with newer python is opengl only, mac with older python have neither - this 4 different behavior is according to current CI.

The commit which disabled it, and the one which made it start to work, are below:

commit 15dd24a
Author: Hin-Tak Leung <[email protected]>
Date:   Mon Jul 31 21:50:05 2023 +0100

    These attributes are not (yet) available under m116; may never be. RE-VISIT!!!

commit 0a62144
Author: Hin-Tak Leung <[email protected]>
Date:   Tue Aug 1 04:43:05 2023 +0100

    m113: `SkImage::CompressionType` has been renamed to `SkTextureCompressionType`

    Milestone 113:

      * `SkImage::CompressionType` has been renamed to `SkTextureCompressionType` and moved to
        `include/core/SkTextureCompressionType.h`
Copy link
Owner

@kyamagu kyamagu left a comment

Choose a reason for hiding this comment

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

LGTM. It is okay to leave release notes in the root, but I prefer having those notes somewhere else for housekeeping at some point in the future.

README.md Show resolved Hide resolved
@kyamagu
Copy link
Owner

kyamagu commented Dec 4, 2023

@HinTak Thanks for the tough work. As for the Vulkan support in m87, the implementation was experimental and never expected to work as expected. Those were there to check the possibility of supporting multiple backends. I appreciate your rework on those APIs

@HinTak HinTak merged commit 8025331 into kyamagu:main Dec 5, 2023
14 checks passed
@HinTak HinTak deleted the m120-public branch December 5, 2023 00:51
@HinTak HinTak restored the m120-public branch December 5, 2023 00:52
@HinTak
Copy link
Collaborator Author

HinTak commented Dec 5, 2023

As of now,
2072 passed, 108 skipped, 13 xfailed

Compared to v87.5
2171 , 27, 8

@HinTak
Copy link
Collaborator Author

HinTak commented Dec 5, 2023

Damn - it is "RTL" languages in the release note!

@HinTak HinTak deleted the m120-public branch May 22, 2024 18:42
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.

2 participants