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

Use newer SkiaSharp APIs #216

Merged
merged 8 commits into from
Apr 16, 2024
Merged

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Apr 5, 2024

I am looking to make things more compatible with SkiaSharp 3.x but still retaining the compatibility with 2.x.

This issue has a bit more info and some links: mono/SkiaSharp#2790

Basically, we have added some of the new 3.x APIs into 2.x so that we can all start using those to ease migration. Also, I have some secret 3.x APIs that will make this work. The linked issue has some tools that I have been using to detect cases where things will break.

I believe that I have fixed all the breaks for Svg.Skia so you will not need to target 3.x to be compatible with 3.x. This may not be the case for all libraries in the universe, but Svg.Skia seems to only touch the breaks in a few places and we can fix that with an update.

I will add some additional comments to the code with reasons.

@mattleibow mattleibow marked this pull request as draft April 5, 2024 19:23
@mattleibow
Copy link
Contributor Author

Converting to draft since the SkiaSharp is still a preview. I am hoping to push a stable soon and we can all win.

Copy link
Contributor Author

@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.

Added some comments about things. Right now 3.x may still not work as we are waiting for this PR to land in 3.x: mono/SkiaSharp#2789

However, if my tool is to be believed, there were about 20 APIs that needed changing, and 18 were the image filter crop rects. Switching to SKRect is generally better anyways as the other properties are not actually used by Svg.Skia.

<PackageReference Include="SkiaSharp" Version="2.88.6" />
<PackageReference Include="SkiaSharp" Version="2.88.8-preview.1.1" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the NuGet is just a preview, but once I manage to get all the other things ready and it lives for a few days and gets a few download I will push it stable.

Comment on lines +55 to +56
public static SKImageFilter CreateShader(SKShader shader, bool dither, CropRect? cropRect = null)
=> new ShaderImageFilter(shader, dither, cropRect);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shader filter is the new filter that is preferred over paint filter. According to Google: google/skia@7d0f853

SkImageFilters::Paint did not use every slot of the SkPaint, with only
its color, alpha, color filter, and shader having a meaningful effect on
the image filter result. It was always blended into a transparent dst,
so blend mode wasn't very relevant, and it was always filled to whatever
required geometry, so stroke style, path effect, and mask filters were
ignored or not well specified.

Color, alpha, and color filter can all be combined into an SkShader, so
a more constrained SkImageFilters::Shader provides the same useful
capabilities without as many surprises.

The Paint filter right now may do extra things, but it appears that all the things can be done via merged filters. For example, a fill image/color can be a done with an Image/Color shader. I am not too familiar with how the image filters get created in Svg.Skia and all the potential caveats so I did not feel too comfortable switching just yet.

In 2.x, the Shader filter is really a paint filter with the shader set so should be identical to 3.x Shader. In 3.x, the Paint filter is really a lie and is just the Shader from the paint, so the behavior might be different. https://github.com/mono/SkiaSharp/pull/2789/files#diff-a167a40fc94fe828c0c2d37f6e69d29949af733c40bf8a8b632223a277b2f712R426

I may try be smarter and detect if you just have a Shader, then use a Shader filter, if you just have a Color or something then use that filter type. However, it may be better to just do it right in the caller so SkiaSharp does not do weird things.

If you have ideas on this, I can try and make the compat even more awesome.

Comment on lines +1066 to +1085
case ShaderImageFilter shaderImageFilter:
{
if (shaderImageFilter.Shader is null)
{
sb.AppendLine($"{indent}var {counter.ImageFilterVarName}{counterImageFilter} = default(SKImageFilter);");
return;
}

var counterShader = ++counter.Shader;
shaderImageFilter.Shader.ToSKShader(counter, sb, indent);

sb.Append($"{indent}var {counter.ImageFilterVarName}{counterImageFilter} = ");
sb.AppendLine($"SKImageFilter.CreateShader(");
sb.AppendLine($"{indent} {counter.ShaderVarName}{counterShader},");
sb.AppendLine($"{indent} {shaderImageFilter.Dither.ToBoolString()},");
sb.AppendLine($"{indent} {shaderImageFilter.Clip?.ToCropRect() ?? "null"});");

sb.AppendLine($"{indent}{counter.ShaderVarName}{counterShader}?.Dispose();");
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I did this right... I don't think this code path will be hit currently as no current API actually uses shader image filters, but this is for when we do.

Comment on lines +700 to +714
case ShaderImageFilter shaderImageFilter:
{
if (shaderImageFilter.Shader is null)
{
return null;
}

return shaderImageFilter.Clip is { } clip
? SkiaSharp.SKImageFilter.CreateShader(
ToSKShader(shaderImageFilter.Shader),
shaderImageFilter.Dither,
ToSKRect(clip.Rect))
: SkiaSharp.SKImageFilter.CreateShader(
ToSKShader(shaderImageFilter.Shader),
shaderImageFilter.Dither);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new shader filter in the skia side.

Comment on lines -505 to +532
return SkiaSharp.SKImageFilter.CreateColorFilter(
ToSKColorFilter(colorFilterImageFilter.ColorFilter),
ToSKImageFilter(colorFilterImageFilter.Input),
ToCropRect(colorFilterImageFilter.Clip));
return colorFilterImageFilter.Clip is { } clip
? SkiaSharp.SKImageFilter.CreateColorFilter(
ToSKColorFilter(colorFilterImageFilter.ColorFilter),
ToSKImageFilter(colorFilterImageFilter.Input),
ToSKRect(clip.Rect))
: SkiaSharp.SKImageFilter.CreateColorFilter(
ToSKColorFilter(colorFilterImageFilter.ColorFilter),
ToSKImageFilter(colorFilterImageFilter.Input));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit unfortunate, but to avoid ambiguity, I could not make the SKRect parameter nullable. However, this code is not too painful. The main change here is that we now just use the Rect directly instead of wrapping it in a CropRect.

SkiaSharp 3.x no longer has the crop rect type at all, so not much we can do besides adding a dummy type - not so nice.

Note: I did not touch the code in the source gen as I am not sure if that generated code is meant to work with multiple versions of skia? If a new source gen is supposed to work with older 2.x versions, then the new APIs will not exist. However, I can also add those changes if need be.

Copy link
Contributor Author

@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.

let me see if the UI can do this...

build/SkiaSharp.HarfBuzz.props Outdated Show resolved Hide resolved
build/SkiaSharp.Linux.props Outdated Show resolved Hide resolved
build/SkiaSharp.props Outdated Show resolved Hide resolved
@mattleibow
Copy link
Contributor Author

yes it can :) sorta

@mattleibow mattleibow marked this pull request as ready for review April 10, 2024 21:58
@wieslawsoltes wieslawsoltes merged commit f987aaf into wieslawsoltes:master Apr 16, 2024
14 checks passed
@mattleibow mattleibow deleted the dev/skia branch April 16, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants