roc-streaming / rt-tests

Real-time tests for Roc Toolkit.
Mozilla Public License 2.0
1 stars 6 forks source link

Added.cpp and.h files for the command line tool test, modified CMakeLists.txt #8

Open u7763283 opened 8 months ago

u7763283 commented 8 months ago

The test content can run normally and the test results are correct.

u7763283 commented 8 months ago

Reference issues:

4

roc-toolkit:Issue #552

gavv commented 8 months ago

Thanks for PR, will review it in upcoming days!

gavv commented 8 months ago

Nice! Here are a few suggestions:

  1. Since we use libsndfile for reading wav files, I guess we can use it in generate_wavfile() too, to simplify the code.

  2. For WAV file, it's better to use temporary file instead of using hard-coded path in current dir. This way we don't limit concurrent execution of same tests, don't touch source directory, and don't require current dir to be writable by current user (may be important when we execute tests on remote box). I guess yoy could use std::filesystem::temp_directory_path().

  3. We can use sndfile.hh instead of sndfile.h, it provides C++ version of the library with RAII (we won't need to call sf_close() manually).

  4. Let's use vector of strings instead of single string for commands (like, {"roc-send", "-vv", ...} instead of "roc-send -vv ..."). This way we can be sure we won't mess with shell substitutions (e.g. we don't need to escape temp file path).

  5. Please rename file from command_line.cpp to test_command_line.cpp (same as the other test is named). Also I think the header file is not needed because it's never used outside the test. You can move class definition to an anonymous namespace in the .cpp file.

  6. On my machine, test fails:

    [ RUN      ] CommandLine.SendReceiveWavFile
    Successfully open!
    Wav file sent successfully!
    
    Wav file received successfully!
    0@
    Samplerate does not match
    sended file:44100
    received file:48000
    /home/victor/dev/roc-streaming/rt-tests/tests/command_line.cpp:185: Failure
    Value of: compare_wavfiles(filename.c_str(), received_file.c_str())
      Actual: false
    Expected: true
    The contents of the original file and the received file do not match!
    [  FAILED  ] CommandLine.SendReceiveWavFile (1511 ms)

    It can be fixed by adding --rate 44100 to receiver command-line. Please add it.

  7. On my machine, there is a segfault in compare_wavfiles(). The problem is that sf_readf_double() expects number of samples per channel (a.k.a. number of frames), so that you need to pass to it buffer_size / 2 insead of buffer_size.

  8. After fixing two problems above, the comparison fails for me:

    [ RUN      ] CommandLine.SendReceiveWavFile
    Successfully open!
    Wav file sent successfully!
    
    Wav file received successfully!
    
    File data do not match
    /home/victor/dev/roc-streaming/rt-tests/tests/command_line.cpp:185: Failure
    Value of: compare_wavfiles(filename.c_str(), received_file.c_str())
      Actual: false
    Expected: true
    The contents of the original file and the received file do not match!
    [  FAILED  ] CommandLine.SendReceiveWavFile (1512 ms)

    Actually I expected this from the code. The reason is that you're doing direct comparison in compare_wavfiles(). Doing direct comparison is not correct - the received stream may differ from the sent stream: there may be shifts, drops, and leading silence.

    This is described in detail here: roc-streaming/roc-go#101. The last section (starting with "Here is an algorithm ...") describes how we can do the comparison correctly. This algorithm is implemented in roc-streaming/roc-go#111. It's in Go, not in C, but I think it's rather readable even if you don't know Go. In rt-tests, we should implement something similar.


BTW, in case you're interested, later you could take a look at these issues in toolkit repo, I think they would have implementation similar to what you did here: roc-streaming/roc-toolkit#576, roc-streaming/roc-toolkit#246.