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

TexturePacker: use std::hash to check for dupes #23296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented May 18, 2023

This changes TexturePacker to use std::hash instead of the included md5 implementation. This seems to still provide the same Textures.xbt file.

I'm not sure if my implemented std::hash method is really as good as the md5 implementation.

How has this been tested?

produces the same sha256 for Textures.xbt

What is the effect on users?

none

Signed-off-by: Lukas Rusak <lorusak@gmail.com>
@lrusak lrusak added Type: Improvement non-breaking change which improves existing functionality Component: Build v21 Omega labels May 18, 2023
@lrusak lrusak added this to the Omega 21.0 Alpha 2 milestone May 18, 2023
@lrusak lrusak requested a review from neo1973 May 18, 2023 03:00
@fritsch
Copy link
Member

fritsch commented May 18, 2023

Some questions:
What's the reasoning behind it? Simplification? Runtime Speed? Code Volume?
You implemented a reversible hash now, when the magic pattern is known, why?
Also seeding with the size when most pictures are maybe 1920x1080 is questionable.
I take this is just for comparing and nothing cryptographically relevant.

Why not use a proper upstream hash implementation? Is it cause of speed constraints? A simple crc32 is fast as hell.

@jenkins4kodi
Copy link
Contributor

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

For more information please see our current code style guidelines.

Copy link
Member

@neo1973 neo1973 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall nice reduction in code size, just one suggestion to think about.

Comment on lines +84 to +90
for (auto x : frame.rgbaImage.pixels)
{
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = (x >> 16) ^ x;
seed ^= x + 0x9e3779b9 + (seed << 6) + (seed >> 2);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (auto x : frame.rgbaImage.pixels)
{
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = ((x >> 16) ^ x) * 0x45d9f3b;
x = (x >> 16) ^ x;
seed ^= x + 0x9e3779b9 + (seed << 6) + (seed >> 2);
}
auto frameHash = std::hash<std::string_view>()(
std::string_view(reinterpret_cast<const char*>(frame.rgbaImage.pixels.data()),
frame.rgbaImage.pixels.size()));
seed ^= frameHash + 0x9e3779b9 + (seed << 6) + (seed >> 2);

Not an expert on hash functions, but it looks to me as this one is intended for integers? My suggestion is to use the std::string_view hasher, I assume it will give good results.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jun 19, 2023
@jenkins4kodi
Copy link
Contributor

@lrusak this needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build Rebase needed PR that does not apply/merge cleanly to current base branch Type: Improvement non-breaking change which improves existing functionality v22 "P"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants