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

Image randomly blurred/upscaled (due to expand animation?) #57

Open
justinas55 opened this issue Jul 23, 2021 · 14 comments
Open

Image randomly blurred/upscaled (due to expand animation?) #57

justinas55 opened this issue Jul 23, 2021 · 14 comments

Comments

@justinas55
Copy link

justinas55 commented Jul 23, 2021

Describe the bug
Dropping an image to filepond sometimes renders it blurred/upscaled (container is bigger than image resized for preview) and sometimes it is working correctly.

filepond good

filepond blurred

To reproduce:

Context

Investigations/Solutions
It seems like container height is captured in the start of animation and is much less than it expands to. So image is resized to small image, but then container expands - at least that's my guess.

In

const previewContainerHeight = root.rect.element.height;
.
If I change that to:
var previewContainerWidth = root.element.clientWidth; var previewContainerHeight = root.element.clientHeight;
then it seems to work correctly, but didn't have the time to properly validate this solution, yet.

Also using fixed height option imagePreviewHeight fix this problem as well.

P.S. Really grateful for this project, it's a very nice plugin!

@rikschennink
Copy link
Collaborator

Wow that's a weird bug. Thank you so much for the detailed report, will look into this as soon as possible.

@rikschennink
Copy link
Collaborator

I tried to produce this with a big jpeg and png but I'm not able to. Tested on MacOS chrome 92 and Windows 10 Chrome 92. (Tested on Firefox and Safari as well just to be sure)

FilePond calculates sizes in a requestAnimationFrame loop so there are no reflows during animations, while re-requesting clientWidth and clientHeight will get the correct size it shouldn't be needed as it should be calculated before.

Will leave this open though as it looks like something is up.

@justinas55
Copy link
Author

Did a couple more tests and tracked it down to screen refresh rate:

  • I use 100 Hz screen refresh rate, if I switch to 60 Hz - problem disappears.
  • Also tried Microsoft Edge Version 92.0.902.62 (Official build) (64-bit) where problem is same as in Chrome
  • Works in Firefox 88.0.1 (64-bit) without the problem even @ 100 Hz.
  • Tried enabling/disabling hardware acceleration and switching off all extensions, resetting default theme, but was still reproducing in Chrome @ 100 Hz.

I tried to screenshot page when hitting breakpoint in previewImageLoaded in both refresh rates, but to my eyes I don't see anything obvious between 60 Hz and 100 Hz views, just that root.rect.element.height is too small @ 100 Hz:

filepond-preview-debug2-60hz
filepond-preview-debug2

@justinas55
Copy link
Author

calling requestAnimationFrame two times in a row before calling drawImage does help, but so does adding setTimeout(..., 10).
my guess:

Just didn't find how root.height = ... results in actual element's height being set, but probably the fix would be to ensure that requestAnimationFrame(() => drawPreview...) is called after panel's height is set.

@rikschennink
Copy link
Collaborator

Thank you so much for this info, I'm going to try and find a way to reproduce this and will get back to you.

@rikschennink
Copy link
Collaborator

The issue is that I don't have a way to test this / reproduce the problem.

Is there any way to simulate 100hz in chrome? Firefox has a layout.frame_rate setting.

@justinas55
Copy link
Author

I googled a bit, but didn't find such an option. But if you have any ideas/commits/PRs I can test it.

@rikschennink
Copy link
Collaborator

Can you try replacing

https://github.com/pqina/filepond-plugin-image-preview/blob/master/src/js/index.js#L172

with a setTimeout(/* */, 0)

I'm thinking maybe because of the high frame rate it runs before the height has been applied?

@justinas55
Copy link
Author

When I changed those lines to

requestAnimationFrame(function() {
				  setTimeout(function () {
					root.dispatch('DID_FINISH_CALCULATE_PREVIEWSIZE', {
					  id: props.id
					});
				  }, 20);
              });

It seems to be stable now, but using 0ms or 10ms delays still resulted in same artifact.

Another one that works and seems a bit cleaner to me:

requestAnimationFrame(function() {
				  requestAnimationFrame(function () {
					root.dispatch('DID_FINISH_CALCULATE_PREVIEWSIZE', {
					  id: props.id
					});
				  });
              });

@rikschennink
Copy link
Collaborator

can you remove the wrapping requestAnimationFrame ?

@justinas55
Copy link
Author

Still reproduces with this:

if (root.ref.shouldDrawPreview) {
              // queue till next frame so we're sure the height has been applied this forces the draw image call inside the wrapper view to use the correct height
				setTimeout(function () {
					root.dispatch('DID_FINISH_CALCULATE_PREVIEWSIZE', {
					  id: props.id
					});
				  }, 0);
              root.ref.shouldDrawPreview = false;
            }

@rikschennink
Copy link
Collaborator

Alright, thanks for the quick test, so nested requestAnimationFrame seems to do the trick.

@rikschennink
Copy link
Collaborator

Published 4.6.10 🤞

@justinas55
Copy link
Author

Wow, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants