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

M121 public #222

Merged
merged 28 commits into from
Jan 26, 2024
Merged

M121 public #222

merged 28 commits into from
Jan 26, 2024

Conversation

HinTak
Copy link
Collaborator

@HinTak HinTak commented Dec 7, 2023

The changes are all straight-forward. The font tests pass, but the change of what Typeface.MakeDefault does (from a normal default to empty) as from upstream may need rethinking / emulate properly, or with a large warning in the release notes. It will likely break somebody's code, if they assume MakeDefault will give them Times/Arial/Noto, instead of a font with no glyphs.

Re-think on that.

…-g746dbd8c61..canvaskit/0.38.2-2397-g349c1179c4

$ git whatchanged canvaskit/0.38.2-2393-g746dbd8c61..canvaskit/0.38.2-2397-g349c1179c4
commit 349c1179c43ef46f2804404952b9460dc007d76a
Author: John Stiles <johnstiles@google.com>
Date:   Sat Nov 25 22:41:31 2023 -0500

    Use SkToInt to avoid warning in Flutter roll.

    The Flutter roll was failing due to -Wsign-compare.

    Bug: chromium:1505053
    Change-Id: Id12876f6f97682466f19b56cfa562366380f27cb
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/783036
    Auto-Submit: John Stiles <johnstiles@google.com>
    Commit-Queue: Brian Osman <brianosman@google.com>
    Reviewed-by: Brian Osman <brianosman@google.com>
    (cherry picked from commit 0eea0b277d7d35e4c2612646d7dfe507341e337e)
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/783297
    Reviewed-by: John Stiles <johnstiles@google.com>
    Commit-Queue: John Stiles <johnstiles@google.com>
    Auto-Submit: Brian Osman <brianosman@google.com>

:100644 100644 002618a118 36e9685a75 M	src/gpu/ganesh/ops/DrawMeshOp.cpp

commit 20006736edb40de72dd5a7d5cce2ce877c9460bd
Author: John Stiles <johnstiles@google.com>
Date:   Fri Nov 24 09:40:11 2023 -0500

    Avoid combining extremely large meshes.

    Bug: chromium:1505053
    Change-Id: I42f2ff872bbf054686ec7af0cc85ff63055fcfbf
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/782936
    Commit-Queue: Michael Ludwig <michaelludwig@google.com>
    Reviewed-by: Michael Ludwig <michaelludwig@google.com>
    Auto-Submit: John Stiles <johnstiles@google.com>
    (cherry picked from commit 6169a1fabae1743709bc9641ad43fcbb6a4f62e1)
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/782577
    Commit-Queue: Brian Osman <brianosman@google.com>
    Auto-Submit: Brian Osman <brianosman@google.com>
    Reviewed-by: John Stiles <johnstiles@google.com>
    Commit-Queue: John Stiles <johnstiles@google.com>

:100644 100644 5ea123d5fb 002618a118 M	src/gpu/ganesh/ops/DrawMeshOp.cpp

commit 48311c8e39d1892245dfd5ecc1f557a14825ec69
Author: Jim Van Verth <jvanverth@google.com>
Date:   Tue Nov 7 15:12:39 2023 -0500

    Fix colorspace transform with multitexture color text.

    Color text glyphs can be stored across multiple atlas textures, which
    requires sampling from one or another texture. However, if we use
    multiple appendTextureLookup calls with the same colorspace transform
    helper, it ends up generating multiple copies of its helper functions.

    Instead, we will pick our texture, sample its value, and apply the
    colorspace transform to the result outside the if-then-else block.

    Bug: b/309422936
    Bug: chromium:1499098
    Change-Id: I468fe3ef323081b22537427f1f7a894e8a52073c
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/775397
    Reviewed-by: Brian Osman <brianosman@google.com>
    Commit-Queue: Jim Van Verth <jvanverth@google.com>
    (cherry picked from commit f91d39395e85b5803dd6231ba111e59955a0b012)
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/779337

