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.