configure internal frame size in nanoseconds

[hrishikeshSuresh]

Referring issue #164 I ran $ roc-recv -vv -s rtp+rs8m::10001 -r rs8m::10002 --frame-size="100" and $ roc-send -vv -s rtp+rs8m:127.0.0.1:10001 -r rs8m:127.0.0.1:10002 -i ./file.wav --frame-size="100" The audio was streamed successfully and did not encounter any issues.

[gavv]

Thanks for the PR! Will take a look tonight or tomorrow.

[gavv]

BTW, the build is failing.

[hrishikeshSuresh]

I just noticed that I didn't update src/modules/mixer/test_mixer.cpp and other files that use Mixer. I will work on it now.

[hrishikeshSuresh]

I think I might have made a slight mistake here. frame_size is a parameter for ResamplerReader. Should it be of type core::nanoseconds_t or size_t ? Because in the above commits, I thought it should have been changed to core::nanoseconds_t and changed ResamplerReader.

[gavv]

I guess it should be nanoseconds_t. BTW I'd change the name from frame_size to frame_length to make it clear that it is something other than just the number of samples.

[hrishikeshSuresh]

I will put up a message here once I fix all these errors.

[hrishikeshSuresh]

You can review all the changes made. I think I fixed almost all errors I had made earlier.

[gavv]

A bit busy currently, will take a look in a few days. Could you please fix tests? (see travis build).

[gavv]

Also, please rebase on fresh develop.

[hrishikeshSuresh]

A bit busy currently, will take a look in a few days. Could you please fix tests? (see travis build).

Yes, I'm trying to fix it. It looks like CHECK(mixer.valid()) is failing. Is it because mixer.valid() is returning false?

[gavv]

Is it because mixer.valid() is returning false?

Yep.

[gavv]

Try to run roc-test-audio -v or roc-test-audio -v -g mixer -n <testname>.

[hrishikeshSuresh]

A bit busy currently, will take a look in a few days. Could you please fix tests? (see travis build).

I looked into it and the error occurs in src/modules/roc_audio/mixer.cpp, where we are checking whether frame_size is greater than temp_buf size (line 45). When I printed the values while running the tests, the frame size = 618 and temp_buf size = 500. So do we have to change the values of frame length or buffer size in the tests?

[gavv]

Ah, I see.

Earlier mixer was configured with frame size (in samples) and now it is configured with frame length (in nanoseconds). But mixer tests still pass the old value, MaxSz, which is in samples and is equal to 500.

Mixer treats it as 500 ns, converts it to samples, allocates a buffer from pool, and checks that the buffer is large enough to hold the frame. The pool is configured to allocate 500-samples buffers, but 500 ns is larger than 500 samples.

So to fix it, we should update tests and pass correct frame length to mixer. We should pass a value in nanoseconds that corresponds to MaxSz (500) samples.

[gavv]

Could you please rebase on fresh develop and make the history linear?

[hrishikeshSuresh]

I have made the history linear right now. Will start making the changes that were requested in the previous commit.

[gavv]

Ping me when this will be ready for review. Don't forget to remove debugging prints, run scons fmt, pull fresh develop and rebase on it, and squash all commits into one or several :)

UPD: and fix travis build, of course.

[hrishikeshSuresh]

It looks like the build is failing due to src/tests/roc_audio/test_mock_reader.h (which I did not edit), showing error: iteration 65536 invokes undefined behavior [-Werror=aggressive-loop-optimizations]. Any idea as to why this is happening here, as this file should be unaffected by the changes I have made in the other files?

[gavv]

That code did not change, but the code using it did, and gcc detected that the new usage is incorrect. Actually it found a bug in tests that I already mentioned: you incorrectly pass frame length in nanoseconds, instead of frame size in bytes, to mock reader, which expects number of samples. The value in nanoseconds is very high compared to number of samples, and exceeds MaxSz in mock reader, causing a buffer overflow. Gcc detected that.

[gavv]

It seems that the file mode of lots of files was changed, unintentionally I guess:

Could you revert the mode change?

[hrishikeshSuresh]

It seems that the file mode of lots of files was changed, unintentionally I guess:

Could you revert the mode change?

Oh, I will fix it right now.

[gavv]

Serious work! A few more minor comments... I think we're at the finish line though.

PS. BTW the file mode change was not fully reverted, see https://github.com/roc-project/roc/pull/274/files

[gavv]

Please also revert file mode change for src/tests/roc_rtp/test_packets/generate_headers.py.

[gavv]

@hrishikeshSuresh this PR is steadily accumulating conflicts, which will make rebasing harder and harder... Do you have some time to finish it? If you're out of time, I could try rebase it and fix the remaining issue with sample rate. IIRC it's the only unresolved issue in this PR.

[hrishikeshSuresh]

I am working on it. I think I'll push the changes within 2 days.

[gavv]

OK, thanks!

[hrishikeshSuresh]

I'll start rebasing it now.

[hrishikeshSuresh]

I think you can review the PR now! Only changes I had to make was in roc_sndio/target_sox/sox_source.cpp and sox_sink.cpp and updated the tests in roc_sndio, to get device rate from sox/pulseaudio.

[gavv]

Great. But could you rebase on fresh develop? The PR have 30 commits currently, and I guess it should contain only one commit (the last one). I think the easiest way to do it is to pull fresh develop, switch to it, and cherry-pick your commit there.

Also, there are some unintentional changes in this PR. E.g.: roc_recv.rst (unintentional new line removal?), src/lib/src/private.h (it was deleted, PR restores it), src/lib/src/sender.cpp (it was moved, PR restores it), src/modules/roc_address/parse_socket_addr.cpp~7f5219a563768d18bf2a4834935915fd4b68ecfa (what is this), and so on. After rebasing, please go through your diff and cleanup it.

[gavv]

Thanks!