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

Correct lossless issue on linux x86 #579

Merged
merged 6 commits into from
Sep 11, 2015
Merged

Conversation

mayeut
Copy link
Collaborator

@mayeut mayeut commented Sep 9, 2015

Update #571

@malaterre
Copy link
Collaborator

this fixes the symptoms, not the bug

@mayeut
Copy link
Collaborator Author

mayeut commented Sep 10, 2015

@malaterre,

opj_tcd_makelayer takes a threshold that's computed in opj_tcd_rateallocate

the computation for the threshold in case of lossless transform is identical to the computation in opj_tcd_makelayer. Thus, it expects "computation" == stored_threshold to be true at some point.
The stored threshold will always be rounded to 64bits. "computation" precision will depend on compilation options, architectures, ...

IMHO, this PR removes the dependency on 64bits precision.

If you fill it does not fix the issue properly, I'll revert the "invalid fix" & only merge the tests in.

It doesn't change the outcome of the test suite, that's weird...
@mayeut
Copy link
Collaborator Author

mayeut commented Sep 10, 2015

@malaterre,

Given the definition of DBL_EPSILON:

#define DBL_EPSILON     2.2204460492503131e-016 /* smallest such that 1.0+DBL_EPSILON != 1.0 */

It might not be a good fix.

We might want to make the check with epsilon directly in the test in opj_tcd_makelayer

if ((thresh - (dd / dr)) <= DBL_EPSILON) /* do not rely on float equality, check with DBL_EPSILON margin */
    n = passno + 1;

I think the later would be OK, do you agree ?

@malaterre
Copy link
Collaborator

@mayeut that's a lot better ! I still do not like the equality (does not work for floating point anyway). So I would write:

if ((thresh - (dd / dr)) < DBL_EPSILON)

Do we need an fabs here also ?

Remove float equality test. Such a test has no meaning.
@mayeut
Copy link
Collaborator Author

mayeut commented Sep 11, 2015

@malaterre,

I removed = from the test. It's now:

if ((thresh - (dd / dr)) < DBL_EPSILON)

fabs would have been needed if original test was ==. This is not the case here.

@malaterre
Copy link
Collaborator

ACK. Everything is fine for me. Thx

mayeut added a commit that referenced this pull request Sep 11, 2015
Correct lossless issue on linux x86
Fix #571
@mayeut mayeut merged commit 5d95355 into uclouvain:master Sep 11, 2015
@mayeut mayeut deleted the lossless branch September 11, 2015 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants