-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
Converting to draft since the SkiaSharp is still a preview. I am hoping to push a stable soon and we can all win. |
There was a problem hiding this 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.
build/SkiaSharp.props
Outdated
<PackageReference Include="SkiaSharp" Version="2.88.6" /> | ||
<PackageReference Include="SkiaSharp" Version="2.88.8-preview.1.1" /> |
There was a problem hiding this comment.
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.
public static SKImageFilter CreateShader(SKShader shader, bool dither, CropRect? cropRect = null) | ||
=> new ShaderImageFilter(shader, dither, cropRect); |
There was a problem hiding this comment.
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.
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; | ||
} |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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...
yes it can :) sorta |
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.