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

Fixed ignoring frequencies in pHash #13

Closed
wants to merge 1 commit into from

Conversation

artyom-razinov
Copy link

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
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
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
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
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.

None yet

2 participants