Switch rendering to surface, decrease image size #47
Conversation
RossComputerGuy
force-pushed
the
fix/jitter
branch
from
6c13e02 to
9d70074
Compare
RossComputerGuy
changed the title
|
@RossComputerGuy this change seems to completely break the ability to pinch to zoom. Is that working on your end? |
This makes zooming work while keeping performance up.
RossComputerGuy
force-pushed
the
fix/jitter
branch
from
06fa136 to
1246680
Compare
example/lib/pages_with_tiled_images/main_page_list_images_inspector.dart
Outdated
Show resolved
Hide resolved
| width: 1024, | ||
| height: 768, |
There was a problem hiding this comment.
This is almost certainly still way too big, but should be better than the current implementation.
Ideally these sizes would be as close as possible to the resolution you will actually display the image at.
There was a problem hiding this comment.
You also probably only want to specify one dimension or use ResizeImagePolicy.fit (which is not the default)
There was a problem hiding this comment.
Ah, I wasn't sure what values to use so I just chose that. When I tried it out, I got extra vertical whitespace.
There was a problem hiding this comment.
Yeah, if you use fit it will use that as a bounding box - if you specify one dimension it'll automatically scale the other dimension to preserve the aspect ratio.
There was a problem hiding this comment.
Gotcha, I switched it to the fit policy.
|
@RossComputerGuy in general we don't want to arbitrarily change numbers. The reason for the Instead, if you find that a particular value has an effect on performance, you should follow that number until you find out why. Then you can (hopefully) optimize the root cause of the performance issue. As for the decoding the images. First, with regard to aspect ratio - I believe I was forcing the images into an exact width and height on purpose. All of our tiles are supposed to be the same size. I suggest returning that behavior to how it was. @dnfield - you mentioned that these images were being decoded at 4k or something, right? But the PR diff seems to indicate that we were decoding with a width 1024, which is way less than 4k, isn't it? That width also isn't massively wider than the screen. In fact, if we were to view in landscape mode, 1024 wouldn't even fill the width of the screen. In portrait mode, it's a bit wider, but not to an obnoxious extent. So it seems to me like the decoding size probably wasn't a meaningful contributing factor to the issue, right? |
Didn't know that was a DPI value. I'll change it back to 72. |
|
If you don't use a resize image the images get decoded at their natural resolution. The images in this project are large. If you try to display many of them at once and pan them on a 1080 screen like the tablet I was using, the GPU is spending a lot of time down sampling them every frame which is contributing to jitter. |
|
So @dnfield are you saying that the adjustment in this PR has no effect on the decoding size? Because it sounds like either we were already decoding at the smaller value of |
|
Would it be more beneficial to have pre-scaled images and basically do what this page of the asset docs say? |
|
The initial adjustment has no effect. The later commits to use resize image should help but might not be enough when you're showing many tiles in a row. |
|
What adjustment would be enough? I think our goal here is to do whatever you need us to do to prove or disprove the hypothesis. |
I think a real solution would probably have multiple resolutions and select the appropriate one for the current resolution. Enough is zooming to your desired level and figuring out the actual display area of the image and decoding to that size. |
|
Specifically if there are three images to a grid row, divide the width of the screen by 3 and make the width of the image that. |
|
That sounds like something we would do after fully vetting the hypothesis. But given that we still don't seem to have any particular confirmation that this is the root cause, and given that it would require a significant amount of work to make any such changes to our PDF viewer, how can we fully confirm that this issue is the root cause before making such a large investment? |
Downsample the images significantly and see if it helps. |
|
Ok, so what I'm asking is, what value do you need to see to be convinced one way or the other? Again, all of this work is based on showing you what you need to see, to determine if this is a problem somewhere in Flutter that should be addressed. This entire demo only exists for that purpose. So do you want us to divide that number 3? by 10? What value will confirm or reject the hypothesis on your end? |
|
The Galaxy tablet I was using has a width of 1200 in portrait mode. Make the width no larger than 400. |
|
Ok. @RossComputerGuy please do as Dan requested, and then please re-run the same profile as Dan. |
|
Tested and this was the result: https://www.youtube.com/watch?v=lBgnfwWU35I |
|
@RossComputerGuy that video seems to indicate obvious jitter, but as a reminder, we need to always test under load. You're testing the lowest level of load when you pan a single image tile. Instead, you should zoom in far enough to get about 9 tiles, and then you shoudl run both the vertical and horizontal animation together, in an oval. Also, please be sure to push changes to the PR when you post performance updates so that we're clear about what code yielded that result. @dnfield assuming the down-scaling was implemented correctly, this seems to disprove the hypothesis. |
|
Alright, I wasn't sure if a push was needed or not but I'll keep that in mind. I decided to run in release mode, I scrolled down a bit, zoomed, and got this. https://youtu.be/K5VaBorcOXY But yeah, seems like jitter is still there. |
No description provided.