noxworld-dev / opennox

OpenNox main repository.
GNU General Public License v3.0
451 stars 25 forks source link

Refactor audio decoding #708

Closed dennwc closed 1 month ago

dennwc commented 2 months ago

Previously WAV decoding and PCM/ADPCM/MP3 state was all coupled together. This PR splits is into separate structures, making it possible to add new audio formats or test existing ones. This could also help swap the audio backend later too.

It also adds a test that verifies that our decoder gets at least the same number of frames as ffmpeg when decoding Nox data (reference test table included).

Hashes of decoded audio we get is not the same as with ffmpeg, unfortunately. But it's probably only off by a few bits here and there. Will need to check later.

Also, our music decoding results is just a few samples shorter (~100 samples; ~4 ms) comparing to ffmpeg decoding. It doesn't happen with dialogs, though.

Required sign-off

blmarket commented 1 month ago

Seems weird. my comment is not showing if not logged in... Trying to quote:

  1. Not sure we can ask other devs to have their own installations to run tests. i.e. this change creates external dependency which dev may not have right now
  2. dialog can be localized, so some files are not always available. (e.g. there can be XYXYE.wav and XYXYK.wav etc.)

Would be great if there's a flag to control running test which needs original game data, and disable it by default. (How about we skip the test if env NOX_DATA is not specified?)

dennwc commented 1 month ago

Yeah, it runs under the assumption that there's at least one GoG-like installation somewhere. I do have localizations too, but they are in different folders that auto-detection cannot recognize.

How about we skip the test if env NOX_DATA is not specified?

The whole point of that noxtest package was to not set NOX_DATA explicitly :sweat_smile: Maybe some special value will do? Like NOX_DATA=-.

blmarket commented 1 month ago

Let me take it back. I think current behavior is the best.

What I expected to see:

dennwc commented 1 month ago

Unfortunately I don't think there's any good way around -v. Without it all test logs (including skipped tests, as you noticed) are suppressed.

We could write to stderr directly to get around that, but this usually breaks other Go tools that expect specific output format from go test. We could also write to stdout from tests, but that's also considered a bad practice.

So we should probably keep the current behavior and probably document that its advised to pass -v to tests.

blmarket commented 1 month ago

Unfortunately I don't think there's any good way around -v. Without it all test logs (including skipped tests, as you noticed) are suppressed.

We could write to stderr directly to get around that, but this usually breaks other Go tools that expect specific output format from go test. We could also write to stdout from tests, but that's also considered a bad practice.

So we should probably keep the current behavior and probably document that its advised to pass -v to tests.

Agreed. thanks for sharing your thoughts!