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

Memory leak in FastSSIM when image dimensions are not multiples of 32 #229

Open
abhinaukumar opened this issue Apr 21, 2020 · 3 comments · May be fixed by #230
Open

Memory leak in FastSSIM when image dimensions are not multiples of 32 #229

abhinaukumar opened this issue Apr 21, 2020 · 3 comments · May be fixed by #230

Comments

@abhinaukumar
Copy link

In the attached screenshots, I am running FastSSIM with the same image as input and output, of size 627x482. So, we expect that at every level, upon downsampling, the images still have the same entries in corresponding locations. Wherever different, I have printed the column number, row number and the corresponding entries in the two images. The junk values (sometimes negative) at the borders of the image demonstrate the memory leak. The first line of the output shows an attempt to access row index 120 of the image, while the dimensions show that upon downsampling twice, the heights of the image at the first two levels are 482/2 = 241 and 241/2 = 120. So, there should not be an attempt to access row index 120.

daala_fastssim_mem_leak
daala_fastssim_mem_leak2

@tdaede
Copy link
Contributor

tdaede commented Apr 21, 2020

The main implementations of ssim in the daala repo are the dump_ssim and dump_msssim implementation (I wrote the latter after discovering similar issues in fastssim). Arguably we should delete the fastssim implementation as I don't think there is any reason to use it instead of the other two.

@abhinaukumar
Copy link
Author

I see. Considering how much faster it is than dump_ssim and dump_msssim, I think it is worth holding on to. I have created a PR with a simple fix that should help.

@tdaede
Copy link
Contributor

tdaede commented Apr 21, 2020

Neat, if it's delivering advantages over the other two we should definitely keep it around.

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

Successfully merging a pull request may close this issue.

2 participants