fix(network): stop handing librist a byte count as buffer_size - #8
Conversation
Every rist:// ingest failed to open with "Result too large":
[irl-source] Connecting to: rist://.../
[irl-source] Failed to open input: Result too large
[irl-source] Reconnecting in 2s...
apply_demuxer_options set "buffer_size" to network_buffer_mb in bytes
for every URL. Four things in FFmpeg 9.0's libavformat declare that
option name, and three of them agree it is a byte count: udp.c (which
feeds it to setsockopt SO_RCVBUF), rtpproto.c and rtsp.c (which both
forward it to the udp:// they open). librist.c is the outlier — there it
is the RIST recovery window in milliseconds, declared 0..30000:
{ "buffer_size", "set buffer_size in ms", OFFSET(buffer_size),
AV_OPT_TYPE_INT, {.i64=0}, 0, 30000, .flags = D|E },
2 MB is 2097152, so av_opt_set_dict() on the URLContext rejected it with
AVERROR(ERANGE) and ffurl_open failed before librist opened a socket.
The byte-semantic protocols all declare -1..INT_MAX and had been taking
the value silently, which is why only RIST surfaced it. rist:// now
keeps librist's own recovery default, tuned per stream by the URL's
buffer= parameter that rist_parse_address2() reads.
While here, extend the receive buffer to the TCP-based protocols. It was
only ever applied to srt:// as "recv_buffer_size", but tcp.c declares
that same option (SO_RCVBUF), and rtmp_open, http_open_cnx and
ff_tls_open_underlying each thread the caller's option dictionary down
into the transport they open. So one unconditional set now gives the
buffer effect on rtmp(s)://, http(s):// and tcp:// as well, where
"buffer_size" had been landing in the dictionary unread. Only tcp.c and
libsrt.c declare the name and both are -1..INT_MAX, so it cannot ERANGE
the way buffer_size did.
The srt:// test also moves from strstr(url, "srt://") to a scheme-prefix
check, so a path segment or query parameter can no longer decide which
protocol options apply.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe receiver stream now detects URL schemes only at the URL start. Network buffer options apply according to protocol, and SRT latency uses exact scheme detection. ChangesNetwork option handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Every rist:// ingest failed to open with "Result too large":
[irl-source] Connecting to: rist://.../
[irl-source] Failed to open input: Result too large
[irl-source] Reconnecting in 2s...
apply_demuxer_options set "buffer_size" to network_buffer_mb in bytes for every URL. Four things in FFmpeg 9.0's libavformat declare that option name, and three of them agree it is a byte count: udp.c (which feeds it to setsockopt SO_RCVBUF), rtpproto.c and rtsp.c (which both forward it to the udp:// they open). librist.c is the outlier — there it is the RIST recovery window in milliseconds, declared 0..30000:
{ "buffer_size", "set buffer_size in ms", OFFSET(buffer_size),
AV_OPT_TYPE_INT, {.i64=0}, 0, 30000, .flags = D|E },
2 MB is 2097152, so av_opt_set_dict() on the URLContext rejected it with AVERROR(ERANGE) and ffurl_open failed before librist opened a socket. The byte-semantic protocols all declare -1..INT_MAX and had been taking the value silently, which is why only RIST surfaced it. rist:// now keeps librist's own recovery default, tuned per stream by the URL's buffer= parameter that rist_parse_address2() reads.
While here, extend the receive buffer to the TCP-based protocols. It was only ever applied to srt:// as "recv_buffer_size", but tcp.c declares that same option (SO_RCVBUF), and rtmp_open, http_open_cnx and ff_tls_open_underlying each thread the caller's option dictionary down into the transport they open. So one unconditional set now gives the buffer effect on rtmp(s)://, http(s):// and tcp:// as well, where "buffer_size" had been landing in the dictionary unread. Only tcp.c and libsrt.c declare the name and both are -1..INT_MAX, so it cannot ERANGE the way buffer_size did.
The srt:// test also moves from strstr(url, "srt://") to a scheme-prefix check, so a path segment or query parameter can no longer decide which protocol options apply.
Summary by CodeRabbit