Avoid the deprecated SkFilterQuality in the Engine APIs

[flar]

Flutter is transitioning away from the SkFilterQuality enum upon which the Flutter FilterQuality enum is based.

This PR is a WIP to transition our public FilterQuality name to a new class that implements all or most of the functionality of the new SkSamplingOptions. A number of issues remain outstanding:

• Need to upgrade our canvaskit so we can implement this for Web
• FilterQuality is disappearing from SkPaint, upon which our Paint object is closely based
• In Skia, the sampling options tend to be associated with a number of objects (for instance ImageShader) for which we don't take any FilterQuality or similar object
• In Skia, a number of the rendering calls (mostly drawImage variants) now take a SkSamplingOptions parameter directly rather than taking this value from the Paint object
• Should we expose the implementation classes _MipmapFilterQuality and _CubicFilterQuality or just have them exposed via the factories on FilterQuality?
• Should we support custom cubic filters (the equations take 2 coefficients and we expose a hard-coded B=.3/C=.3 pair for compatibility, but other options might be suitable for some developers).

This PR will currently fail due to not implementing a lot of the new API changes on web, but it is good to provide a baseline on discussions about what to do with our APIs.

Fixes: flutter/flutter#76737

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jason-simmons]

LGTM

[yjbanov]

• Need to upgrade our canvaskit so we can implement this for Web

Do we have respective issues filed in skbug.com to add this to CanvasKit?

• FilterQuality is disappearing from SkPaint, upon which our Paint object is closely based

I think for the scope of this change it's safe to assume that it won't disappear.

• In Skia, the sampling options tend to be associated with a number of objects (for instance ImageShader) for which we don't take any FilterQuality or similar object

If we make sampling options an extension of the FilterQuality enum, would it still be compatible with those objects? For example, would FilterQuality.medium still make sense for ImageShader?

• In Skia, a number of the rendering calls (mostly drawImage variants) now take a SkSamplingOptions parameter directly rather than taking this value from the Paint object

Yeah, our Paint class is very overloaded. It's hard to tell which properties apply to which draw calls. For example, Paint.maskFilter doesn't apply to drawVertices, nor does isAntiAlias, but Paint.imageFilter does. If that's intentional behavior then Paint doesn't make it clear.

• Should we expose the implementation classes _MipmapFilterQuality and _CubicFilterQuality or just have them exposed via the factories on FilterQuality?

This depends on whether FilterQuality is compatible with all places that take MipmapFilterQuality and CubicFilterQuality. Otherwise, the risk is we'll end up with another Paint-like object that can do 10 things, but only an unknown subset of those things apply in any given draw call.

• Should we support custom cubic filters (the equations take 2 coefficients and we expose a hard-coded B=.3/C=.3 pair for compatibility, but other options might be suitable for some developers).

I think these are pretty standard concepts that would give developers more control. I'd say yes, let's expose them. We'll need to find a way to emulate these in HTML, if there's no direct support.

[flar]

• Need to upgrade our canvaskit so we can implement this for Web

Do we have respective issues filed in skbug.com to add this to CanvasKit?

The latest rev of the public JavaScript API supports nearly everything we need here except for the filter options in drawAtlas. I'll have to file an issue for that.

I figured out how to extend our internal canvaskit_api wrappers to expose the necessary APIs and am nearly done with getting all of this hooked up on the canvaskit renderer except for drawAtlas, which will require a new rev of the public API.

• FilterQuality is disappearing from SkPaint, upon which our Paint object is closely based

I think for the scope of this change it's safe to assume that it won't disappear.

It is already not used. It is vestigial. We need to deal with that because setting it will not impact rendering at this point. That's a real change and it has already happened.

And, the removal of the property from SkPaint is only pending removing it from one other client of Skia, so it is not like this can be ignored for too long.

But, the point is not when it will happen, but that there will be a new way of doing things very soon now and if we're going to make changes, we should include preparing for that new world as part of those changes.

That isn't to say that we can't have it continue to be a property of the dart level Paint object, but that it will go away in the underlying APIs that we are implemented on top of.

• In Skia, the sampling options tend to be associated with a number of objects (for instance ImageShader) for which we don't take any FilterQuality or similar object

If we make sampling options an extension of the FilterQuality enum, would it still be compatible with those objects? For example, would FilterQuality.medium still make sense for ImageShader?