:100644 100644 c06f830e4c 1e9ae1b5f1 M	src/gpu/ganesh/effects/GrAtlasedShaderHelpers.h
:100644 100644 d575b18972 60ede17f3e M	src/gpu/ganesh/effects/GrBitmapTextGeoProc.cpp

commit ad9fc9ee5fb0bfd199b2db596f823395431f53c6
Author: Michael Ludwig <michaelludwig@google.com>
Date:   Wed Nov 8 13:14:09 2023 -0500

    GrBlurUtils: Subset on low-res pixel boundary, not float coords

    The low-res blurred image was rendered into the rounded-out coords,
    and any decal-tiling should occur beyond their boundaries. By using
    the original scaled coords as the subset boundary, we'd introduce
    transparency at the edge when re-expanding.

    Previously this was never visible because of quirks in how
    SkBlurImageFilter requested its output (always padded with the blur
    radius, even if it were to then be cropped).

    While the changes to the subsetting logic accounted for 90% of the
    visible seams when testing in chrome (and all the visible seams when
    running in viewer), it was also necessary for the downscaled blur
    to write one extra pixel of padding that could be sampled from when
    doing the re-expand.

    Bug: chromium:1500021
    Change-Id: I18c147270db7b42568f4e7034b13c2dc4ab19e4a
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/775517
    Reviewed-by: Robert Phillips <robertphillips@google.com>
    Commit-Queue: Michael Ludwig <michaelludwig@google.com>
    (cherry picked from commit e585bb68893be9e4f771f1da6e850b3f587ba9c4)
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/776156
    Commit-Queue: John Stiles <johnstiles@google.com>
    Auto-Submit: Michael Ludwig <michaelludwig@google.com>
    Reviewed-by: John Stiles <johnstiles@google.com>
    (cherry picked from commit 96270e0be6b3acdbd59e5931a122be16958c9c1e)
    Reviewed-on: https://skia-review.googlesource.com/c/skia/+/776957
    Commit-Queue: Robert Phillips <robertphillips@google.com>

:100644 100644 49ca7459ac 313f5c4ce5 M	gm/blurs.cpp
:100644 100644 cf3974bb5f e3dee58cc1 M	src/gpu/ganesh/GrBlurUtils.cpp
Milestone 121
-------------
  * Deprecated `GrMipmapped` and `GrMipMapped` alias have been removed in favor of `skgpu::Mipmapped`.
$ diff -u skia-m120/include/core/SkSurfaceProps.h skia-m121/include/core/SkSurfaceProps.h
--- skia-m120/include/core/SkSurfaceProps.h	2023-12-01 00:02:47.000000000 +0000
+++ skia-m121/include/core/SkSurfaceProps.h	2023-12-06 02:01:54.000000000 +0000
@@ -59,8 +59,6 @@
         // Currently this only impacts GPU backends
         kAlwaysDither_Flag              = 1 << 2,
     };
-    /** Deprecated alias used by Chromium. Will be removed. */
-    static const Flags kUseDistanceFieldFonts_Flag = kUseDeviceIndependentFonts_Flag;

     /** No flags, unknown pixel geometry. */
     SkSurfaceProps();
…ypeface::MakeEmpty

The behavior is different:
    Returns the default normal typeface, which is never nullptr.
vs
    Returns a non-null typeface which contains no glyphs.

It started to be behind a "#if !defined(SK_DISABLE_LEGACY_DEFAULT_TYPEFACE)" in m120.
"SK_DEFAULT_TYPEFACE_IS_EMPTY" and "SK_DISABLE_LEGACY_DEFAULT_TYPEFACE" are defined in m121.

Milestone 120
-------------
  * `SkTypeface::MakeFromName`, `SkTypeface::MakeFromFile`, `SkTypeface::MakeFromStream`, and
    `SkTypeface::MakeFromData` are deprecated and will be removed eventually. These should be replaced
    with calls directly to the SkFontMgr that can provide the appropriate typefaces.

    `SkTypeface::MakeDefault()` has been deprecated. Soon it will return an empty typeface and
    eventually be removed.

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

  * Added a new public type, `SkColorTable`, to own the lookup tables passed into `SkColorFilters::Table`, which allows clients and the returned `SkColorFilter` to share the table memory instead of having to duplicate it in any wrapper types that lazily create Skia representations.
  * The deprecated `SkTableColorFilter` class and its methods have been removed. Clients should use
    `SkColorFilters::Table` and `SkColorFilters::TableARGB` (defined in include/core/SkColorFilter.h).

Fixes kyamagu#227

See also:

commit e775fcb
Author: Hin-Tak Leung <htl10@users.sourceforge.net>
Date:   Thu Oct 26 00:45:50 2023 +0100

    Emulate pre-m116 SkTableColorFilter class and SkTableColorFilter::Make
@HinTak
Copy link
Collaborator Author

HinTak commented Jan 18, 2024

@kyamagu skia m122 should be out in a few days so this is probaby ready to go soon. Still missing a "README.m121" etc, but code-wise this is okay. The number of skipped tests has just gone under 100, so it is only 70-ish worse than m87 now. I'll add a README.m121 soon. As for the default typeface case, it is difficult to tell what was the default before (it differs from system to system), but I think the old behavior is probably the same as skia.Typeface("") (note the empty string, equivalent toskia.Typeface.MakeFromName(""), fonts with any name, since "" matches anything). I think most people should use a non-empty string e.g. skia.Typeface("Roman") (or "Times" or "Sans"). I think telling people that if they really don't care what font is used, they should convert skia.TypeFace() to Skia.Typeface("") isn't too troublesome either. Do you want skia.Typeface() to behave like skia.Typeface("") (to avoid breaking people's code) or align with upstream's recommendation i.e. make people choose a font?

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 18, 2024

@kyamagu #148 's example relies on a "non-empty" default font.

@HinTak HinTak requested a review from kyamagu January 18, 2024 01:15
@kyamagu
Copy link
Owner

kyamagu commented Jan 18, 2024

@HinTak Thanks!

As for skia.Typeface(), I am okay with either case: 1) keeping the old behavior with a deprecation warning, or 2) switching to the upstream behavior with a specific deprecation error message. What do you think?

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 18, 2024

@kyamagu it is kind of a difficult choice - upstream obviously does not want people to assume they can get a font/typeface that draws something with null input, but I expect many existing skia-python users do (as in #148 ). Also I suspect very few skia-python users actually want the MakeEmpty behavior (a valid font/typeface that draws nothing). Also slightly complicated is, do people call the default constructor THEN make changes to it, and how would it work if it starts with "match anything"? There is also the 3rd option, which is withdrawing skia.Typeface() altogether to make sure people change their code to skia.Typeface("") if they really don't care. So we really have 3 choices on this (1) make it behave like make Empty (very few users want this), (2) make it behave like "", (3) removing skia.typeface() altogether to force people to change their code to skia.typeface(""). I am leaning towards (2) or (3). (2) is probably least complaints from users. Oh, I probably should add an explicit "makeempty()" just in case there are people who really want that. (Difficult to think why anybody wants a font/typeface which draws nothing...). I think going for (2) but saying we'll eventually do (3) would be a good idea.

@kyamagu
Copy link
Owner

kyamagu commented Jan 18, 2024

@HinTak Sure, let's take option 2: keeping the backward compatibility with a deprecation warning. It is possible to explicitly emit python warning (warnings.warn) or add a deprecation note in the docstring. Either is fine.

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 18, 2024

@kyamagu cool. I'll look into adding the warning and including it in this pull.

@kyamagu
Copy link
Owner

kyamagu commented Jan 18, 2024

@HinTak Thanks! Perhaps calling warnings.warn or logging.warning via pybind11 API is sufficient for visible user notice.

The closest is SkTypeface::MakeFromName("", SkFontStyle()), but that's also

    Deprecated: call SkFontMgr::matchFamilyStyle or SkFontMgr::legacyMakeTypeface

with a "#if !defined(SK_DISABLE_LEGACY_FONTMGR_REFDEFAULT)" already, so we go
directly to use SkFontMgr::legacyMakeTypeface().

Use member function from SkFontMgr::RefDefault() instance object
@HinTak
Copy link
Collaborator Author

HinTak commented Jan 20, 2024

Okay, the latest does this (4 times, with 4 tests):

tests/test_font.py:430
  ..../tests/test_font.py:430: DeprecationWarning: "Default typeface" is deprecated upstream. Please specify name/file/style choices.
    (skia.Typeface.MakeDefault(), 10),

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.

Just be careful regarding the future maintenance effort related to the freetype patch.

@kyamagu
Copy link
Owner

kyamagu commented Jan 20, 2024

@HinTak Many thanks!

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 25, 2024

@kyamagu thanks for the heads-up on the font patch. I am aware of it being flaky and font-related development is an active area in recent skia - that's what brought me to skia-python, actually. Was wanting to have anothe look at #148 before merging.
I just checked the logs, apparently I have a few re-enabled tests not yet included. Just pushed, will paste new result when close.

ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.27s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.21s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.11s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.08s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 3.69s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 3.77s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.50s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.11s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.91s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.94s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.70s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.21s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1842 passed, 347 skipped, 4 xfailed, 4 warnings in 4.86s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1842 passed, 347 skipped, 4 xfailed, 4 warnings in 4.72s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 6.31s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 5.07s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 4.31s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 5.21s 
ubuntu-22.04 (aarch64) for cp3712:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.65s 
ubuntu-22.04 (aarch64) for cp3812:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.41s 
ubuntu-22.04 (aarch64) for cp3912:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 40.92s 
ubuntu-22.04 (aarch64) for cp31012:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.56s 
ubuntu-22.04 (aarch64) for cp31112:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 38.10s 
ubuntu-22.04 (aarch64) for cp31212:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 38.29s 

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 25, 2024

looking at the re-enable tests, I realized skia.Font() and skia.Font(None, ....) should trigger deprecation warnings too. The latter is a bit tricky (but #148 is basically its test) so I shall add those two before merging.

@HinTak
Copy link
Collaborator Author

HinTak commented Jan 26, 2024

Skipped is below 100 now.

ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.61s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.54s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.47s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.41s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.01s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.15s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.49s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.07s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.86s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.10s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.78s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.85s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1850 passed, 339 skipped, 4 xfailed, 4 warnings in 15.59s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1850 passed, 339 skipped, 4 xfailed, 4 warnings in 15.10s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 16.08s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 14.68s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 14.81s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 13.50s 
ubuntu-22.04 (aarch64) for cp3712:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 42.18s 
ubuntu-22.04 (aarch64) for cp3812:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 42.62s 
ubuntu-22.04 (aarch64) for cp3912:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 41.84s 
ubuntu-22.04 (aarch64) for cp31012:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 41.24s 
ubuntu-22.04 (aarch64) for cp31112:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 38.27s 
ubuntu-22.04 (aarch64) for cp31212:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 39.01s 

I am adding deprecation warnings to Font() and Font(None,...) too, as upstream don't return a practically useable (i.e. draw something) Font with them lately. This pushes the warnings up to 20+ as skia.Font() is used quite often in the tests...

@kyamagu
Copy link
Owner

kyamagu commented Jan 26, 2024

BTW, python 3.7 is already EOL. Feel free to drop the support

@HinTak HinTak merged commit 2e41f21 into kyamagu:main Jan 26, 2024
14 checks passed
@HinTak
Copy link
Collaborator Author

HinTak commented Jan 27, 2024

@kyamagu the new release is having a 403 error reaching pypi - any authentication change recently? Maybe actions needs updating (it says "legacy")?

ERROR HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/
Invalid or non-existent authentication information. See
https://pypi.org/help/#invalid-auth for more information.

About python 3.7 - cutting it doesn't really save build time, so probably can leave it alone until something breaks...

@kyamagu
Copy link
Owner

kyamagu commented Jan 27, 2024

@HinTak Will look into it

@HinTak HinTak deleted the m121-public branch April 30, 2024 16:07
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