Picture shader #8835
Picture shader #8835
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
I signed CLA agreement
…On Fri, May 3, 2019 at 1:45 PM googlebot ***@***.***> wrote:
Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project (if not, look below for help).
Before we can look at your pull request, you'll need to sign a Contributor
License Agreement (CLA).
📝 *Please visit https://cla.developers.google.com/
<https://cla.developers.google.com/> to sign.*
Once you've signed (or fixed any issues), please reply here (e.g. I
signed it!) and we'll verify it.
------------------------------
What to do if you already signed the CLA Individual signers
- It's possible we don't have your GitHub username or you're using a
different email address on your commit. Check your existing CLA data
<https://cla.developers.google.com/clas> and verify that your email is
set on your git commits
<https://help.github.com/articles/setting-your-email-in-git/>.
Corporate signers
- Your company has a Point of Contact who decides which employees are
authorized to participate. Ask your POC to be added to the group of
authorized contributors. If you don't know who your Point of Contact is,
direct the Google project maintainer to go/cla#troubleshoot (Public
version <https://opensource.google.com/docs/cla/#troubleshoot>).
- The email used to register you as an authorized contributor must be
the email used for the Git commit. Check your existing CLA data
<https://cla.developers.google.com/clas> and verify that your email is
set on your git commits
<https://help.github.com/articles/setting-your-email-in-git/>.
- The email used to register you as an authorized contributor must
also be attached to your GitHub account
<https://github.com/settings/emails>.
ℹ️ *Googlers: Go here
<https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fflutter%2Fengine%2Fpull%2F8835>
for more info*.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8835 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AK5NP7SPO43TBAEJTRTVBY3PTSB5LANCNFSM4HKWFDIQ>
.
|
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Just wanted to clarify something: Picture shader does work on the CPU backend. The fiddle from the original comment has a subtle bug (it specifies that there are 6 indices, but only supplies three). Because the vertices are being used in-order, no indices are needed, so it can be simplified (and made safe) by removing them: |
|
Brian - great catch! When I changed it from drawing a square to a triangle I forgot to change the indices count along with the vertex count. I still believe the PictureShader will need to be created on the GPU thread since we are referencing a surface-backed Picture, but it is good to know the CPU path for PictureShader works... |
|
@cmkweber do you plan to follow up with the simplifications @brianosman suggested? /cc @Hixie for |
|
@cbracken - The PictureShader needs to be on the GPU thread because it references a surface-backed Picture created on the GPU thread. @brianosman was correcting my earlier mistake that a bug might be preventing CPU PictureShaders, but it is my belief CPU PictureShaders aren't suitable here. |
| @@ -2669,6 +2670,13 @@ class ImageShader extends Shader { | |||
| super._(); | |||
| } | |||
|
|
|||
| /// A shader (as used by [Paint.shader]) that tiles a picture. | |||
There was a problem hiding this comment.
Please provide additional documentation, e.g. sample code (see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#provide-sample-code).
There was a problem hiding this comment.
(notably, how to create a PictureShader should be documented here, as well as how to use it)
| /// | ||
| /// The shader is returned asynchronously to allow time for the gpu to | ||
| /// draw the picture and compile a shader. | ||
| Future<PictureShader> toShader(TileMode tmx, TileMode tmy, Float64List matrix4) { |
There was a problem hiding this comment.
In general we prefer to avoid abbreviations, so tileModeX or some such.
In https://master-api.flutter.dev/flutter/painting/paintImage.html we use RepeatImage which doesn't expose mirror. Is your goal to eventually add mirror values to the RepeatImage enum?
|
Ah, I hadn't realized that this was a parallel to ImageShader. We literally never use ImageShader. If it has a problem, it might be simpler for us just to remove it entirely than to add more API surface to try to improve it. |
|
@Hixie - I would agree ImageShader doesn't have many use cases, other than possibly tiling an image pattern. The reason in favor of PictureShader is that it is sometimes useful to draw to a canvas and use the result in further drawing, leaving everything on the GPU and avoiding the round-trip with ImageShader. This functionality is critical for our app, to help draw a large amount of paths, we instead pre-triangulate our entire scene and draw it in 1 or 2 batches using drawTriangles. To be able to color paths individually, we then draw a simple texture atlas and use that as the paint shader in drawTriangles. Since this is being done every frame, a more streamlined way would be to add surface backing functionality to Canvas. Essentially, a user could create a surface-backed canvas, draw to it, request a shader from it, etc. This would allow the user to reuse the same SkSurface/SkCanvas (implicitly) and just draw to it and request a shader each frame. |
|
I think we should consider how to use ImageShader and PictureShader to reimplement paintImage, if it's more efficient than what paintImage does today. (Maybe just for some cases.) At a very minimum, we should have extensive tests here to make sure that ImageShader and PictureShader actually work as we expect. Right now there's really nothing to stop us from regressing the behaviour and we'd never know because these codepaths don't even get run once in any of our automated testing as far as I can tell. |
|
@Hixie - paintImage seems to have more functionality that wouldn't be directly correlated with ImageShader, such as colorFilter and invertColors. They could conceivably be added by using a SkShaders::Blend. Also, the image slicing isn't supported with ImageShader. ImageShader would be more efficient because paintImage issues a number of draw calls to achieve tiling. I agree that tests should be added to accommodate ImageShader and PictureShader. PictureShader is a proposal but as I've mentioned before, a better alternative is a surface-backed canvas that is long-lived which allows a shader to be requested after drawing operations, but that is a bigger API change... |
|
Since we don't use ImageShader at all, I'm certainly open to larger API changes if you think they'd be better. |
|
@Hixie - What is your advice for how to approach the API changes? I have a few proposals below and would like some feedback. I have distilled our rendering pipeline into a simple skia fiddle: The important thing to note is how a canvas uses the result of a previous canvas:
Here are a couple proposals to support this:
My guess is Option 3 would be best because it would be synchronous (on the Dart side) and not have to create GPU tasks constantly and just plug into the current pipeline. I'm not familiar enough with the rendering code to know how to accomplish this though, somewhere on the GPU thread it would have to special case (if SkPaint.shader == PictureShader) and playback its contents before continuing. If this doesn't make sense I can try to explain it another way but I wanted to get your thoughts on which would be the best way to proceed. |
|
I guess my first question would be: what problem are we trying to solve? Maybe what we should do is start from an issue, with a description maybe like "Our app draws N paths each frame, and doing so takes Xms or device Y, which is unacceptably slow". Then we can talk more about exactly what effect you're trying to achieve, and see if there's a better way to solve that specific problem. For example, maybe we just need a "drawPaths" method that draws paths faster than we currently draw them when calling drawPath in a loop. Maybe you're using paths to draw a repeating pattern and the fact that it's paths isn't the issue, it's just that you don't have an efficient way to repeat the pattern. I don't know. It's hard to evaluate proposals without knowing what the problems are. |
|
The initial issue is related to drawing many paths unacceptably slow. Our app is a coloring book app which can have anywhere from 100 to 500 paths per scene. Luckily, there is no animation to the paths, they don't change at all, just the canvas transform changes as the user pans and zooms. The only animation involves the paths changing colors. Simple scenes were performant, but once we loaded in scenes with 100 or more shapes, with that many draw calls it struggled to achieve 30fps on the few devices available to me. I wrote a simple main.dart to test drawing 300 or more objects with various methods: drawPath with every path, drawVertices with every path, and drawVertices with all paths at once, and drawVertices in one call was the only one that was performant across devices. I can try and find this dart test and share it if necessary. I think a drawPaths might be a good addition, but I'm not sure if it will be performant because it will still generate many draw calls (unless it unions the paths), and still need to triangulate. Let me know your thoughts, I can also provide some sample scenes if necessary. Screenshots: Also - the reason for the supersampling is drawvertices doesn't support antialiasing, this isn't required but would be a nice to have... |
|
Oh, yeah, you definitely don't want to have to do all the triangulation and atlassing and so on yourself. That seems like a lot of work for something that should just work well out of the box. I recommend filing a bug (https://github.com/flutter/flutter/issues/new?labels=severe%3A+performance&template=performance.md) and we'll get the skia folks involved. If you have tests or sample code that we can look at specifically, that would be fantastic. Also, regarding drawVertices not doing antialiasing, please file a bug for that too. |
|
@Hixie - Luckily this triangulation and atlas creation is already done, we wrote an Adobe Illustrator export script to handle this. The only missing piece is being able to draw the texture atlas and use that result as a shader for drawVertices. I have spoke with some of the skia folks already and they appear to be on board, some of them are on this PR I believe. As for drawVertices not doing anti-aliasing, there is a skia issue for that but my understanding is the solution is a long ways out for that. I can create a new issue, but wouldn't adding drawPaths ultimately present the same problem? It will presumably take an SkPaint as an argument which would draw all the paths with the same SkPaint, where each path needs its own SkPaint. |
|
You may have done it for your case but I'm more worried about the next ten people who hit this problem, no offense intended. :-) |
|
@Hixie - no offense taken! What about the issue of painting individual paths, won't that issue still exist if drawPaths is added to canvas? Unless you are envisioning it also triangulates, creates an atlas, and uses ImageShader internally to accomplish this. I'm in favor of this, it would make our exporter much simpler, I'm worried about the development time for this to be added to Flutter. We are hoping to launch within the next 3-4 months and I could see something this substantial not being implemented for a long while, let alone it getting onto a stable channel so I could resume using continuous integration. The only thing really missing for us to accomplish what my fiddle is demonstrating is ImageShader using makeImageSnapshot but without rasterizing. It would be great if this could be accomplished synchronously, but we could manage without that. |
I don't know, that's why I think we should start from a more comprehensive description of the problem we're trying to solve, as discussed above.
What might make sense for you is to create your own engine and ship your app from that locally on the short term, and on the long term we can see what a more comprehensive solution might look like. |
|
One issue I have heard from both cmkweber and other flutter developers is
the need to efficiently create images (by drawing). In this case, the image
allows his app to quick change/animate the contents of a drawVertices, but
that is just one use for a dynamically created image (typically to be used
as a shader for some other draw(s). The ability to "snap off" an image from
a drawing canvas is the most obvious, but other ways can work too.
...
offscreen = new ...
canvas = offscreen.getCanvas()
// draw into canvas
image = offscreen.snapShotImage()
// now image can be draw elsewhere, or turned into a shader
…On Mon, Jun 3, 2019 at 6:18 PM Ian Hickson ***@***.***> wrote:
What about the issue of painting individual paths, won't that issue still
exist if drawPaths is added to canvas?
I don't know, that's why I think we should start from a more comprehensive
description of the problem we're trying to solve, as discussed above.
We are hoping to launch within the next 3-4 months and I could see
something this substantial not being implemented for a long while
What might make sense for you is to create your own engine and ship your
app from that locally on the short term, and on the long term we can see
what a more comprehensive solution might look like.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#8835?email_source=notifications&email_token=ADDLXIR6HWCCG3OWBO37L5DPYWKEBA5CNFSM4HKWFDI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW23HTA#issuecomment-498447308>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADDLXIQPYCTEVWDW3YDOJFTPYWKEBANCNFSM4HKWFDIQ>
.
|
|
@reed-at-google thanks for adding that - I agree there are numerous use cases here with RTT. Currently in flutter the only way to make an image snapshot is with Picture.toImage - but it goes all the way to CPU. @Hixie - is there anything we could add in the intermediate while drawPaths is being fleshed out? Right now Image always references a CPU SkImage, but if functionality was added to allow it to reference a surface-backed GPU SkImage, it would provide a lot of versatility. I'd like to avoid (if possible) having to compile my own local engine and keep it in sync with latest flutter and be able to use continuous integration. |
|
I have created a performance issue for the long-tail resolution to this: Would still be very interested in a short-term solution as proposed above... |
|
@cmkweber @reed-at-google @Hixie it sounds like the plan is to not proceed with this, but instead to deal with this upstream in Skia. If so, should we close this PR? |
|
@cbracken - It seems like the plan is to close this, but it would be extremely helpful to my project for a short term fix for this since the upstream fix is probably a long way out. As @reed-at-google pointed out there are many uses for the ability to reference a surface backed image. If I just had a way to "snap-off" an image from the canvas and use it (with ImageShader) without going to CPU that would be immensely useful. |
|
@cmkweber For the short term I would recommend building a custom engine with this feature. No need for us to ship the feature just for you. :-) |
|
@Hixie - It looks like that may be my only option. At the risk of being annoying at this point, I will try one more 'Hail Mary'! These are a number of open issues I think would benefit from this, so I don't think the issue would be just for me in fairness... ImageFilter Widget As you noted here ImageFilter can't be 60fps because the rasterizer returns images asynchronously. Slow transform: Your comment notes that RepaintBoundary.toImage is going from GPU to CPU and back to GPU. https://api.flutter.dev/flutter/widgets/ModalRoute/buildTransitions.html Routes will stop animating and pause during screen transition if transform is used. Saturation Blur: The comment here about using a repaint boundary requires static content, since RepaintBoundary.toImage Draw repeat patterns: Your suggestion here is to avoid the toImage call and use drawPicture multiple times. Texture Canvas: This user is essentially mirroring the entire Canvas API just to be able to use the drawn texture elsewhere. |
|
#14733 would be addressed to some extent by this PR, but the others wouldn't. |
|
To reiterate, the problem with this PR is that it extends an API that we're already not using, to solve a use case that we should solve at a higher level. I could see us getting behind this PR if it removed the existing unused ImageShader, reimplemented paintImage (in the framework) in a backwards-compatible fashion using this (thus actually showing that we will use this feature), and had a benchmark that showed that it was an actual improvement. (We'd also to address the comments that are already applicable to this PR, such as adding sample code and so on.) But if the goal is just to solve the stated problem, then it's not the right long-term solution, and we shouldn't take short-term solutions. |
|
I spoke with @reed-at-google and @brianosman of the Skia team about this. They gave me more context and pointed out a few things that we could do that would be even better for you than this PR:
I am happy for us to do these, provided that as part of that work or before that work we first pay down the technical debt we have in this area:
|
|
(We don't have an ETA for when to do this; we have a lot of higher priority work going on right now. We would accept PRs that move us in that direction if you are interested.) |
|
@Hixie - that's great news, I truly believe opening up toImage calls to return a usable Image on the current frame opens up myriad possibilities. Entire widget trees could be screenshotted and have ImageEffects applied, allowing things like animated saturation blurs, or animated convolution filters if they get added. I think it is good if ImageShader is kept, otherwise the texture coordinates supplied to Vertices can't be used effectively. I'll definitely help where I can, my trials with storing a surface backed image with toImage calls were unsuccessful since I ran into GrSingleOwner problems, so I'm not the best one to tackle that. But I can help write tests and documentation and rework paintImage to use ImageShader. |
|
Also - to add, the reason I didn't use PictureShader directly in this PR is because I was noticing SkResourceCache objects being held longer than expected. This may be alleviated now though with the merging of this issue: #9004 |
This PR is addressing:
#flutter/flutter#31338
It implements a new shader type: PictureShader. This is a shader that is constructed through Picture.toShader() which returns Future(PictureShader). The reason this does not follow the same construction as ImageShader (by passing in an image) is that PictureShader must be created on the GPU thread. If it was constructed through PictureShader(picture, ...) the actual shader would be blank until it is actually drawn by the GPU.
You can see in this Skia fiddle that an SkPictureShader cannot be allocated on the CPU path:
https://fiddle.skia.org/c/6090413aa2b53941221574f14f3fd383
If you comment out the beginning 'if' statement, the fiddle crashes...
Another thing to note is that the shader generation doesn't use SkPicture::makeShader. Unfortunately, the bulk of that function is copied here but without the use of SkResourceCache. If this shader is created every frame the SkResourceCache grows quickly and causes large collections by the time dart sees the shaders are no longer referenced. Even without the SkResourceCache, the collection of just these shaders is noticeable in Debug, in Release it is performant.
A better long-term solution may be to provide flutter the ability to use an SkSurface backed canvas instead of just a one time use Canvas with PictureRecorder.
cc @brianosman @fmalita