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

Fix the GetKerningPairAdjustments API. #2858

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

pdjonov
Copy link
Contributor

@pdjonov pdjonov commented May 14, 2024

Description of Change

The SkTypeface::getKerningPairAdjustments API states:

If count is non-zero, then the glyphs parameter must point to at least [count] valid glyph IDs, and the adjustments parameter must be sized to at least [count - 1] entries. If the method returns true, then [count-1] entries in the adjustments array will be set. If the method returns false, then no kerning should be applied, and the adjustments array will be in an undefined state (possibly some values may have been written, but none of them should be interpreted as valid values).

However, SkTypeface.GetKerningPairAdjustments returns count entries and further in the case where getKerningPairAdjustments returns false it may return invalid values to the caller. This PR:

  • Adds a Span-based overload of GetKerningPairAdjustments that:
    • correctly expects glyphs.Length - 1 output entries (but will accept and ignore more),
    • returns true if SkTypeface::getKerningPairAdjustments wrote valid data into the output span,
    • otherwise clears the output span to zero so that even if invalid data was written, the caller won't see it and will get sensible results even when ignoring the return value.
  • Rewrites the original GetKerningPairAdjustments in terms of the new overload.
    • For backwards compatibility, an extra zero element is returned at the end of the array.
    • However, partial/invalid results will no longer be returned (the caller will receive zeroes instead).

Some typefaces are known to never support kerning. Calling this method with all zeros (e.g. getKerningPairAdustments(NULL, 0, NULL)) returns a boolean indicating if the typeface might support kerning. If it returns false, then it will always return false (no kerning) for all possible glyph runs. If it returns true, then it may return true for somne glyph runs.

This patch adds a HasGetKerningPairAdjustments that callers can use to query whether the font contains any kerning information or whether GetKerningPairAdjustments will always return zeroes.

API Changes

Added:

  • public bool SkTypeface.HasGetKerningPairAdjustments { get; }
  • public bool GetKerningPairAdjustments (ReadOnlySpan<ushort> glyphs, Span<int> adjustments)

Behavioral Changes

public int[] GetKerningPairAdjustments (ReadOnlySpan<ushort> glyphs) will now return zeroes in places where it previously returned partial/invalid data that the Skia documentation says to ignore.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

Apologies if this isn't quite up to the project standards. I was initially going to just submit a bug report, but then I thought it might be nicer (and likelier to actually get fixed) if I made a quick PR instead of just complaining. I fully acknowledge that I'm not invested enough in this project to learn all of the conventions properly, and I apologize if this wastes anyone's time cleaning up what I've misunderstood. Y'all can do with this patch whatever you will.

I just think it'd be nice if I could be rid of this abomination:

private unsafe delegate bool SkApi_GetKerningPairAdjustments(IntPtr typeface, UInt16* glyphs, Int32 count, Int32* adjustments);
private static SkApi_GetKerningPairAdjustments s_GetKerningPairAdjustments;

private static SkApi_GetKerningPairAdjustments GetKerningPairAdjustmentsFunc()
{
    if (s_GetKerningPairAdjustments == null)
    {
        var apiBindingsType = typeof(SKTypeface).Assembly.GetType("SkiaSharp.SkiaApi");
        var apiFunc = apiBindingsType.GetMethod("sk_typeface_get_kerning_pair_adjustments", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic);
        s_GetKerningPairAdjustments = apiFunc.CreateDelegate<SkApi_GetKerningPairAdjustments>();
    }

    return s_GetKerningPairAdjustments;
}

private static unsafe bool HasKerningPairAdjustments(SKTypeface typeface)
{
    var func = GetKerningPairAdjustmentsFunc();
    return func(typeface.Handle, null, 0, null);
}

private static unsafe void GetKerningPairAdjustments(SKTypeface typeface, ReadOnlySpan<ushort> glyphs, Span<int> adjustments)
{
	Debug.Assert(glyphs.Length >= 2 && adjustments.Length == glyphs.Length - 1);

	var func = GetKerningPairAdjustmentsFunc();

	bool res;
	fixed (ushort* g = glyphs)
	fixed (int* a = adjustments)
		res = func(typeface.Handle, g, glyphs.Length, a);

	if (!res)
		adjustments.Clear();
}

@pdjonov
Copy link
Contributor Author

pdjonov commented May 14, 2024

@dotnet-policy-service agree

@pdjonov pdjonov marked this pull request as ready for review May 14, 2024 22:08
@pdjonov
Copy link
Contributor Author

pdjonov commented May 14, 2024

I don't understand why the tests are failing. The stuff in the logs doesn't look like stuff that I've broken, so I'm taking this PR out of draft.

Again, I made this PR since it seemed a better thing to do than just opening a ticket and hoping someone else would write code for me. But I don't have time to learn what's going on with Run the bootstrapper for tests-android and so on. Do whatever y'all want with the patch. (And sorry again if you'd have preferred a ticket to a half-baked PR.)

@mattleibow
Copy link
Contributor

@pdjonov no, thanks for this PR. It looks very good.

CI is a bit temperamental and I am still trying to figure out why macos and Android keep failing the first run. Android has additional issue with the emulators not always booting and macOS has random test crashes.

But, if the other test platforms all pass then it is highly likely it is just a random failure.

Also, the test were all failing before since main had been broken for some time. But it is all fixed now.

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the docs.

I looked at the skia source and it seems only the SkTypeface_FreeType (FreeType-based typefaces) support this feature. This probably means just Linux-based OSes will support this (Linux, Android and WASM)

Do you perhaps have a test for this? Not sure if you have a font that you can use to check some things and then just make sure the values get passed around correctly?

@mattleibow
Copy link
Contributor

Going to merge this as tests are hard for this scenario and we can add them later. Even skia does not have tests...

@mattleibow mattleibow merged commit 7c5b9ef into mono:main Jun 3, 2024
1 of 2 checks passed
@mattleibow mattleibow added the backport/release/2.x Backport this PR to release/2.x label Jun 3, 2024
github-actions bot pushed a commit that referenced this pull request Jun 3, 2024
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
(cherry picked from commit 7c5b9ef)
mattleibow pushed a commit that referenced this pull request Jun 18, 2024
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
(cherry picked from commit 7c5b9ef)

Co-authored-by: Phill Djonov <phill@vec3.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/release/2.x Backport this PR to release/2.x community ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants