roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.03k stars 205 forks source link

Frame timestamps #539

Closed gavv closed 7 months ago

gavv commented 11 months ago
gavv commented 10 months ago

ResamplerReader and ResamplerWriter both had similar bug: they produced negative CTS when their input frames had zero CTS. Instead, they should produce zero CTS until there is first input frame with non-zero CTS (it's a legit case).

Similar bug was present in ChannelMapperWriter, PipelineLoop, and Packetizer.

Fixes:

I also added some panics and validator checks to catch this kind of bugs earlier: 806ffcd753f3c7f30ea6ae76c0110bc4da09c81a

@baranovmv Could you please adjust tests for ResamplerReader, ResamplerWriter, and Packetizer, to catch this bug?

I already covered it in tests for ChannelMapperWriter and PipelineLoop in commits above.

gavv commented 10 months ago

I've pushed some commits related to this issue, most interesting are these:

gavv commented 10 months ago

I've implemented pipeline-level tests for timestamp forwarding: 4e9013661b48f223b4bcfb795eb05d8a9b462719

New tests:

All bugs mentioned above were found by these tests, so pipeline tests are super useful, as usual.

So currently most of the tests pass and CTS feature mostly works (hooray!), however two tests, marked red, still fail. These two tests enable resampling (unlike all other tests), i.e. the bug is related to ResamplerReader and ResamplerWriter.

If I make input & output sample rate equal in the failing tests, they will pass. If I change allowed CTS error from one microsecond to one millisecond (search for TimestampEpsilon constant), they will pass too. But millisecond error looks suspicious and I guess indicates a bug either in tests or in code.

@baranovmv Could you take a look at those tests? If there is a bug indeed, could you also add resampler-level test that reproduces it?

gavv commented 10 months ago

We should check if Depacketizer correctly handles case when first packet for frame had zero CTS, and second packet for the same frame had non-zero CTS.

I think it's not covered right now.

We need a test for this and maybe a fix.

gavv commented 10 months ago

Crash in roc-send if repair endpoint is not provided, but source endpoint uses rtp+rs8m instead of rtp.

roc-send -v -s rtp+rs8m://lo:10001 -c rtcp://lo:10003 -i file:stash/loituma.wav 
02:48:43.450 [3097326] [inf] roc_sndio: sox source: opening: driver=(null) path=stash/loituma.wav
02:48:43.450 [3097326] [inf] roc_sndio: sox source: in_bits=16 out_bits=32 in_rate=44100 out_rate=0 in_ch=2 out_ch=0 is_file=1
02:48:43.450 [3097326] [inf] roc_pipeline: sender sink: adding slot
02:48:43.450 [3097326] [inf] roc_node: sender node: connecting audiosrc interface of slot 0 to rtp+rs8m://lo:10001
02:48:43.451 [3097326] [inf] roc_node: sender node: bound audiosrc interface to 0.0.0.0:49882
02:48:43.451 [3097326] [inf] roc_node: sender node: connecting audioctl interface of slot 0 to rtcp://lo:10003
02:48:43.451 [3097326] [inf] roc_node: sender node: bound audioctl interface to 0.0.0.0:52258

src/internal_modules/roc_core/optional.h:58: error: roc_panic()

ERROR: roc_pipeline: optional: attempt to dereference unitialized object

Backtrace:
#1: 0x5632b22f72ee roc::core::print_backtrace_full()+0x6e
#2: 0x5632b22f5dd2 roc::core::die_gracefully(char const*, bool)+0x72
#3: 0x5632b22ef6cd roc::core::panic(char const*, char const*, int, char const*, ...)+0x18d
#4: 0x5632b21feedf roc::core::Optional<roc::rtp::TimestampExtractor, 232ul>::operator->() const+0x3f
#5: 0x5632b21fdbbe roc::pipeline::SenderSession::update(long)+0x3e
#6: 0x5632b21f8164 roc::pipeline::SenderSlot::update(long)+0x24
#7: 0x5632b21f6923 roc::pipeline::SenderSink::update(long)+0x73
#8: 0x5632b21f5de8 roc::pipeline::SenderLoop::process_subframe_imp(roc::audio::Frame&)+0x68
#9: 0x5632b21f413b roc::pipeline::PipelineLoop::process_next_subframe_(roc::audio::Frame&, unsigned long*)+0x11b
#10: 0x5632b21f3d5d roc::pipeline::PipelineLoop::process_subframes_and_tasks_precise_(roc::audio::Frame&)+0x7d
#11: 0x5632b21f3caf roc::pipeline::PipelineLoop::process_subframes_and_tasks(roc::audio::Frame&)+0x2f
#12: 0x5632b21f5c9d roc::pipeline::SenderLoop::write(roc::audio::Frame&)+0xcd
#13: 0x5632b22018aa roc::sndio::Pump::run()+0x46a
#14: 0x5632b21e1c3e main+0x1b7e
#15: 0x7f9f1fe461ca __libc_init_first+0x8a
#16: 0x7f9f1fe46285 __libc_start_main+0x85
#17: 0x5632b21dfff1 _start+0x21
zsh: IOT instruction  roc-send -v -s rtp+rs8m://lo:10001 -c rtcp://lo:10003 -i 
gavv commented 10 months ago

Crash in roc-send if repair endpoint is not provided, but source endpoint uses rtp+rs8m instead of rtp.

Fixed.

gavv commented 10 months ago

I've found another bug (reproduced in pipeline tests): when packet CTS is non-zero, but very small, Depacketizer, ResamplerReader, and ResamplerWriter can produce negative timestamps.

I've pushed a fix: 76df2ecb512cc1ffcafdcca5dda7963cfad27ba1

@baranovmv Could you take a look if we can cover these 3 cases with unit tests (for Depacketizer, ResamplerReader, and ResamplerWriter)?

gavv commented 7 months ago

Done!