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

Add compatibility overloads for SKFilterQuality #2963

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

Youssef1313
Copy link
Contributor

@Youssef1313 Youssef1313 commented Jul 31, 2024

Libraries may want to remain supporting both SkiaSharp 2.x and 3.x, as we currently do in Uno Platform.

We achieve this by detecting the assembly version and using UnsafeAccessor when needed for APIs that have changed in SkiaSharp 3.

However, there is one remaining issue for Uno Platform, which is SKFilterQuality. As the overloads now take SKSamplingOptions which is a completely new type in SkiaSharp 3, we can't use UnsafeAccessor to get this to work.

This PR adds new obsolete APIs that accepts SKFilterQuality and forwards it to the SKSamplingOptions overload. For Uno Platform, we will use paint.FilterQuality when on 2.x, and use the new shader overloads via UnsafeAccessor when on 3.x

NOTE: This is applicable for any library that needs to remain on 2.x while allowing consumers to upgrade to 3.x.

@mattleibow, if you can review please

@Youssef1313
Copy link
Contributor Author

Ping @mattleibow for review

@mattleibow
Copy link
Contributor

I am not sure how this would work. These are all new APIs that never existed in 2.x:

https://github.com/mono/SkiaSharp/blob/release/2.88.8/binding/Binding/SKBitmap.cs#L877-L884

What are you currently doing today? What version of SkiaSharp are you working with?

@Youssef1313
Copy link
Contributor Author

@mattleibow We are compiling against 2.x, but still want to support users who upgrade to 3.x via a compatibility layer we did using UnsafeAccessor.

Currently, we can't use UnsafeAccessor for SKSamplingOptions as it's a whole new type. All we can do is some ugly reflection that we want to avoid.

With this PR, assuming ported to 2.x, we will be able to use the new methods both on 2.x and 3.x

@mattleibow
Copy link
Contributor

How will this work in 2.x? Are you able to create a PR against release/2.x to show. Not sure 2.x has this arg so is it just a no-op?

Also, adding a test to make sure nothing blows up will also be great.

Thanks!

@Youssef1313
Copy link
Contributor Author

Youssef1313 commented Aug 14, 2024

@mattleibow Okay I missed that this won't be possible to backport to 2.x (I had it right in the original PR description), still will allow us to easily use UnsafeAccessor for 3.x specifically, and fallback to set quality on the paint when on 2.x

@mattleibow
Copy link
Contributor

OK, I see how this works. I think this is fine then, they are obsolete and removed from the public API. So, this should make it safe.

I see the 2.x version of skia had a more rudimentary form of the sampling options, and I contemplated adding that. But, rather add this than change the past and cause other issues.

mattleibow
mattleibow previously approved these changes Aug 14, 2024
@Youssef1313
Copy link
Contributor Author

Youssef1313 commented Aug 16, 2024

@mattleibow Great! I assume the CI failures are not related to my PR? Not sure if they need to be fixed separately before this PR can be merged or not necessary for merging this?

@mattleibow
Copy link
Contributor

The CI failures are a failure of CI. Still trying to figure it out. Every month it seems that CI needs some tweaks.

@mattleibow mattleibow merged commit e5675c9 into mono:main Aug 16, 2024
1 of 2 checks passed
@Youssef1313 Youssef1313 deleted the add-compat-skfilterquality branch August 16, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants