pqina / filepond-plugin-image-preview

🖼 Show a preview for images dropped on FilePond
https://pqina.nl/filepond
MIT License
46 stars 26 forks source link

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

Open justinas55 opened 3 years ago

justinas55 commented 3 years ago

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 https://github.com/pqina/filepond-plugin-image-preview/blob/1484606b564ef9e705a7f738e1d5beba16351856/src/js/view/createImageWrapperView.js#L298 . 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 commented 3 years ago

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

rikschennink commented 3 years ago

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 commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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

rikschennink commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

can you remove the wrapping requestAnimationFrame ?

justinas55 commented 2 years ago

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 commented 2 years ago

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

rikschennink commented 2 years ago

Published 4.6.10 🤞

justinas55 commented 2 years ago

Wow, thanks!