Support SkFontArguments::Palette #297

khaledhosny · 2025-01-02T02:30:25Z

To allow selecting color fonts palette.

HinTak · 2025-01-02T03:10:11Z

I have let CI started, but I wish you have got in touch earlier, as I have already got some of this overlapping in my hard disk already. See #259

src/skia/Font.cpp

tests/test_font.py

src/skia/Font.cpp

tests/test_font.py

HinTak · 2025-01-02T07:35:40Z

@khaledhosny ci failed on mac os (which has python 3.13.1). You need to pull in the fix for #295 , which is #296 , to pass ci.

HinTak · 2025-01-02T17:24:38Z

FWIW, even namespacing these classes under FontArguments is troublesome - there is aSkFontArguments class upstream in core, and a skia::textlayout::FontArguments class in the paragraph module.

khaledhosny · 2025-01-02T17:50:50Z

I’m not sure I get the namespacing comments. The classes are already nested in Python, skia.FontArguments.Palette(), skia.FontArguments.Palette.Overrides(), and skia.FontArguments.Palette.Override(), so I’m not sure what should I do other than that.

HinTak · 2025-01-02T22:34:16Z

This #298 isn't how it is on my hard disk (I have neighbouring code with does something else, so there are collisions while cherry-picking), nor ready for used or tested, but I think if it works, together with your pull, should mostly just be the other half of allowing one to address #259 , loading COLRv1 fonts with custom / non-default palettes.

HinTak · 2025-01-02T22:40:21Z

With the two together, you should be able to do something like this to load a colrv1 font with custom palette:

# Read font file into fontdata
scanner = skia.FontScanner()
# do stuff with FontArguments.Palette
newface = scanner.MakeFromStream(fontdata, fontargs)

khaledhosny · 2025-01-02T23:11:55Z

With this PR I can already select non-default palettes and override their colors in COLR (v0 and v1) fonts, but I’m testing only with FreeType fonts:

fontMgr = skia.FontMgr.New_Custom_Empty()

face1 = fontMgr.makeFromFile(fontpath)
font1 = skia.Font(face, face.getUnitsPerEm())

fontArgs2 = skia.FontArguments()
fontArgs2.setPalette(1)
face2 = face1.makeClone(fontArgs2)
font2 = skia.Font(face2, size)

fontArgs3 = skia.FontArguments()
palette3 = skia.FontArguments.Palette(
    1,
    skia.FontArguments.Palette.Overrides(
        [
            skia.FontArguments.Palette.Override(0, skia.ColorMAGENTA),
            skia.FontArguments.Palette.Override(6, skia.ColorBLUE),
            skia.FontArguments.Palette.Override(7, skia.ColorGREEN),
            skia.FontArguments.Palette.Override(8, skia.ColorCYAN),
        ]
    ),
)
fontArgs3.setPalette(palette3)
face3 = face1.makeClone(fontArgs3)
font3 = skia.Font(face3, size)

string = "\u06DD"
text1 = skia.TextBlob.MakeFromShapedText(string, font1)
text2 = skia.TextBlob.MakeFromShapedText(string, font2)
text3 = skia.TextBlob.MakeFromShapedText(string, font3)

bounds = text.bounds()

width = math.floor(bounds.width() * 3)
height = math.floor(bounds.height())

surface = skia.Surface(width, height)
with surface as canvas:
    canvas.drawColor(skia.ColorWHITE)
    paint = skia.Paint()
    canvas.drawTextBlob(text1, -bounds.x(), -bounds.y(), paint)
    canvas.drawTextBlob(text2, bounds.width() - bounds.x(), -bounds.y(), paint)
    canvas.drawTextBlob(text3, bounds.width() * 2 - bounds.x(), -bounds.y(), paint)

HinTak · 2025-01-02T23:56:47Z

Interesting. Thanks for the code - some of it can go into the tests directory. (skia's resources/fonts/ is available for tests).

FWIW, I have some incomplete code which supposedly let you do face.getFontDescriptor().setPaletteIndex(N) (and short-circuiting to face.setPaletteIndex(N)) to switch. Does not work (yet).

HinTak · 2025-01-03T11:19:13Z

If the asserts are not logically independent I.e. you just want an exact match of a composite structure, it might be clearer to do assert x and y and z a single mulit-line assert, any mis-match is considered a failure. If there are multiple independent/stages ways of failure, you do a fixture then multiple tests.

HinTak · 2025-01-03T11:36:56Z

I think face.getFontDescriptor().setPaletteIndex(N) not working is an upstream bug but I have not gotten round to file it yet. Basically the paletteindex is stored in an intermediate structure but never passed onto the font renderer. In the case of colrv1, freetype never gets it set. But similar comments applies to the colrv0 path in the directwrite rendering path too.

HinTak · 2025-01-03T11:49:00Z

assert isinstance... is typically one per test, and the object itself done as a re-used fixture. See the other tests.

HinTak · 2025-01-03T11:51:24Z

See for example the fixture at

skia-python/tests/test_paragraph.py

Line 12 in db7d7fc

@pytest.fixture(scope='session')

and the init test below.

HinTak · 2025-01-03T11:53:11Z

And all the fxtures finally get passed to the usage test at

skia-python/tests/test_paragraph.py

Line 45 in db7d7fc

    
           def test_Paragraph_linebreak(paragraph_builder, textlayout_text_style, textlayout_font_collection, paragraph_style):

To allow selecting color fonts palette.

khaledhosny · 2025-01-03T12:55:16Z

Interesting. Thanks for the code - some of it can go into the tests directory. (skia's resources/fonts/ is available for tests).

That would be nice, but testing palette overrides will requiring checking the rendered output (possibly against a reference image), but I don’t see any tests doing something like this.

HinTak · 2025-01-03T19:56:34Z

That would be nice, but testing palette overrides will requiring checking the rendered output (possibly against a reference image), but I don’t see any tests doing something like this.

Well, setting and getting it back should work. It is probably possible to render to a surface and detect it is "mostly red", say, just by count the red pixels. I have a test in the paragraph part, which used to have trouble with rendering "\n" as a new line rather than .notdef. The test is simply that the rendered area should be tall than wide, whichever the default font is.

HinTak · 2025-01-05T00:06:59Z

tests/test_font.py

@@ -46,6 +46,44 @@ def test_FontArguments_setCollectionIndex(fontarguments):
    fontarguments.setCollectionIndex(0)




I think there needs to be a dozen more tests for each of the new properties/methods of the new classes added.

HinTak · 2025-01-05T01:23:27Z

src/skia/Font.cpp

+            return std::vector<Override>(
+                self.overrides, self.overrides + self.overrideCount);
+        },
+        &SetPaletteOverrides)


Don't you need a keep_Alive policy here too? On the set method.

HinTak · 2025-01-05T01:27:54Z

I am starting to see the variationposition code from which this modelled from a bit buggy about life times and zero length arguments...

HinTak · 2025-01-08T15:05:13Z

tests/test_font.py

+    assert palette.index == index
+    assert len(palette.overrides) == len_overrides
+    assert palette.overrideCount == len_overrides
+    assert repr(palette) == f"Palette(index={index}, overrideCount={len_overrides})"


I would probably not have a repr test - since the __repr__ method itself is somewhat a personal choice and subjected to changes later too.

HinTak · 2025-01-08T15:05:57Z

tests/test_font.py

+    assert palette.overrideCount == len_overrides
+    assert repr(palette) == f"Palette(index={index}, overrideCount={len_overrides})"
+    for i, override in enumerate(palette.overrides):
+        assert repr(override) == f"Override(index={override.index}, color={override.color})"


Ditto on testing the precise form of repr outcome.

HinTak · 2025-01-08T15:09:50Z

src/skia/Font.cpp

+        Specify a palette to use and overrides for palette entries.
+        )docstring",
+        py::arg("palette"))
+    .def("setPalette",


There are some setPaletteIndex/getPaletteIndex methods in other classes. Perhaps do both?

I also wonder whether this should be a read/write property instead.

HinTak · 2025-01-08T15:11:53Z

You added 3 classes, so there should be 3 init tests, in addition to the class method tests...

HinTak · 2025-01-10T03:56:51Z

src/skia/Font.cpp

+                self.overrides, self.overrides + self.overrideCount);
+        },
+        &SetPaletteOverrides)
+    .def_readwrite("index", &SkFontArguments::Palette::index)


PaletteIndex might be a better choice of name, given font argument might have other indices.

HinTak · 2025-01-10T16:27:03Z

I have had a better look at the example code. I think this pull is substantially incomplete (in addition to all the detailed comments so far) - to be of any use to somebody who isn't the font author, it really needs the face.getFontDescriptor().get*Palette*Override* method to find out what is settable (or use another tool to examine the font to find out what is settable). Hence this is of no use without those.

So this is not going to be merged without substantially additional work.

HinTak · 2025-01-10T17:22:09Z

I see your example must have used one of your own authored arabic fonts (Amiri Quran?). That's fair enough, but an example with one of the English fonts would be preferred; as well as some tests with the font files in skia/resources/fonts/ .

I"d have preferred that this stays open until you or somebody else improves on it. Nevermind.

khaledhosny · 2025-03-05T01:01:38Z

Well, I added bindings for existing Skia APIs, but you are asking for palette discoverability APIs that Skia does not currently have as a prerequisite of merging this PR, so I don’t know what I was supposed to do.

HinTak · 2025-03-05T02:00:40Z

I wrote earlier that palette discovery looks to be possible binding around face.getFontDescriptor().get*Palette*Override* (the equvalent c++ code behind).

HinTak · 2025-03-05T02:02:05Z

I have the python bits for face.getFontDescriptor()... but I haven't got time to finish it yet.

khaledhosny · 2025-03-05T15:52:32Z

I wrote earlier that palette discovery looks to be possible binding around face.getFontDescriptor().get*Palette*Override* (the equvalent c++ code behind).

This does not query the font, it returns whatever the user have set.

HinTak · 2025-03-05T15:59:57Z

Yes, I believe that is an upstream bug. Skia-python is not against carry interim fixes for upstream bugs, before/during they are being accepted.

khaledhosny · 2025-03-05T16:13:52Z

Skia has no code whatsoever to query font palettes, so again you blocked this PR on developing code that upstream does not have. If Skia is fine for its users with its API as it is, why can’t this Python bindings.

HinTak · 2025-03-05T16:18:07Z

Example - #290 - it is an upstream bug for which a proposed fix was posted a while ago by somebody else, and I intend to take it on here at some point, even if upstream hasn't accepted it yet. That fix helps avoiding people filing bug reports with spurious and misleading information here, so we should have it even if upstream hasn't accepted it.

HinTak · 2025-03-06T01:02:32Z

Note I am not "blocking" this PR. I am merely saying it is incomplete / need more work, because as it is, it requires special input font files and example uses of the additional code requires intimate knowledge of the special test font files. Until it becomes more generally useful to other input font files, the new code is only useful to you, the supplier/author of the special input font file.

IMHO, PR (in general, not specifically for this project) are just NOT merged by default, and nobody (including me) should assume any pull submitted will be merged. It just isn't, and shouldn't.

HinTak · 2025-03-06T01:09:13Z

Also, FWIW, other pulls have stayed opened for months / years without a demand to merge. Typically mine get merged here after after 2 months when I say it is ready. You are demanding a merge or withdrawal in 10 days?

khaledhosny · 2025-03-06T02:06:02Z

So this PR is incomplete because Skia, the upstream, is missing an API you think it important to use the existing Skia API that this PR exposes to Python users?

CSS allow selecting/overriding font palettes (just like the API this PR binds), but browsers lack any API to query font palettes, so CSS support for font palettes is unusable, too?

Skia/CSS allow setting OpenType font features, but lack APIs to query fonts for supported features, so Skia/CSS support for font features is unusable, too?

To be frank, it feels like you are just making things up, and while no one should expect their code contribution to be accepted, you should have stated your (made up) objection up front, instead of wasting my time with minute details while then later stating this PR can never be merged until Skia develop some API you think are a prerequisite for using this existing API.

HinTak · 2025-03-06T04:44:44Z

FWIW, #181 stayed open for 18 months, before it was superceded and closed. Personally I think I would prefer to spend my time (and you spend yours) actually coding, than continuing this exchange. Sorry.