Yes, I've already completed this work. ImageShader now takes an optional FilterQuality, which is as expressive as the SamplingOptions, and medium makes sense.

• In Skia, a number of the rendering calls (mostly drawImage variants) now take a SkSamplingOptions parameter directly rather than taking this value from the Paint object

Yeah, our Paint class is very overloaded. It's hard to tell which properties apply to which draw calls. For example, Paint.maskFilter doesn't apply to drawVertices, nor does isAntiAlias, but Paint.imageFilter does. If that's intentional behavior then Paint doesn't make it clear.

I've basically added them as optional parameters to our Dart version of the methods, deferring to the FilterQuality stored in the Dart version of the Paint object if it is not specified.

• Should we expose the implementation classes _MipmapFilterQuality and _CubicFilterQuality or just have them exposed via the factories on FilterQuality?

This depends on whether FilterQuality is compatible with all places that take MipmapFilterQuality and CubicFilterQuality. Otherwise, the risk is we'll end up with another Paint-like object that can do 10 things, but only an unknown subset of those things apply in any given draw call.

This is more like an abstract base class with multiple subclasses, not a holder of a bunch of possibly related, but often unrelated values. I don't see how that applies.

• Should we support custom cubic filters (the equations take 2 coefficients and we expose a hard-coded B=.3/C=.3 pair for compatibility, but other options might be suitable for some developers).

I think these are pretty standard concepts that would give developers more control. I'd say yes, let's expose them. We'll need to find a way to emulate these in HTML, if there's no direct support.

I think we're already sunk just trying to implement all of the sampling options in HTML, but I haven't dug very deep there. Many of the web_ui versions of the objects have Unimplemented for the HTML renderer in the first place - ImageShader is one of them that is only implemented at all on the canvaskit renderer. This isn't a question of whether the filtering options are supported, the entire feature isn't supported.

[flar]

Current status: I'm in the final stages of implementing the API described here on canvaskit.

[zanderso]

Is there a github issue that this can be linked to?

[flar]

I found 2 main issues with missing canvaskit APIs, filed under:

https://bugs.chromium.org/p/skia/issues/detail?id=11732

[flar]

Is there a github issue that this can be linked to?

Link added

[yjbanov]

Are we still waiting for the CanvasKit parts?

[flar]

I don't think I need to hold up for these 2 cases. I'm working on moving the recent doc changes over and writing test cases (both for my own testing and for CI testing).

I've backed off on adding the sampling parameters to our public Dart Canvas class which ended up being a wild goose chase. I had added them and had that all compiling and then I discovered a class in the framework that was "implementing" Canvas which then threw compile errors because its signatures didn't match. In the end, I decided not to add the arguments to our Canvas classes and just have them get the data from the Paint as they have always been doing. This means that we still have drawAtlas implemented, but it can't specify the new sampling options to canvaskit until they add it to their API. It will still limp along with the old FilterQuality taken from the CkPaint object if they haven't eliminated that under the covers yet. I have a TODO for that for which I will have to file a flutter issue if I push the PR updates before they get around to it.

And, while we have a way to instantiate a canvaskit MatrixImageFilter under the covers - which still uses the old SkFilterQuality - it isn't hooked up to the web version of ImageFilter.matrix, so there is no way to access it. We currently have a CI test that makes sure that we support ImageFilter at the Ck level, but the public API is stubbed out with "Unimplemened" so only the test can access it. That's an entirely different bug, but it means that it doesn't matter in the short term when these sampling changes will go in. There was an issue filed on matrix being Unimplemented a while back: flutter/flutter#45213

[flar]

After modifying all of the code paths and adding the various SkSamplingOptions variations to the public Flutter FilterQuality class I didn't see enough benefit from the combinations that we don't already expose via the existing 4 enums in FilterQuality, so I backed off on the API changes and switched this PR to a mostly bookkeeping change that weans our code off of the SkFilterQuality support as much as possible.

There are still a couple of references in the CanvasKit code due to the fact that not all of the canvaskit methods were updated to provide new ways to pass in the sampling options (see https://bugs.chromium.org/p/skia/issues/detail?id=11732).

[flar]

This is ready to go. It leaves a couple of references still in there which can be eliminated once https://bugs.chromium.org/p/skia/issues/detail?id=11732 lands.

[zanderso]

lgtm w/ nits

[flar]

@yjbanov the remaining canvaskit parts can be easily patched in later, are your questions resolved?