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

Typoed coefficients in luminance calculation #77

Open
pteichman opened this issue Apr 27, 2018 · 3 comments
Open

Typoed coefficients in luminance calculation #77

pteichman opened this issue Apr 27, 2018 · 3 comments
Labels

Comments

@pteichman
Copy link

The luminance calculation in smartcrop.js is currently:

function cie(r, g, b) {
    return 0.5126 * b + 0.7152 * g + 0.0722 * r;
}

But going back to the HDTV Rec. 709 (either https://en.wikipedia.org/wiki/Rec._709 or the original https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.709-6-201506-I!!PDF-E.pdf ), these coefficients should be:

0.2126 * r + 0.7152 * g + 0.0722 * b

With two differences: first is that the r and b channels have been swapped, and second is the typo of 0.5126 for 0.2126 in the weight of the red channel. This should also be technically on linear RGB rather than sRGB (as you'll likely get from JS) but that's less of an issue.

The current code weights blue ~7x higher than it should. I don't know whether it's worth changing smartcrop.js results at this point to fix it, though.

@pteichman
Copy link
Author

Thinking about this a little more, I think the result is that smartcrop.js prefers blue edges.. Which probably doesn't skew the results badly one way or another for most images.

@jwagner
Copy link
Owner

jwagner commented May 1, 2018

Thanks for reporting this. This clearly wasn't intended to be that way. I'll have a look when I find time.

@jwagner jwagner added the bug label May 1, 2018
jwagner added a commit that referenced this issue May 6, 2018
@jwagner
Copy link
Owner

jwagner commented May 6, 2018

Gave it a quick test, as you suspected, the results are mostly but not exactly.
I want to play with the smartcrop algorithm again anyways and actually derrive all of the magic fuzz factors in a more scientific way than guessing so I'll probably release the change together with that (when it happens).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants