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

Fast accuraterip checksum #37

Merged

Conversation

MerlijnWajer
Copy link
Collaborator

This is an experimental pull request to calculate AccurateRip checksums a lot faster. I have only cherry-picked and rebased my code, I haven't actually tested it yet. But I'm putting this out here now - so others can also give it a run (and let me know/flame me if it doesn't work).

It adds in my version of leo-bogerts https://github.com/MerlijnWajer/accuraterip-checksum - mostly 32 bit fixes.

This pull request also adds the ability to:

  • Calculate the AccurateRip V2 checksum. (But the code doesn't yet do this, since we need changes in other places as well, better served in a separate pull request)
  • Calculate the AccurateRip (V1/V2) checksum of a FLAC file, by calling the external flac decoder.

None of this adds extra gstreamer dependencies, but replaces some functionality that requires gstreamer.

It does add an dependency on the 'flac' binary (only if you are working directly on FLAC files) and it adds the accuraterip-checksum tool to the repository, as a single C file, plus some automake changes. If you do not want to the tool in the repository (license is compatible), then I can work on way to make it an external tool.

@MerlijnWajer
Copy link
Collaborator Author

Please don't merge this PR (yet) - I'd like to get some feedback, and may rebase the commits in the coming days. I have more queued up, but I want to get this in first.

@MerlijnWajer
Copy link
Collaborator Author

It looks like the CI environment will need additonal headers and -dev versions of some libraries, likely (at least) this: libsndfile1-dev

@JoeLametta
Copy link
Collaborator

JoeLametta commented Sep 20, 2016

Please don't merge this PR (yet) - I'd like to get some feedback, and may rebase the commits in the coming days. I have more queued up, but I want to get this in first.

OK, thanks for letting me know.

It looks like the CI environment will need additonal headers and -dev versions of some libraries, likely (at least) this: libsndfile1-dev

I've just added libsndfile1-dev to main. Let me know if I also need to add flac as a dependency.

@JoeLametta
Copy link
Collaborator

@MerlijnWajer Any news about this one?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 15, 2016

I've tested this one both with the HTOA test CD and a "standard" one and it worked fine (as expected the final AccurateRip computation is now way faster).

PS: The pull request was locally rebased before testing it.

@MerlijnWajer
Copy link
Collaborator Author

Right. So this may be a candidate for merging, then. I can rebase it today.
What this doesn't add yet -- is ARv2 support, in the sense that it's not yet added to the result structure, and it's not yet matched against. But perhaps we can just merge this soon, and then I'll submit a new PR for the ARv2 checksums?

I'll rebase it asap, as mentioned.

There are still some changes required to make whipper handle ARv2 checksums,
currently doesn't call the Task for a V2 checksum. V1 should keep working as-is.
@JoeLametta
Copy link
Collaborator

perhaps we can just merge this soon, and then I'll submit a new PR for the ARv2 checksums?

Yeah that's fine.
IIRC there's also a separate issue tracking just ARv2 support.

I'll rebase it asap, as mentioned.

Great!

@MerlijnWajer
Copy link
Collaborator Author

(Rebased)

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 15, 2016

(Rebased)

OK. Just to be extra clear: should I merge it?^ (in that case I think I should also update the README)

Thanks

^ Not this one but the rebased pull request ;)

@MerlijnWajer
Copy link
Collaborator Author

Yes, please merge it.

@JoeLametta JoeLametta merged commit 7749c19 into whipper-team:master Oct 15, 2016
@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 15, 2016

Yes, please merge it.

Done.
Travis CI reports a new test failing:

FAIL: testAccurateRipChecksum (morituri.test.test_image_image.TrackSingleTestCase)
testAccurateRipChecksum
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/twisted/internet/defer.py", line 149, in maybeDeferred
    result = f(*args, **kw)
  File "/usr/local/lib/python2.7/dist-packages/twisted/internet/utils.py", line 201, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/usr/local/lib/python2.7/dist-packages/twisted/internet/utils.py", line 197, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/travis/build/JoeLametta/whipper/morituri/test/test_image_image.py", line 39, in testAccurateRipChecksum
    self.assertEquals(h(checksumtask.checksums[0]), '0x00000000')
  File "/usr/local/lib/python2.7/dist-packages/twisted/trial/_synctest.py", line 425, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
  File "/usr/lib/python2.7/unittest/case.py", line 511, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 504, in _baseAssertEqual
    raise self.failureException(msg)
FailTest: '0xd60e55e1' != '0x00000000'

Here's the portion of the test code which is failing: if that's expected and normal I'll silence that test.

@MerlijnWajer
Copy link
Collaborator Author

Regarding the test failure, I think that may just happen because my code may return '0x0' and not '0x00000000'...

@MerlijnWajer
Copy link
Collaborator Author

Oh, hang on, that's probably not right/true. Let me look at it a bit more.

@MerlijnWajer
Copy link
Collaborator Author

I think the problem is that his test case tests something that doesn't happen in practice (as far as I can see) - performing accurate rip checksums on parts of a file. In all the rip use cases that I know of, we perform the check on the entire file. cdparanoia rips it to a file, and then we perform a check on the while file. Supporting this special behaviour is not particularly easy.

This specific test case is a cue file where there are several tracks in a single flac file?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 16, 2016

This specific test case is a cue file where there are several tracks in a single flac file?

Correct: this one.

@JoeLametta
Copy link
Collaborator

Supporting this special behaviour is not particularly easy.

For the moment being I've Just commented out the failing lines (4636c55).

PS: The four assertions I've commented out are all failing.

@RecursiveForest
Copy link
Contributor

Given the unusual nature of the test, I think it's probably better to just delete the tests outright if we're never going to try and support them.

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