nguillaumin / jflac

Forked from SourceForge
http://jflac.sourceforge.net/
Other
25 stars 10 forks source link

Merge JustFlac codec #12

Closed wolfgangasdf closed 7 years ago

wolfgangasdf commented 8 years ago

As announced in https://github.com/nguillaumin/jflac/issues/10 this is a pull request to merge the codec from https://github.com/drogatkin/JustFLAC into this, to obtain one working (incl. stream playing, no premature stopping) java flac codec & SPI that is maven-compatible (JustFlac is not).

This merge would overwrite the current implementation of seeking/skipping, but the JustFlac version thereof works here.

I tried to merge all commits after the sourceforge jflac forks, except some of the recent coverity additions.

I asked drogatkin (the maintainer of JustFlac, who fixed a number of bugs and added [also] rice2) if he would agree to this merge: He finds that this is a good idea.

But I am no flac expert, however, the current jflac simply doesn't work here, see https://github.com/nguillaumin/jflac/issues/10 . It would be great if someone else could confirm this, just run the SndPlayer example from here and from https://github.com/wolfgangasdf/jflac.

alexhk90 commented 8 years ago

I am using jFLAC (via Maven) with the RICE2 enhancements and the seeking functions, so will check if everything still works after this merge.

nguillaumin commented 8 years ago

Thanks, no objection in principle, if you can resolve the merge conflicts?

wolfgangasdf commented 8 years ago

I tried to merge this. I tested a rice2 test file from http://samples.mplayerhq.hu/flac, seems to work with Player.java.

wolfgangasdf commented 7 years ago

I give up, you have some very good changes here, your nice binary-comparison tests are all fine, and the reason that JustFlac worked was because -1 was returned from getFrameLength, and that's acceptable because AudioSystem.NOT_SPECIFIED==-1. So I completely removed the merge with JustFlac (sorry for wasting your time!), I see no reason. I changed the issues announced in my first post, and a test case for stream playing, using the same binary comparison method as you did. Seems to work here very well. Seems like the travis test fails with the new stream test case, do you have a hint on what to do?

alexhk90 commented 7 years ago

A quick look at the tests that are failing suggests some of the test files are being (falsely?) detected as unsupported by the test framework. Looking at the file names below at least one is 24 bit (and not 44.1 kHz) so I'm guessing is is something to do with this.

Test code:

DataLine.Info info = new DataLine.Info(SourceDataLine.class, audioFormat); Assert.assertTrue("Play.playAudioStream does not handle this type of audio on this system.", AudioSystem.isLineSupported(info));

Results:

Failed tests: test[0: ../jflac-codec/src/test/resources/testdata/McDougalsMen24bit_48kHz.flac](org.jflac.apps.DecoderStreamComparisonTest): Play.playAudioStream does not handle this type of audio on this system.

test[1: ../jflac-codec/src/test/resources/testdata/cymbals.flac](org.jflac.apps.DecoderStreamComparisonTest): Play.playAudioStream does not handle this type of audio on this system.

test[2: ../jflac-codec/src/test/resources/testdata/ExitMusic.flac](org.jflac.apps.DecoderStreamComparisonTest): Play.playAudioStream does not handle this type of audio on this system.

test[3: ../jflac-codec/src/test/resources/testdata/deerhunter.flac](org.jflac.apps.DecoderStreamComparisonTest): Play.playAudioStream does not handle this type of audio on this system.

test[4: ../jflac-codec/src/test/resources/testdata/thear1.flac](org.jflac.apps.DecoderStreamComparisonTest): Play.playAudioStream does not handle this type of audio on this system.

Tests run: 10, Failures: 5, Errors: 0, Skipped: 0

nguillaumin commented 7 years ago

I'm not familiar with AudioSystem at all, but I suspect it's related to the fact that the Travis CI box doesn't have sound capabilities (mixers, etc...), so we can't really get a Line from it (which presumably maps to some OS provided mixer facility?).

Is there a way to workaround that, not reference the OS-provided sound facilities, but use some kind of dummy/internal mixer or something?

nguillaumin commented 7 years ago

I added a test that seem to confirm this theory, the Travis CI build reports zero mixers...

nguillaumin commented 7 years ago

I tweaked the Travis CI config to install pulseaudio and now it reports 1 mixer:

Found 1 mixers
Found mixer info: default [default]
    Description: Direct Audio Device: default, default, default
    Vendor: ALSA (http://www.alsa-project.org)
    Version:

However your test still doesn't work. I think something should be achievable with pulseaudio to set up virtual mixers or whatever to make it work, not sure if that's something you guys would be familiar with?

Otherwise in the meantime if you want this merged what we can do is just skip this specific test on Travis CI, by adding something like Assume.assumeTrue(System.getenv("TRAVIS") == null);

nguillaumin commented 7 years ago

I'm still keen on a CHANGELOG update by the way as I'm not sure I could articulate myself what you fixed here :wink:

wolfgangasdf commented 7 years ago

I would like to debug this but setting up a local travis environment is pretty hard. I also couldn't find a solution online. So, I disabled the test if it runs in travis. Do you think this is ok? I added a short line to the changelog, tell me if you want something else. In case you want to merge this, I have just send the I send the committer of a0eb9b4740c05a83e20746d09817356d508aee3b an email, I tell you if he/she has something to say to this.

nguillaumin commented 7 years ago

Ok thanks, it's not ideal yeah but that's better than nothing. For debugging Travis issues, usually what I do is just push stuff to a test branch and Travis builds it automatically. Not great as the only way to test things is to add some code, or tweak the Travis CI YAML file...

I'll wait a bit before merging then in case there's feedback.

nguillaumin commented 7 years ago

Released as v1.5.1, will be available in Maven Central in the next hours