Skip to content

Use img_hash for rendering tests#292

Merged
CryZe merged 1 commit intoLiveSplit:masterfrom
wooferzfg:perceptual-hashing
Jan 23, 2020
Merged

Use img_hash for rendering tests#292
CryZe merged 1 commit intoLiveSplit:masterfrom
wooferzfg:perceptual-hashing

Conversation

@wooferzfg
Copy link
Copy Markdown
Member

@wooferzfg wooferzfg commented Jan 21, 2020

Fix #287

@wooferzfg
Copy link
Copy Markdown
Member Author

wooferzfg commented Jan 21, 2020

@CryZe The single_line_title rendering test seems to produce different results in the CI than it does for me locally. This issue also seems to be happening in other PRs, where I think the test randomly started failing over the last few days.

@wooferzfg wooferzfg marked this pull request as ready for review January 21, 2020 02:42
@wooferzfg wooferzfg requested a review from CryZe January 21, 2020 02:42
CryZe
CryZe reviewed Jan 21, 2020
View reviewed changes
src/rendering/software/mod.rs Outdated
// supported by Rust just yet.
not(all(
target_arch = "x86",
not(target_feature = "sse"),
Copy link
Copy Markdown
Collaborator

@CryZe CryZe Jan 21, 2020

Choose a reason for hiding this comment

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

If the image hash is good enough, we shouldn't need this anymore either.

Copy link
Copy Markdown
Member Author

@wooferzfg wooferzfg Jan 21, 2020

Choose a reason for hiding this comment

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

Seems like some of the image comparisons are still slightly off. Could we retrieve the images from the targets that are failing and see what it actually looks like?

Copy link
Copy Markdown
Collaborator

@CryZe CryZe Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You could try to run these tests locally. Just install the specific target and run the tests with the target.

rustup target add i586-pc-windows-msvc
cargo test --all-features --target i586-pc-windows-msvc

@CryZe
Copy link
Copy Markdown
Collaborator

CryZe commented Jan 21, 2020 •
edited
Loading

The single_line_title rendering test seems to produce different results in the CI than it does for me locally.

Even now with the image hash? You may need to run cargo update then to get the latest versions of each crate. But yeah I noticed that this changed recently as well.

Actually if you locally get a different hash, then the hash may not be robust enough.

@wooferzfg
Copy link
Copy Markdown
Member Author

wooferzfg commented Jan 21, 2020 •
edited
Loading

The tests pass for me after running cargo update. The difference between the hash I was getting before and the one I get now is just 1 bit, so we could potentially have a tolerance for the distance between hashes that's more than 0. I'm just not sure what that tolerance should actually be.

@CryZe
Copy link
Copy Markdown
Collaborator

CryZe commented Jan 21, 2020

I think keeping it on 0 for now is okay. Because the image we are getting now is actually fairly questionable. We are seeing some weird diagonal aliased line now, which may be worth investigating and reporting to whatever caused this. So I feel like we shouldn't just completely brush off notable changes like these via a larger tolerance value.

@CryZe
Copy link
Copy Markdown
Collaborator

CryZe commented Jan 21, 2020

Well, looks like we need a tolerance value on non-SSE platforms at least. Maybe we can use the cfg we had previously to conditionally give those platforms a larger tolerance value.

@CryZe
Copy link
Copy Markdown
Collaborator

CryZe commented Jan 21, 2020

Another way to minimize these problems should be to render the more broken tests in a higher resolution. That way minimal changes such as one glyph being +/- 1 pixel off causes less of a relative error.

@wooferzfg wooferzfg force-pushed the perceptual-hashing branch 2 times, most recently from 581cdb4 to 0465c1e Compare January 22, 2020 03:15
@wooferzfg
Copy link
Copy Markdown
Member Author

wooferzfg commented Jan 22, 2020

I switched to using actual and expected for the file names because:

  • I was having trouble differentiating which file was which when the hashes were too similar, which will happen much more often with perceptual hashing.
  • base64 uses /, which I think causes problems when creating the file path.

@wooferzfg wooferzfg force-pushed the perceptual-hashing branch 2 times, most recently from 62f0b05 to 16a311d Compare January 22, 2020 04:04
@wooferzfg
Copy link
Copy Markdown
Member Author

wooferzfg commented Jan 22, 2020 •
edited
Loading

I tried using bigger hashes, but that caused the rendering tests on the Linux mips builds to start failing for some reason. I haven't tried higher resolution rendering of the actual images yet.

@wooferzfg wooferzfg requested a review from CryZe January 22, 2020 04:28
CryZe
CryZe reviewed Jan 22, 2020
View reviewed changes
src/rendering/software/tests.rs Outdated
Comment on lines +235 to +238
let expected_path = format!("target/renders/{}_expected.png", name);

if distance > get_comparison_tolerance() {
let actual_path = format!("target/renders/{}_actual.png", name);
Copy link
Copy Markdown
Collaborator

@CryZe CryZe Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if I'm too much of a fan of this. The old one had the advantage that I could switch branches and it still would find the expected image for the specific branch, so if visual changes would happen, it was still able to create a proper diff image.

Copy link
Copy Markdown
Collaborator

@CryZe CryZe Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What we could do is throw the hash base64 string into AES to get highly scrambled bytes out again and revert the AES to get back the image hash for the expected AES hash.

Copy link
Copy Markdown
Member Author

@wooferzfg wooferzfg Jan 23, 2020

Choose a reason for hiding this comment

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

For now, I reverted to the old behavior of using the hash in the filename, but with the / all replaced with $ so that there's no issue with certain filenames. We can use AES if we think that the hashes being too similar is a problem (but I'm also not sure how to use the aes crate).

CryZe
CryZe approved these changes Jan 23, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@CryZe CryZe left a comment

Choose a reason for hiding this comment

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

I guess this is good enough for now. Good work! :)

@CryZe CryZe merged commit b8eb568 into LiveSplit:master Jan 23, 2020
@CryZe CryZe added code quality Affects the quality of the code. enhancement An improvement for livesplit-core. rendering The issue or pull request is affecting the rendering. labels Jan 23, 2020
@CryZe CryZe added this to the v0.12 milestone Jan 23, 2020
@wooferzfg wooferzfg deleted the perceptual-hashing branch January 23, 2020 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CryZe CryZe CryZe approved these changes

Assignees

No one assigned

Labels

code quality Affects the quality of the code. enhancement An improvement for livesplit-core. rendering The issue or pull request is affecting the rendering.

Projects

None yet

Milestone

v0.12

Development

Successfully merging this pull request may close these issues.

Use perceptual hashing algorithm for the rendering tests

2 participants

@wooferzfg @CryZe