Skip to content

Image enconding and deconding fixes #4051

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

Closed
wants to merge 7 commits into from

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Jun 13, 2021

No description provided.

@fmassa
Copy link
Member

fmassa commented Jun 14, 2021

Thanks a lot for the PR!

This in principle looks good to me, but I would love if @andfoy could have a closer look.

Also, could you add a test?

@fmassa
Copy link
Member

fmassa commented Jun 14, 2021

Also, do you know if this fixes #3613?

@andfoy
Copy link
Contributor

andfoy commented Jun 14, 2021

In principle, the changes do not modify any of the current behaviour, as they only provide a refactor of terms for clarity. I'll take a look if this fixes the libpng bug. But I agree with the changes

@cyyever
Copy link
Contributor Author

cyyever commented Jun 15, 2021

Thanks a lot for the PR!

This in principle looks good to me, but I would love if @andfoy could have a closer look.

Also, could you add a test?

To test image decoding errors, we'd better use fuzzing tests. I have some plans, but it should be put in a separate PR.

@fmassa
Copy link
Member

fmassa commented Jun 21, 2021

@cyyever can you clarify what are your thoughts about the fuzzy testing that you are describing?

Also, can you clarify what this PR fixes?

@cyyever
Copy link
Contributor Author

cyyever commented Jun 21, 2021

@cyyever can you clarify what are your thoughts about the fuzzy testing that you are describing?

Also, can you clarify what this PR fixes?

I mean we use libfuzzer to generate malformed-images. This is a good way to check image decoding and encoding bugs. However, it needs to change the test infrastructure to include c++ test cases. I think those changes need a separate PR.

And this PR fixes some problem reported by clang-tidy and other static code analyzers.
I'm not sure whether #3613 would be fixed. Nevertheless, it is absolutely useful.

@andfoy
Copy link
Contributor

andfoy commented Jun 22, 2021

This PR does not fix #3613:

Python 3.9.5 | packaged by conda-forge | (default, Jun 19 2021, 00:32:32) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import torchvision
>>> image = torchvision.io.read_image("283xnnabju4z.png")
>>> image = torchvision.io.read_image("283xnnabju4z.png")
free(): invalid size
Aborted (core dumped)

@cyyever cyyever closed this Jun 25, 2021
@cyyever cyyever deleted the image_fixes branch July 1, 2021 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants