Relaxing roc_lib test #283
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @alexandremgo! Thanks for the PR! I will take a look tomorrow morning and answer to your questions. |
|
Hi @alexandremgo,
Receiver may have some kind of current sample value that will be used to ensure that we receive the required sample. On the receiver side the algorithm will be the following:
You can use an atomic flag as: class Sender {
public:
void stop() {
stopped_ = 1;
}
private:
virtual void run() {
while (!stopped_) {
// send a signal
}
}
core::Atomic stopped_;
// ... other stuff
};
The only requirement that this test should not be too long because it is the unit test. Lets make some experiments and measure how much time do we need to receive 1000, 10000, 100000 non-zero samples.
No |
gavv
reviewed
Nov 4, 2019
I'd prefer that unit tests for each module were taking no more than 200-500ms on an unloaded system. So in one test we can retrieve, say, about 100ms of samples. On 44100 Hz it would be 4410 samples. We likely also want it to be a multiple of packet length * FEC block size, so |
We should think whether it can happen on floats in a reasonable time. If it can't, there is no need to handle it. |
Currently this is the only supported encoding. In future we will add more. |
gavv
added
work in progress
|
During the test I often (especially when the samples that have to be received - N - is put to more than Moreover, in some cases (when displaying the log messages, or having a low N) I have the pure virtual method error I had once: But i have no error when running the test without valgrind |
|
When using |
|
Great, so we have a backtrace. I've created #285. |
It would help if in case of failure you printed the two sample streams passed to sender and retrieved by receiver. Maybe at least the last 100 or 1000 samples. So we can see what is the problem: rounding error, shifted stream, garbage inside the stream, etc. |
|
The sample step is Logs from the receiver side: Example 1: Example 2: The problem seems to start with the |
|
Huh. These two lines mean that the incoming queue size on the receiver became larger than 3000 samples and it dropped the session and started producing zeros because there were no sessions anymore. Then, shortly, the session was auto-created again, but since the stream was now shifted, you got an incorrect sample. The minimum and maximum boundaries may be configured using max_latency_overrun and max_latency_underrun in roc_receiver_config. Currently this test uses default values. The default maximum is The incoming queue, in average, should be about |
Currently the value of In the Sender side, I send arrays of samples of size while (!stopped_) {
for (size_t i = 0; i < total_samples_; ++i) {
samples[i] = sample_value;
sample_value = increment_sample_value(sample_value, sample_step_);
}
for (size_t off = 0; off < total_samples_; off += frame_size_) {
if (off + frame_size_ > total_samples_) {
off = total_samples_ - frame_size_;
}
roc_frame frame;
memset(&frame, 0, sizeof(frame));
frame.samples = samples + off;
frame.samples_size = frame_size_ * sizeof(float);
const int ret = roc_sender_write(sndr_, &frame);
roc_panic_if_not(ret == 0);
}
} |
It's in nanoseconds: https://roc-project.github.io/roc/docs/api/reference.html#c.roc_receiver_config::target_latency
At the first glance the code looks good. |
|
You can temporary increase the maximum allowed latency (set it to some very big value) and monitor the incoming queue size on the receiver. When resampler is disabled (as in our test) the queue size is reported here: https://github.com/roc-project/roc/blob/master/src/modules/roc_audio/latency_monitor.cpp#L214-L219 Currently it is reported every 5 seconds: https://github.com/roc-project/roc/blob/master/src/modules/roc_audio/latency_monitor.cpp#L18 But you can hack it to report more frequently. You can also hack your test to run longer. Then we can inspect what is the trend. Maybe the queue size is constantly and slowly increasing. This would mean that the sender is faster than the receiver for some reason. Maybe it's just a fluctuation (e.g. caused by system load or valgrind) and the rate is OK in average but sometimes either sender or receiver are stuck for a while. |
|
Monitoring the latency / receiver queue size (every milliseconds) First ex: default maximum allowed latency x 10 The Sender already sent 88 arrays of samples before the Receiver receive its first sample. is really interesting 2nd example: default maximum allowed latency We can see the same behavior: Am I doing something wrong ? Or is the queue size inversely proportional to the latency ? |
|
@alexandremgo Hi, sorry for late replay. I'm freelancing this month so have a little free time... Thanks for this experiment! Let's see what is happening here. First, you have found a bug in latency reporting code: 4294967196 is an overflowed negative value. Here is a fix: Second, we can see that the watchdog was triggered: ...and dropped the session. This means that during last 30000 samples all packets were dropped and thus all samples were blank. Then the session was restarted, the stream was shifted, and we got the same panic as before. (The watchdog parameters can be configured via API as well. See roc_receiver_config). Also we can see that the rtp validator is triggered: This means that it detected a stream jump: the previous packet had sequence number N, but the next packet had sequence number > N+100. When such a jump is detected, rtp validator drops the packet. I've checked out your branch and tried to entirely disable latency monitor (audio::LatencyMonitor::update), watchdog (audio::Watchdog::update), and rtp validator (rtp::Validator::read). After doing this, the test stopped panicking and now runs forever. Here is how the queue size (the "latency") is changing:
So the queue size decreased dramatically on the beginning and then was slowly increasing. The logs are full of these messages: Now let's put all these stuff together. It seems that the sender thread was stuck for a while (likely because of the valgrind), and the receiver stream position went ahead of the sender stream position. This led to the situation when the sender is always behind the receiver and its packets are always outdated for the receiver. The receiver, in turn, always have to drop the packets and produces zero samples. Normally this situation is detected by the latency monitor or watchdog and the session is restarted. We tried to disable them and the session was stuck in this half-dead state. Conclusions:
|
alexandremgo
force-pushed
the
relax_roc_lib_test
branch
from
bdb25e2
to
4c751e9
Compare
|
Thanks for the explanations @gavv, and no pb ;) I've added your fix for the latency overflowed negative value, and relaxed the test by allowing stream shifts. After that, the test was successful, but I was ending up again with the pure virtual method call error of issue #285. So I've re-ordered the Proxy field as advised. It seems to fix the problem. Without valgrind, the test works correctly. But with valgrind, the test never terminates. We receive enough samples, and the Sender does join. But then the test is stuck: As the Sender seems to join, I don't understand why the test does not terminate. |
|
On a successful test from the branch develop, we have: Compared to the logs i've posted on the previous comment, it seems that on my branch, the roc_receiver does not close. And therefore the [update] For now i've added a hack: a stop function for the Receiver that reads an arbitrary number of frames. And it does make the tests successful What would be a cleaner way to fix that ? |
|
Here is the backtrace of the stucked test (it is printed if we send SIGTERM to it): |
|
It seems it is the same bug in Transceiver as described here: #277. |
|
@alexandremgo After reading your comment and experimenting with your branch it became clear what is the source of the problem in #277 and how we can fix it. See my last comment there. So the proper way to fix this test is to fix #277. If you wish, you can pick that issue up. Currently nobody is working on it. Or we can merge this PR without the workaround and just don't enable valgrind on travis until we will fix the bug. |
alexandremgo
force-pushed
the
relax_roc_lib_test
branch
from
84fba6e
to
7643fa6
Compare
|
The PR is ready for review ;) |
gavv
added
ready for review
gavv
reviewed
Nov 28, 2019
|
LGTM! (except one nitpick above). Tested it locally. Works good, even when running 10 concurrent instances (earlier it failed already on 6 concurrent instances). @alexandremgo Could you also try to enable roc-lib-test in valgrind travis checks? It is disabled here: |
gavv
added
needs revision
alexandremgo
force-pushed
the
relax_roc_lib_test
branch
from
3c8b01f
to
acaca17
Compare
|
It would be also nice to squash all test-related commits into a single one. Sorry, forgot to mention it before. |
Indefinitely read samples until we accumulate N non-zero samples Enable roc-lib-test in valgrind travis checks
alexandremgo
force-pushed
the
relax_roc_lib_test
branch
from
acaca17
to
0c1665a
Compare
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Working on issue #278
I have several questions:
The samples array is not defined in
TEST_GROUPanymore. It is defined inSenderwith a value that increases bysample_step_in an infinite loop. InReceiverI check if the sample received is equal to a multiple (maybe ? for now i only check if it is equal to acurrent valueorcurrent value + sample_step_) ofsample_step_. Is it a good idea ?If the test succeeds, as the
Sendersends infinitely a signal, theSendernever joins. And the test never ends. Do you have any idea how I could stop it ?What value should we put for
Nfor theReceiver(Ndefined by: theReceivershould accumulateNnon-zero samples)Thanks!
PS: I did not handle the possible overflow yet
PS2: Does another mode than
ROC_FRAME_ENCODING_PCM_FLOATexists forroc_frame_encoding(I did not see another one in the doc) ?