Closed Hrick87 closed 2 months ago
github-actions[bot]
commented
6 months ago :robot: Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.
Hrick87
commented
6 months ago @gavv
I was hoping you might have an idea as to why the macos13-x86_64/build-3rdparty.sh, macos13-x86_64/standard-build.sh and macos/universal-binaries.sh scripts are failing in my latest commit 471615c? The log and error code returned has not been helpful. Commit 779319d passed macos13-x86_64/standard-build.sh, but 471615c has identical code.
I apologize for all the commits of me testing minor changes to the scripts, but I was unable to figure out how to run the macOS scripts locally like I did with the other environments. Also, GitHub Actions runs the build scripts MUCH faster than my Virtual Box instance can. I can squash them once this is resolved if that is preferred.
Once passing, my code will be ready for review.
Thank you!
gavv
commented
6 months ago Sorry for delay. I triggered build on develop & master, and the same issue is reproduced there, so it's no specific to your PR.
https://github.com/roc-streaming/roc-toolkit/actions/runs/7242028057/job/19726898808
I'll take a look.
gavv
commented
6 months ago I apologize for all the commits of me testing minor changes to the scripts, but I was unable to figure out how to run the macOS scripts locally like I did with the other environments. Also, GitHub Actions runs the build scripts MUCH faster than my Virtual Box instance can. I can squash them once this is resolved if that is preferred.
No worries, github allows to squash commits into one when merging PR, I can just use that button.
gavv
commented
6 months ago This issue with macos checks is gone.
I think the reason was this: https://github.com/actions/setup-python/issues/577
It's not reproducing now, but I've pushed a fix that I hope will prevent it from appearing again: c55443c1e6eace80d7e37bb586678bc165b0c115
Hrick87
commented
6 months ago This issue with macos checks is gone.
I think the reason was this: actions/setup-python#577
It's not reproducing now, but I've pushed a fix that I hope will prevent it from appearing again: c55443c
The python issue was the only error message I was seeing on my end too, so the override argument should resolve it, but this is a bit beyond me. I have a lot to learn about containerization and GitHub Actions still. Anyways, as of now all the builds are passing, so I'm going to open up my PR for review. Thank you for your help.
gavv
commented
6 months ago Could you please rebase on fresh develop?
gavv
commented
6 months ago Hi, I've pushed commit 03d29eb97211ca87593566998c5087590c1bae38 to develop branch that adds new fields sample_format and pcm_format to SampleSpec.
It mostly does not affect this PR, but SampleSpec ctor now has new parameter pcm_format, which you can just set to audio::Sample_RawFormat (which means that source/sink produces/consumes samples in "raw" format - 32-bit PCM floats).
So please rebase on fresh develop.
github-actions[bot]
commented
6 months ago :robot: The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.
Hrick87
commented
5 months ago @gavv I was hoping to get some advice on one of the tests. I am trying to remove the use of core::Array
I am failing this test at the line CHECK(!sndfile_source.read(frame));
TEST(sndfile_source, eof_restart) {
core::TempFile file("test.wav");
{
test::MockSource mock_source;
mock_source.add(FrameSize * NumChans * 2);
SndfileSink sndfile_sink(arena, sink_config);
CHECK(sndfile_sink.open(NULL, file.path()));
Pump pump(buffer_factory, mock_source, NULL, sndfile_sink, FrameDuration,
SampleSpecs, Pump::ModeOneshot);
CHECK(pump.is_valid());
CHECK(pump.run());
}
SndfileSource sndfile_source(arena, source_config);
CHECK(sndfile_source.open(NULL, file.path()));
audio::sample_t frame_data[FrameSize * NumChans] = {};
audio::Frame frame(frame_data, FrameSize * NumChans);
for (int i = 0; i < 3; i++) {
CHECK(sndfile_source.read(frame));
CHECK(sndfile_source.read(frame));
CHECK(!sndfile_source.read(frame)); //This line fails
CHECK(sndfile_source.restart());
}
}My understanding of the test is that the second read() call is expected to reach eof and set eof_ to true, so that when the third read() call happens in the for loop, the call will return false. I think the issue is that the second read() call is not reaching the eof_ = true line and so the third call fails. I cannot find the issue in my code that is causing this. I've been stuck on this problem for quite some time.
I currently have:
bool SndfileSource::read(audio::Frame& frame) {
roc_panic_if(!valid_);
if (eof_) {
return false;
}
if (!file_) {
roc_panic("sndfile source: read: non-open input file");
}
audio::sample_t * frame_data = frame.samples();
sf_count_t frame_left = (sf_count_t)frame.num_samples();
sf_count_t buffer_size = frame_left;
while (frame_left > 0) {
sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);
printf("read %ld bytes\n", n_samples);
if (n_samples == 0) {
printf("eof, n_samples = 0\n");
roc_log(LogDebug, "sndfile source: got eof from sndfile");
eof_ = true;
break;
}
frame_left -= n_samples;
}
if (frame_left == (sf_count_t)frame.num_samples()) {
return false;
}
if (frame_left > 0) {
memset(frame_data, 0, (size_t)frame_left * sizeof(audio::sample_t));
}
return true;
}EDIT: I am able to pass the test by adding return false after the eof_ = true line but this does not seem like a good solution. From my understanding, this is not the intended behavior, as SoX is able to pass its respective test without that addition.
gavv
commented
5 months ago sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);First, seems that you request the whole buffer_size from sndfile, but I think you need to request only frame_left?
Second, I think you don't need a loop at all?
Maybe you can do something like:
samples_per_ch = frame.num_samples() / num_channels;
n_read = sf_read_float(file_, frame.samples(), samples_per_ch);
if (n_read == 0) {
eof = true;
}
if (n_read < samples_per_ch) {
memset(frame.samples() + n_read * num_channels, 0,
(samples_per_ch - n_read) * num_channels * sizeof(sample_t));
}
return !eof;BTW note that sf_read_float() works with number of samples per channel (a.k.a. number of frames, not to be confused with audio::Frame in roc) and frame.num_samples() is number of samples for all channels.
Also I think we need to check for error here too using sf_error_number(). If there is error, we can also add a panic with TODO comment.
gavv
commented
5 months ago If you'll have troubles you could push your code somewhere (to this PR or other branch) and I'll take a look.
Hrick87
commented
5 months ago sf_count_t n_samples = sf_read_float(file_, frame_data, buffer_size);First, seems that you request the whole buffer_size from sndfile, but I think you need to request only frame_left?
Second, I think you don't need a loop at all?
Maybe you can do something like:
samples_per_ch = frame.num_samples() / num_channels; n_read = sf_read_float(file_, frame.samples(), samples_per_ch); if (n_read == 0) { eof = true; } if (n_read < samples_per_ch) { memset(frame.samples() + n_read * num_channels, 0, (samples_per_ch - n_read) * num_channels * sizeof(sample_t)); } return !eof;BTW note that sf_read_float() works with number of samples per channel (a.k.a. number of frames, not to be confused with audio::Frame in roc) and frame.num_samples() is number of samples for all channels.
Also I think we need to check for error here too using sf_error_number(). If there is error, we can also add a panic with TODO comment.
Your code greatly simplified things for me, enough to see the problem! The check should not be if(n_samples == 0) but instead if(n_samples < frame_left) because eof can be signified by 0 or less than read's count argument. I was not accounting for that possibility. Its been fixed. Thank you!
gavv
commented
5 months ago The check should not be if(n_samples == 0) but instead if(n_samples < frame_left) because eof can be signified by 0 or less than read's count argument
Oh, really. Great!
Hrick87
commented
4 months ago @gavv I've been having some trouble refactoring the test cases to be done in a for loop. I'm only attempting to change one test as of now in test_pump.cpp. The print statements and new name method I implemented in IBackend are just for debugging purposes. This refactoring:
TEST(pump, write_read) {
{
enum { NumSamples = BufSize * 10 };
for(size_t n_backend = 0; n_backend < BackendMap::instance().num_backends(); n_backend++){
test::MockSource mock_source;
mock_source.add(NumSamples);
core::TempFile file("test.wav");
IBackend &backend = BackendMap::instance().nth_backend(n_backend);
printf("Currently on: %s\n", backend.name());
fflush(stdout);
{
IDevice *backend_device = backend.open_device(DeviceType_Sink, DriverType_File, "wav", file.path(), config, arena);
if(backend_device == NULL){
printf("Failing sink test: %s\n", backend.name());
fflush(stdout);
}
else{
printf("Passing sink test: %s\n", backend.name());
fflush(stdout);
ISink *backend_sink = backend_device->to_sink();
Pump pump(buffer_factory, mock_source, NULL, *backend_sink, BufDuration, SampleSpecs,
Pump::ModeOneshot);
CHECK(pump.is_valid());
CHECK(pump.run());
CHECK(mock_source.num_returned() >= NumSamples - BufSize);
}
}
printf("File path: %s\n", file.path());
fflush(stdout);
IDevice *backend_device = backend.open_device(DeviceType_Source, DriverType_File, "wav", file.path(), config, arena);
if(backend_device == NULL){
printf("Failing source test: %s\n", backend.name());
fflush(stdout);
continue;
}
printf("Passing source test: %s\n\n", backend.name());
ISource * backend_source = backend_device->to_source();
test::MockSink mock_writer;
Pump pump(buffer_factory,
*backend_source,
NULL,
mock_writer,
BufDuration,
SampleSpecs,
Pump::ModePermanent);
CHECK(pump.is_valid());
CHECK(pump.run());
mock_writer.check(0, mock_source.num_returned());
}
}
} // namespace rocresults in this output:
Currently on: wav
Passing sink test: wav
File path: /tmp/rocr4GtK2/test.wav
Failing source test: wav
Currently on: PulseAudio
Failing sink test: PulseAudio
File path: /tmp/rocIyNcKx/test.wav
Failing source test: PulseAudio
Currently on: sndfile
Passing sink test: sndfile
File path: /tmp/rocXWcJjr/test.wav
Passing source test: sndfile
Currently on: SoX
Passing sink test: SoX
File path: /tmp/rocNy0rfi/test.wav
Passing source test: SoX
src/tests/roc_sndio/test_pump.cpp:58: error: Failure in TEST(pump, write_read)
src/tests/roc_sndio/test_helpers/mock_sink.h:84: error:
expected <0>
but was <0.921875> threshold used was <0.0001>If I disable SoX in my build, the test passes. I also tried reverting the test cases back to their individual formats as I have them in my initial pull request and both sndfile and SoX pass. This leads me to believe something about how my for loop works is causing SoX to fail. Any ideas would be much appreciated!
gavv
commented
4 months ago The test will pass if you do this change:
// from:
ISink *backend_sink = backend_device->to_sink();
// to:
core::ScopedPtr<ISink> backend_sink(backend_device->to_sink(), arena);and:
// from:
ISource * backend_source = backend_device->to_source();
// to:
core::ScopedPtr<ISource> backend_source(backend_device->to_source(), arena);Sox sink uses buffering, and last part of buffer may not be flushed until destructor is called. By using ScopedPtr, we ensure that destructor is called in the end of the block.
Actually, I think this behavior is very unexpected. I think we should add ISink::flush() and modify Pump to call flush() when it exits from loop. For most sinks, flush() will be just no-op, but sinks that use buffering will implement it.
I've created an issue: https://github.com/roc-streaming/roc-toolkit/issues/703
Note that, apparently, you can't use:
core::ScopedPtr<IDevice> backend_device(backend.open_device(...))and instead have to use core::ScopedPtr<ISink> after you call to_sink(). That's because ISink/ISource uses virtual inheritance to derive IDevice, and pointer to IDevice has different address than pointer to ISink/ISource.
Usually we create sinks/sources via BackendDispatcher, which returns ISink/ISource, not IDevice, so this problem usually not present. However this test is a bit more low-level as it uses BackendMap directly,
Another note: in your test, WAV backend is failing because WavSource forbids providing non-empty sample spec. I guess we should use empty sample spec when constructing WavSource/SoxSource/SndfileSource, and SndfileSource should forbid non-empty spec as well.
It corresponds to this previous discussion from chat, because --rate is stored in sample spec:
given it second thought... I think we could simplify convention and just forbid using --rate argument with files at all, and just fail if rate is non-zero. (we can do it in sox too, but only in case if we're opening a file)
And final note - instead of skipping backend if it fails to open file, it'd be better to check if wav is reported by discover_drivers(), and if yes, require backend to be able to open file and fail the test if open fails. Otherwise the test can succeed if backend supports wav but there is a bug in open().
BTW, nice idea to add name() to backends!
gavv
commented
4 months ago One more follow-up issue: https://github.com/roc-streaming/roc-toolkit/issues/704
Hrick87
commented
4 months ago @gavv Hi, I've got a several last questions and then I'll be ready to submit my PR for review.
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp#L159-L161 I admittedly picked a sample rate of 48,000 simply because its a pretty high quality while not generating too large a file. Also sndfile requires we set sample rate, channels, and format before sf_write() occurs. This leads me to my next question.
Currently I only handle the case of sample_rate being empty in sndfile_sink, and it seems none of the test cases currently in roc_sndio account for a possibility where the channels or format is not set. I'm wondering if this is because its not possible for it to happen, and I don't need to worry about this.
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp#L153-L155
Sndfile_sink currently uses the SF_FORMAT_PCM_32 as its PCM format for all writes. I chose this because it has the highest compatibility with most file extensions versus the highest level of quality achievable. In map_to_sndfile() I have a section of code that essentially accounts for the only two file extensions that need a different sub format, and they first attempt the higher quality, then the lower quality, before giving up and returning false. (FORMAT_COUNT is 2).
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_sink.cpp#L100-L123
This feels hacky and probably not intended. The issue is, the only alternative I can think of is mapping the PCM format enums of roc_toolkit to the PCM format enums of sndfile and then assigning it to file_info_.format. This would be quite a large table to map, unless there's a clever way to map without a table that I'm not thinking of. I'd like your thoughts on this.
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/tests/roc_sndio/test_backend_source.cpp#L340-L358
and
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/tests/roc_sndio/test_backend_source.cpp#L270-L288
SoX supports pausing, so even with a file it should still simulate the pause from my understanding. This means different behavior is expected between SoX and other backends from the read() between pause() and restart(). I know this doesn't keep things completely generic, but I'm thinking it will have to do until is_state() issue is closed. Let me know if you think of a better solution for now.
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/tests/roc_sndio/test_backend_source.cpp#L76-L104 and https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/tests/roc_sndio/test_backend_sink.cpp#L56-L70 As you can see I tried to keep no-op in the test cases, but ran into a bit of a problem. When making them generic, this means I no longer have access to simply constructing them. backend_device only allows for opening, which is technically an operation... So essentially I just made the test to be opening and then closing the devices. Maybe a renaming of the test is in order if you do decide we should keep them like this?
https://github.com/roc-streaming/roc-toolkit/blob/e4547299059491aedeff86b2d9c74b61529999cb/src/internal_modules/roc_sndio/target_sndfile/roc_sndio/sndfile_source.cpp#L198-L202
At the time you were unaware of sf_read()'s behavior when you suggested this bit of code to me in an earlier comment. Due to the fact that sf_read() signifies end of file by returning less than the expected amount or 0, this now seems incorrect. Do I need some new condition for memset here, or is it safe to remove it entirely?
After doing some research about carbon.h I found these two relevant posts:
sndfile.c contains an assert to verify sf_count_t is 8 bytes. sf_count_t resolves to int64_t, which further resolves to a signed long. On these two ARM architectures, it seems the assert fails. I'm guessing it is because they are both 32 bit architectures. I am at a loss as to what to do about this. My research has been unhelpful, as many suggestions were to typedef the signed long variable type to int64_t to stay consistent between 32 bit and 64 bit platforms, but obviously that's already being done. Let me know if you have any ideas.
If you see anything else in the code while going over these that looks concerning or you want explanations for, please let me know. I want to try and wrap this up in one go after these questions are answered. Thank you!
gavv
commented
4 months ago Should the auto sample rate of sndfile_sink be 48000?
I think right now 44100 is a better default for us, for two reasons:
for now, CLI tools use static (predefined) RTP payload types and can't use dynamic types; there are static types for 44100, but not for 48k, so if sink/source is 48k, pipeline will do resampling (C API allows to use custom rates, but CLI tools can't do it yet)
consumer sound cards like Intel HDA usually support 44100 natively
So if 44100 is default, then on the typical case when the user runs roc-recv/roc-send on laptop or raspberry, the whole chain between file on on side and speakers/mic on the other won't need resampling.
Should I have a guard in place for number of channels and format as well?
Yes, we need something like this:
(Since we're saving sample_spec, I suggest to apply defaults to sample_spec before copying values to fileinfo).
Is setting the format to a signed 32 bit integer PCM format for all writes ok?
Yes, ideally I think we need to do this:
For now CLI tools never set format in sample spec, so we don't need to handle it in this PR.
We have two issues for handling format in backends: #696, #608.
Would be nice if you add a comment like TOTO(gh-696): map format from sample_space.
Sndfile_sink currently uses the SF_FORMAT_PCM_32 as its PCM format for all writes. I chose this because it has the highest compatibility with most file extensions versus the highest level of quality achievable. In map_to_sndfile() I have a section of code that essentially accounts for the only two file extensions that need a different sub format, and they first attempt the higher quality, then the lower quality, before giving up and returning false. (FORMAT_COUNT is 2).
This feels hacky and probably not intended.
Actually I think this logic makes sense, though maybe we can modify it a bit: instead of hard-coding specific types like SF_FORMAT_XI, we instead can create a static array of subformats sorted from high priority (like float32 as you said) to low priority (like smaller integers).
Then, no matter of the file type, we can first try use format without subformat, and then try each subformat from the list, until first successful attempt.
For now this can be the only behavior we support. Later this behavior will be used only if format is not specified in sample spec.
Is this acceptable until we have is_state() implemented?
I think that's OK.
Is noop even necessary in source and sink tests anymore/is this proper implementation if it is?
The test probably still makes sense, it checks that after we created a file using sink, we can open it using source.
So yeah, maybe just rename it to something like write_open or something.
I think this code is unnecessary, can I remove it?
I think we need the following behavior: if sndfile returns less samples than requested, we should return a frame with what we got from sndfile, padded with zeros until frame end. On the next call we should return false.
Why:
I think same behavior is implemented in Sox.
gavv
commented
4 months ago How can I resolve the build error for the two failing ARM architecture checks?
If you look here: https://github.com/roc-streaming/roc-toolkit/actions/runs/8101072211/job/22140438520?pr=660
You can see:
BUILD build/3rdparty/arm-linux-gnueabihf/gcc-4.9.4-release/sndfile-1.0.25
[download] http://www.mega-nerd.com/libsndfile/files/libsndfile-1.0.25.tar.gz
[unpack] libsndfile-1.0.25.tar.gzNow we can go to sndfile sources.
For 1.0.25: https://github.com/libsndfile/libsndfile/blob/1.0.25/src/sndfile.h.in#L318
For master: https://github.com/libsndfile/libsndfile/blob/master/include/sndfile.h#L368
So actually it's not int64_t in 1.0.25, and I guess this is a bug that was fixed in some later version.
So I think we can just bump libsndile version to the lowest one that doesn't have the bug.
Again, from this link: https://github.com/roc-streaming/roc-toolkit/actions/runs/8101072211/job/22140438520?pr=660
You can copy the very first command:
scripts/ci_checks/docker.sh rocstreaming/toolchain-arm-linux-gnueabihf:gcc-4.9 scripts/ci_checks/linux-arm/arm-linux-gnueabihf-gcc-4.9.shThis way you can run exact same CI check locally (in docker). So you can try different sndfile versions and see which one would work.
My research has been unhelpful, as many suggestions were to typedef the signed long variable type to int64_t to stay consistent between 32 bit and 64 bit platforms, but obviously that's already being done. Let me know if you have any ideas.
Yes, that platform is 32-bit, it uses 32-bit pointers and size_t, but it still supports 64-bit integers and int64_t, they're just not as efficient as 32-bit.
So it looks like sndfile is inconsistent when it first resolves sf_count_t to a 32-bit type but then requires it to be 64-bit. The latest version that just uses int64_t should work fine on 32-bit platform.
gavv
commented
4 months ago How can I resolve the build error for the two failing macOS checks?
It seems to me that this is a know issue with libsndfile version 1.0.25 on macOS that was patched in later versions of sndfile. It also looks like some people have created their own patches for version 1.0.25 (see second link). I am struggling with how I can go about fixing this issue within SConscript using this information. Let me know if you have any ideas.
If it's fixed in later version, let's just bump to minimum version that works properly.
If you know what's that version, let's try it. If you need to do some testing and don't have macOS, let me know, I could do the testing. (I hate that you can't cross-compile to mac from other systems...)
gavv
commented
4 months ago Any other concerns?
Three small comments.
if ((strcmp(format_info.extension, "wav") == 0)
|| (strcmp(format_info.extension, "mat") == 0)) {Seems that we can remove this check and just always use table? (file_type_map) If there is an entry for the format, use table, otherwise use format_info.extension.
This way the knowledge about extensions & driver will be completely isolated in the table.
And the second note, map_to_sndfile looks a bit too long, maybe we can split it into two functions? E.g. one to select format, another to select subformat.
Finally, regarding FileMap:
#pragma onceHrick87
commented
4 months ago If it's fixed in later version, let's just bump to minimum version that works properly.
If you know what's that version, let's try it. If you need to do some testing and don't have macOS, let me know, I could do the testing. (I hate that you can't cross-compile to mac from other systems...)
version 1.0.26 resolved both build issues! Everything you mentioned has been fixed. Ready for a (hopefully) final review and merge :)
gavv
commented
2 months ago Quick question.
In SndfileSink::open, you have:
sample_spec_.set_sample_rate((unsigned long)file_info_.samplerate);In which case opening file for writing may change requested rate?
(Note that the rate is never zero, we explicitly set it to 44100 in ctor if it's zero).
gavv
commented
2 months ago And another one.
Is there a reason to have this code:
if (!eof_) {
if (!seek_(0)) {
roc_log(LogError, "sndfile source: seek failed when restarting");
return false;
}
} else {
if (file_) {
close_();
}
if (!open_(path_)) {
roc_log(LogError, "sndfile source: open failed when restarting");
return false;
}
}which may reopen file, instead of just always using seek?
gavv
commented
2 months ago which may reopen file, instead of just always using seek?
Oh, I see, some file types are unseekable.
gavv
commented
2 months ago Thanks a lot for PR! That was big!
I have pushed a few follow-up patches:
246