drowe67 / LPCNet

Experimental Neural Net speech coding for FreeDV
BSD 3-Clause "New" or "Revised" License
68 stars 24 forks source link

Uncouple circular dependency with Codec2 #43

Closed tmiw closed 1 year ago

tmiw commented 1 year ago

Adds required files from Codec2 so that LPCNet doesn't have to be linked with Codec2 per https://github.com/drowe67/codec2/discussions/342. For those files, we compile with -fvisibility=hidden so that there aren't conflicts with the Codec2 versions when we link LPCNet into the former (source for info about that compiler flag).

Current status: when linked with this PR, Codec2 ctests on master currently fail. However, it's unknown if these failures are related:

The following tests FAILED:
     13 - test_FDMDV_modem_octave_mod_demod (Failed)
     18 - test_COHPSK_modem_freq_offset (Failed)
     21 - test_OFDM_modem_octave_port_Nc_31 (Failed)
     74 - test_freedv_api_2020C_mpd (Failed)

Additionally, LPCNet ctests are failing due to use of developer specific paths (e.g. build_linux inside test_core_nn.sh):

40% tests passed, 3 tests failed out of 5

Total Test time (real) =   0.15 sec

The following tests FAILED:
      1 - core_synthesis_default (Failed)
      2 - core_synthesis_load_20h (Failed)
      3 - core_synthesis_mag (Failed)
tmiw commented 1 year ago

Also, a lot more files needed to be brought in than I thought (though the majority are .h files).

drowe67 commented 1 year ago

Thanks for working on this @tmiw.

These tests run OK for me (no path problems) but fail:

The following tests FAILED:
      1 - core_synthesis_default (Failed)
      2 - core_synthesis_load_20h (Failed)
      3 - core_synthesis_mag (Failed)

I recall there were some numerical/precision issues which made some of these tests work on one machine and not another. Not very good tests I guess. Still, it would be nice to check we haven't broken anything :thinking:

tmiw commented 1 year ago

Thanks for working on this @tmiw.

These tests run OK for me (no path problems) but fail:

The following tests FAILED:
    1 - core_synthesis_default (Failed)
    2 - core_synthesis_load_20h (Failed)
    3 - core_synthesis_mag (Failed)

I recall there were some numerical/precision issues which made some of these tests work on one machine and not another. Not very good tests I guess. Still, it would be nice to check we haven't broken anything 🤔

Well, at least the paths seem to be more platform/developer independent now, but still no dice on my end.

FWIW, the test that's currently being done in GitHub actions/freedv-gui builds is this:

# simple test to make sure the code runs
cd src && sox ../../wav/wia.wav -t raw -r 16000 - | ./lpcnet_enc -s | ./lpcnet_dec -s > /dev/null
drowe67 commented 1 year ago

I manually checked the feature file is the same for master and ms-uncouple-codec2:

~/tmp/LPCNet/build_linux ms-uncouple-codec2 $ ./src/dump_data --test --c2pitch ../wav/birch.wav birch.f32
~/tmp/LPCNet/build_linux ms-uncouple-codec2 $ diff birch.f32 ~/LPCNet/unittest/birch.f32

The pitch estimate is in one of the fields in the .f32 file, so this suggests this PR is doing the same thing as master, which uses the libcodec2 pitch estimation code :+1:

drowe67 commented 1 year ago

Should the LPCNet README.md build instructions be tweaked?

tmiw commented 1 year ago

Should the LPCNet README.md build instructions be tweaked?

Yep, just did. 👍

drowe67 commented 1 year ago

@tmiw In https://github.com/drowe67/LPCNet/pull/44 I've had a go at building up some ctests that pass and can run with GitHub actions. Please try merging this PR with LPCNet/master and lets see if the tests work with this PR. The first ctest should pick up any pitch estimator issues, as that's one of the features dump_data generates.

tmiw commented 1 year ago

@tmiw In #44 I've had a go at building up some ctests that pass and can run with GitHub actions. Please try merging this PR with LPCNet/master and lets see if the tests work with this PR. The first ctest should pick up any pitch estimator issues, as that's one of the features dump_data generates.

All tests pass for me now. 👍

drowe67 commented 1 year ago

@tmiw OK to merge now? I bumped the version number, as I figure this will need a new release.

tmiw commented 1 year ago

@tmiw OK to merge now? I bumped the version number, as I figure this will need a new release.

Yep.

tmiw commented 1 year ago

Should a new release be tagged at https://github.com/drowe67/LPCNet/releases?

drowe67 commented 1 year ago

Should a new release be tagged at https://github.com/drowe67/LPCNet/releases?

Yes, but how about we wait until codec2 and freedv-gui have been updated in line with this change? There might still be some LPCNet tweaks required before the dust settles.

tmiw commented 1 year ago

Should a new release be tagged at https://github.com/drowe67/LPCNet/releases?

Yes, but how about we wait until codec2 and freedv-gui have been updated in line with this change? There might still be some LPCNet tweaks required before the dust settles.

That works. I think there's an issue compiling freedv-gui for Windows so I'll investigate that.