Enable MSAA behind a command line flag for iOS #33461
Conversation
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Minor but LGTM otherwise.
| auto surface = CreateSurfaceFromMetalTexture(context_.get(), drawable.get().texture, | ||
| kTopLeft_GrSurfaceOrigin, // origin | ||
| 1, // sample count | ||
| msaa_samples_, // sample count |
There was a problem hiding this comment.
Some validation of sample counts would be good here. Docs say it could be 1, 2, 4 or 8. Though we should stick to 1 and 4 for max compatibility.
There was a problem hiding this comment.
The Settings object validates that this is 0, 1, 2, 4, 8, or 16. I can add tighter validation here, but should it just bea DCHECK?
There was a problem hiding this comment.
Added another validation FML_CHECK in GPUSurfaceMetalSkia's ctor
|
|
||
| protected: | ||
| IOSContext(); | ||
| explicit IOSContext(int msaa_samples); |
There was a problem hiding this comment.
Make this an enum perhaps? Saying IOSContext(1) for the ctor reads odd.
There was a problem hiding this comment.
Made an enum. Left it as an int in GPUSurfaceMetalSkia.
|
|
||
| virtual std::shared_ptr<impeller::Context> GetImpellerContext() const; | ||
|
|
||
| int get_msaa_samples() const { return msaa_samples_; } |
There was a problem hiding this comment.
Should it be int GetMSAASamples()
There was a problem hiding this comment.
Done (but as GetMsaaSamples. I think our style guide does want this to be get_msaa_samples but that style isn't used elsewhere in this file.
| /// engine/platform. | ||
| /// @param[in] msaa_samples | ||
| /// The number of MSAA samples to use. Only supplied to | ||
| /// Skia, must be either 0, 1, 2, 4, or 8. |
There was a problem hiding this comment.
What happens when an invalid value is passed. Do we default to certain value, or a crash, or undefined behavior.
Should we prevent it on our side or the Skia API has something to prevent it.
There was a problem hiding this comment.
The parsing for that is handled in the Settings parse for command line, but I guess Ic an add more.
There was a problem hiding this comment.
Added validation in GPUSurfaceMetalSkia's ctor.
dnfield
added
the
waiting for tree to go green
| // Skia allows 0 and clamps it to 1. | ||
| FML_CHECK(msaa_samples_ == 0 || msaa_samples_ == 1 || msaa_samples_ == 2 || msaa_samples_ == 4 || | ||
| msaa_samples_ == 8) | ||
| << "Invalid MSAA sample count value: " << msaa_samples_; |
There was a problem hiding this comment.
very optional nits:
maybe add a link or some message about what should be the correct value. I guess people use this parameter should be able to easily figure out when they see the error message though. Your call.
There was a problem hiding this comment.
Normally this should be either 1 (disabled) or 4 (enabled), but the idea is to leave it a little more flexibleright now for experiments/benchmarking.
If we decide to ship this independently of impeller, it'll probably just be set to 4 no matter what.
…" (flutter#33481)" This reverts commit 3ee9c18.
…" (flutter#33481) This reverts commit 8f5e8e7.
4 participants
Enables MSAA behind a flag, similar to Android. This turned out to be easier than I thought - Skia already knows how to create the resolve texture.
Manually/visually verified drawVertices does AA with this set to 4.
iOS part of flutter/flutter#100392
@jonahwilliams FYI