Skip to content

cli tools: fix frame size estimation when frame_length isn't provided#831

Open
baranovmv wants to merge 2 commits into
roc-streaming:developfrom
baranovmv:fix/auto_frame_sz
Open

cli tools: fix frame size estimation when frame_length isn't provided#831
baranovmv wants to merge 2 commits into
roc-streaming:developfrom
baranovmv:fix/auto_frame_sz

Conversation

@baranovmv

Copy link
Copy Markdown
Member

In multi channel scenario a frame sz become too big and Frame Factory refuses to allocate frames. Here the max frame size is estimated similar way to max packet sz estimation.

@baranovmv baranovmv requested a review from gavv February 28, 2026 22:22
@rocstreaming-bot rocstreaming-bot added the S-ready-for-review status: PR can be reviewed label Feb 28, 2026
@rocstreaming-bot

Copy link
Copy Markdown

🤖 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.

@gavv gavv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Comment thread src/tools/roc_copy/main.cpp Outdated
return spec.ns_2_samples_overall(io_config.frame_length) * sizeof(audio::sample_t);
core::nanoseconds_t len = io_config.frame_length;
if (len == 0) {
len = 10 * core::Millisecond;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add:

DefaultFrameLength = 10 * core::Millisecond;

to roc_sndio/io_config.h and remove it from:

  • roc_sndio/io_pump.cpp
  • roc_sndio/target_pulseaudio/roc_sndio/pulseaudio_device.cpp
  • roc_sndio/target_sox/roc_sndio/sox_sink.cpp
  • roc_sndio/target_sox/roc_sndio/sox_source.cpp

(all these definitions have same value of 10ms)

...and reuse it in both main.cpp

@gavv

gavv commented Jun 25, 2026

Copy link
Copy Markdown
Member

Also please rebase on fresh develop to fix CI

@gavv gavv added this to the 0.5.0 milestone Jun 25, 2026
@gavv

gavv commented Jun 25, 2026

Copy link
Copy Markdown
Member

If you remember a command that reproduces the bug, could you create an issue with command and output? I didn't see this issue in my tests. Nice to have, but not critical.

@gavv gavv added S-needs-revision status: Author should revise PR and address feedback and removed S-ready-for-review status: PR can be reviewed labels Jun 25, 2026
In multi channel scenario a frame sz become too big and Frame Factory
refuses to allocate frames. Here the max frame size is estimated
similar way to max packet sz estimation.
@baranovmv baranovmv force-pushed the fix/auto_frame_sz branch from 7b5f737 to 6c0f5d9 Compare June 26, 2026 13:49
@baranovmv

Copy link
Copy Markdown
Member Author

Fixes #821

@baranovmv

Copy link
Copy Markdown
Member Author

@gavv

If you remember a command that reproduces the bug, could you create an issue with command and output? I didn't see this issue in my tests. Nice to have, but not critical.

The issue itself needs arguebly a better solution. This PR proposes a hot-fix for basic and straightforward scenarios in our CLI tools. So at the moment there is no easy way to verify the fix proposed in this PR with our unit tests.

IMHO the issue reveals a usability problem in our public API and as a consequence lead to a corresponding issue in command line parameter of CLI tools.

Let me elaborate on a "better solution" a bit:

  • I'd rather see MTU (size in bytes) parameter instead of roc_sender_config::packet_length (duration)
  • roc_context_config::max_frame_size in it's order could be deduced from MTU, but sometimes it'd make more sense to deduce it from the application. E.g. for pipewire modules it'd be great to tie max_frame_size to quants

@baranovmv baranovmv added S-ready-for-review status: PR can be reviewed and removed S-needs-revision status: Author should revise PR and address feedback labels Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-ready-for-review status: PR can be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants