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

Fix : jpgd.cpp Added Additional check to half_image_x_size and half_image_y_size so that it's values wont be Negative[-1]. #43384

Closed
wants to merge 9 commits into from

Conversation

Bhu1-V
Copy link
Contributor

@Bhu1-V Bhu1-V commented Nov 7, 2020

evidence 1

as u can see this was the image in MRP which was successfully imported and u can also see the size of the image is 1 x 1.
also tested with other 1x1 jpg images working just fine. I made changes only on H2V2ConvertFiltered() Function Since the problem was see there only. But i will try to Replicate the problem with different sampling and see whether any more changes are needed. FIX : #42363

Just add an additional check for `half_image_y_size` and `half_image_x_size` for size of 1 pixel so that it's would be zero and not negative(-1) .
@Bhu1-V Bhu1-V requested a review from akien-mga as a code owner November 7, 2020 20:29
@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:thirdparty labels Nov 7, 2020
@Calinou Calinou added this to the 4.0 milestone Nov 7, 2020
@Bhu1-V
Copy link
Contributor Author

Bhu1-V commented Nov 7, 2020

Just Found out that i need to check value of

const int half_image_x_size = (m_image_x_size >> 1) - 1;
in Function H1V2ConvertFiltered() and aslo value of
const int half_image_y_size = (m_image_y_size >> 1) - 1;
H2V1ConvertFiltered() , to be non - negative to not crash.

@Bhu1-V
Copy link
Contributor Author

Bhu1-V commented Nov 7, 2020

After the last Commit we can fully use 1 x 1 pixel jpg images of any sub-sampling without Engine crash.

EVIDENCE 💯
here are some jpg images of different sub-sampling imported into Engine.
H1V1 : 👍
image
H1V2 : 👍
image
H2V1 : 👍
image
H2V2 : 👍
image

TESTED - PROJECT :
gdtest.zip

@Calinou Calinou added the crash label Nov 7, 2020
@akien-mga
Copy link
Member

akien-mga commented Nov 10, 2020

The fix looks fine, but there are a few changes that should be made before it's merged:

  • Commits should be squashed into one, see PR workflow.
  • The commit message for the resulting commit should be amended to be clearer about what the commit does. "Update filename" is not a good message as it doesn't say what the update does. See CONTRIBUTING.md for tips on writing good commit messages.
  • The four changed lines use inconsistent style and parentheses, they should be fixed to all be exactly the same. There's no reason for them to differ.

Finally, this is a thirdparty library so it would be best if we can get this patch merged upstream, so that all users of this library can benefit from the fix. This can also lead to writing a better fix with feedback from the original author of this code.

Could you make a PR to https://github.com/richgel999/jpeg-compressor with those fixes?

We can still merge our own fix in Godot in the meantime, as historically https://github.com/richgel999/jpeg-compressor development was virtually stopped until a recent surge of bugfixing activity - I don't know if Rich is still around reviewing further PRs or if this will happen in another batch in a year or more.

Calinou and others added 6 commits November 10, 2020 20:18
This improves download speeds at the cost of increased memory usage.

This change also effects HTTPRequest automatically.

See #32807 and #33862.
-Discern between named, indexed and keyed
-Get direct access to functions for typed GDScript and GDNative bindings
-Small changes to some classes in order to work with the new setget binder
This reverts commit 5066b53.
@Bhu1-V Bhu1-V closed this Nov 10, 2020
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Nov 10, 2020
@akien-mga
Copy link
Member

Superseded by #43441.

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.

Godot crashes when importing 1x1 pixel JPEG image using YCbCr and 2x2 sampling factor
4 participants