Skip to content
This repository was archived by the owner on Sep 20, 2021. It is now read-only.

Fixed ignoring frequencies in pHash#13

Closed
artyom-razinov wants to merge 1 commit intoameingast:masterfrom
artyom-razinov:fix-phash
Closed

Fixed ignoring frequencies in pHash#13
artyom-razinov wants to merge 1 commit intoameingast:masterfrom
artyom-razinov:fix-phash

Conversation

@artyom-razinov
Copy link
Copy Markdown

15 of lowest 64 frequencies were ignored instead of suggested 1. This caused images with low details produce 0 hash (e.g.: screenshots of UI).

Note that this breaks compatibility with previously generated values of pHash. I've added tests that check future possible regression of values.

My reference to the algorithm was: http://www.hackerfactor.com/blog/index.php?/archives/432-Looks-Like-It.html#c1410

15 of lowest 64 frequencies were ignored instead of suggested 1. This caused images with low details produce 0 hash (e.g.: screenshots of UI).
Note that this breaks compatibility with previously generated values of pHash. I've added tests that check future possible regression of values.
My references of algorithm were: http://www.hackerfactor.com/blog/index.php?/archives/432-Looks-Like-It.html#c1410
@artyom-razinov
Copy link
Copy Markdown
Author

artyom-razinov commented Oct 8, 2018

It seems that calculation is platform-dependent: tests fail on a different platform (it reproduces locally as well).

@artyom-razinov
Copy link
Copy Markdown
Author

artyom-razinov commented Oct 8, 2018

This test fails on 32-bit iPhone 4S (iOS 9.3): testResizedRGBABitmapDataScanline
aHash and dHash calculation is different on different devices. E.g. aHash values for same image:
on Mac OS: -8608481637110868033
64 bit iPhone Sim: -8608472703578630209
32 bit iPhone Sim: -8608191228601917505

  1. Would you make an issue?
  2. Is my fix in pHash worth merging? If so, I'll remove tests from the PR, because they don't work (I didn't do it to show you the issue).

@ameingast
Copy link
Copy Markdown
Owner

ameingast commented Nov 19, 2018

Is my fix in pHash worth merging?

Absolutely, but I don't want to break backwards compatibility.

If you provide a new hash-type (.pHashNoFrequencies ? - I can't think of a better name) we can talk about merging this in.

@ameingast
Copy link
Copy Markdown
Owner

Closing due to inactivity.

@ameingast ameingast closed this Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants