Message ID | 20201223233253.14917-1-onemda@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] libavformat: add librist protocol | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
On 12/23/2020 8:32 PM, Paul B Mahol wrote: > +static int librist_close(URLContext *h) > +{ > + RISTContext *s = h->priv_data; > + int ret = 0; > + > + s->peer = NULL; > + > + if (s->rist_ctx) > + ret = rist_destroy(s->rist_ctx); > + if (ret < 0) > + return risterr2ret(ret); > + s->rist_ctx = NULL; > + > + if (s->peer_config) > + free((void *)s->peer_config); > + s->peer_config = NULL; > + > + if (s->logging_settings) > + free((void *)s->logging_settings); This is not a good idea. Afaik there could be more than one c library at play, the one linked by librist, and the one linked by libavformat. This is why most libraries always end up adding custom free functions. I see that rist_logging_set(), or anything in logging.h from this library for that matter, is undocumented, but reading the source code it seems it accepts your own allocated rist_logging_settings struct. So IMO first consider asking for some doxy addition to the functions in order to make that officially supported, and then allocate it yourself in librist_open() with av_malloc(sizeof(*s->logging_settings)). Alternatively, they could provide a new rist_logging_free() function of sorts. And same for peer_config above, except that rist_parse_address() does not accept your own allocated rist_peer_config struct. It only accepts an already initialized one from a previous call, so either that's changed, or a custom free() or a reset_to_defaults() function is added. > + s->logging_settings = NULL; > + > + return 0; > +} > + > +static int librist_open(URLContext *h, const char *uri, int flags) > +{ > + RISTContext *s = h->priv_data; > + int ret; > + > + ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr); > + if (ret < 0) > + return risterr2ret(ret);
On 12/23/2020 8:53 PM, James Almer wrote: > On 12/23/2020 8:32 PM, Paul B Mahol wrote: >> +static int librist_close(URLContext *h) >> +{ >> + RISTContext *s = h->priv_data; >> + int ret = 0; >> + >> + s->peer = NULL; >> + >> + if (s->rist_ctx) >> + ret = rist_destroy(s->rist_ctx); >> + if (ret < 0) >> + return risterr2ret(ret); >> + s->rist_ctx = NULL; >> + >> + if (s->peer_config) >> + free((void *)s->peer_config); >> + s->peer_config = NULL; >> + >> + if (s->logging_settings) >> + free((void *)s->logging_settings); > > This is not a good idea. Afaik there could be more than one c library at > play, the one linked by librist, and the one linked by libavformat. > This is why most libraries always end up adding custom free functions. > > I see that rist_logging_set(), or anything in logging.h from this > library for that matter, is undocumented, but reading the source code it > seems it accepts your own allocated rist_logging_settings struct. > So IMO first consider asking for some doxy addition to the functions in > order to make that officially supported, and then allocate it yourself > in librist_open() with av_malloc(sizeof(*s->logging_settings)). > Alternatively, they could provide a new rist_logging_free() function of > sorts. I guess you could also just keep it in heap instead of allocating it. No need to free it that way since it'll be part of RISTContext. > > And same for peer_config above, except that rist_parse_address() does > not accept your own allocated rist_peer_config struct. It only accepts > an already initialized one from a previous call, so either that's > changed, or a custom free() or a reset_to_defaults() function is added. Not so much this one, for the same reason you can't pass a freshly allocated one. > >> + s->logging_settings = NULL; >> + >> + return 0; >> +} >> + >> +static int librist_open(URLContext *h, const char *uri, int flags) >> +{ >> + RISTContext *s = h->priv_data; >> + int ret; >> + >> + ret = rist_logging_set(&s->logging_settings, s->log_level, >> log_cb, h, NULL, stderr); >> + if (ret < 0) >> + return risterr2ret(ret); >
Paul B Mahol (12020-12-24): > This work is sponsored by Open Broadcast Systems. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > configure | 5 + > doc/protocols.texi | 32 ++++++ > libavformat/Makefile | 1 + > libavformat/librist.c | 233 ++++++++++++++++++++++++++++++++++++++++ > libavformat/protocols.c | 1 + > 5 files changed, 272 insertions(+) > create mode 100644 libavformat/librist.c > > diff --git a/configure b/configure > index 90914752f1..462f2dcf64 100755 > --- a/configure > +++ b/configure > @@ -259,6 +259,7 @@ External library support: > --enable-libpulse enable Pulseaudio input via libpulse [no] > --enable-librabbitmq enable RabbitMQ library [no] > --enable-librav1e enable AV1 encoding via rav1e [no] > + --enable-librist enable RIST via librist [no] > --enable-librsvg enable SVG rasterization via librsvg [no] > --enable-librubberband enable rubberband needed for rubberband filter [no] > --enable-librtmp enable RTMP[E] support via librtmp [no] > @@ -1797,6 +1798,7 @@ EXTERNAL_LIBRARY_LIST=" > libpulse > librabbitmq > librav1e > + librist > librsvg > librtmp > libshine > @@ -3488,6 +3490,8 @@ unix_protocol_select="network" > # external library protocols > libamqp_protocol_deps="librabbitmq" > libamqp_protocol_select="network" > +librist_protocol_deps="librist" > +librist_protocol_select="network" > librtmp_protocol_deps="librtmp" > librtmpe_protocol_deps="librtmp" > librtmps_protocol_deps="librtmp" > @@ -6404,6 +6408,7 @@ enabled libopus && { > enabled libpulse && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new > enabled librabbitmq && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection > enabled librav1e && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new > +enabled librist && require_pkg_config librist "librist >= 0.2" librist/librist.h rist_receiver_create > enabled librsvg && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo > enabled librtmp && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket > enabled librubberband && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++" > diff --git a/doc/protocols.texi b/doc/protocols.texi > index de377a9546..aa682f1586 100644 > --- a/doc/protocols.texi > +++ b/doc/protocols.texi > @@ -673,6 +673,38 @@ Example usage: > -f rtp_mpegts -fec prompeg=l=8:d=4 rtp://@var{hostname}:@var{port} > @end example > > +@section rist > + > +Reliable Internet Streaming Transport protocol > + > +The accepted options are: > +@table @option > +@item rist_profile > +Supported values: > +@table @samp > +@item simple > +@item main > +This one is default. > +@item advanced > +@end table > + > +@item buffer_size > +Set internal RIST buffer size for retransmission of data. > + > +@item pkt_size > +Set internal RIST buffer size for receiving and sending data. > + > +@item log_level > +Set loglevel for RIST logging messages. > + > +@item secret > +Set override of encryption secret, by default is unset. > + > +@item encryption > +Set encryption type, by default is disabled. > +Acceptable values are 128 and 256. > +@end table > + > @section rtmp > > Real-Time Messaging Protocol. > diff --git a/libavformat/Makefile b/libavformat/Makefile > index 97d868081b..799e16c59e 100644 > --- a/libavformat/Makefile > +++ b/libavformat/Makefile > @@ -652,6 +652,7 @@ OBJS-$(CONFIG_UNIX_PROTOCOL) += unix.o > > # external library protocols > OBJS-$(CONFIG_LIBAMQP_PROTOCOL) += libamqp.o > +OBJS-$(CONFIG_LIBRIST_PROTOCOL) += librist.o > OBJS-$(CONFIG_LIBRTMP_PROTOCOL) += librtmp.o > OBJS-$(CONFIG_LIBRTMPE_PROTOCOL) += librtmp.o > OBJS-$(CONFIG_LIBRTMPS_PROTOCOL) += librtmp.o > diff --git a/libavformat/librist.c b/libavformat/librist.c > new file mode 100644 > index 0000000000..dacdd4a859 > --- /dev/null > +++ b/libavformat/librist.c > @@ -0,0 +1,233 @@ > +/* > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +/** > + * @file > + * Reliable Internet Streaming Transport protocol > + */ > + > +#include "libavutil/avassert.h" > +#include "libavutil/opt.h" > +#include "libavutil/parseutils.h" > +#include "libavutil/time.h" > + > +#include "avformat.h" > +#include "internal.h" > +#include "network.h" > +#include "os_support.h" > +#include "url.h" > + > +#include <librist/librist.h> > + > +typedef struct RISTContext { > + const AVClass *class; > + > + int profile; > + int buffer_size; > + int packet_size; > + int log_level; > + int encryption; > + char *secret; > + > + struct rist_logging_settings *logging_settings; > + struct rist_peer_config *peer_config; > + struct rist_peer *peer; > + struct rist_ctx *rist_ctx; > +} RISTContext; > + > +#define D AV_OPT_FLAG_DECODING_PARAM > +#define E AV_OPT_FLAG_ENCODING_PARAM > +#define OFFSET(x) offsetof(RISTContext, x) > +static const AVOption librist_options[] = { > + { "rist_profile","set profile", OFFSET(profile), AV_OPT_TYPE_INT, {.i64=RIST_PROFILE_MAIN}, 0, 2, .flags = D|E, "profile" }, > + { "simple", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_SIMPLE}, 0, 0, .flags = D|E, "profile" }, > + { "main", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_MAIN}, 0, 0, .flags = D|E, "profile" }, > + { "advanced", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_ADVANCED}, 0, 0, .flags = D|E, "profile" }, > + { "buffer_size", "set buffer_size", OFFSET(buffer_size), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX, .flags = D|E }, > + { "pkt_size", "set packet size", OFFSET(packet_size), AV_OPT_TYPE_INT, {.i64=9968}, 1, 9968, .flags = D|E }, > + { "log_level", "set loglevel", OFFSET(log_level), AV_OPT_TYPE_INT, {.i64=-1}, -1, INT_MAX, .flags = D|E }, > + { "secret", "set encryption secret",OFFSET(secret), AV_OPT_TYPE_STRING,{.str=NULL}, 0, 0, .flags = D|E }, > + { "encryption","set encryption type",OFFSET(encryption), AV_OPT_TYPE_INT ,{.i64=0}, 0, INT_MAX, .flags = D|E }, > + { NULL } > +}; > + > +static int risterr2ret(int err) > +{ > + switch (err) { > + case RIST_ERR_MALLOC: > + return AVERROR(ENOMEM); > + default: > + return AVERROR_EXTERNAL; > + } > +} > + > +static int log_cb(void *arg, enum rist_log_level log_level, const char *msg) > +{ > + int level; > + > + switch (log_level) { > + case RIST_LOG_ERROR: level = AV_LOG_ERROR; break; > + case RIST_LOG_WARN: level = AV_LOG_WARNING; break; > + case RIST_LOG_NOTICE: level = AV_LOG_VERBOSE; break; > + case RIST_LOG_INFO: level = AV_LOG_INFO; break; > + case RIST_LOG_DEBUG: level = AV_LOG_DEBUG; break; > + case RIST_LOG_DISABLE: level = AV_LOG_QUIET; break; > + case RIST_LOG_SIMULATE: level = AV_LOG_TRACE; break; > + default: level = AV_LOG_PANIC; > + } > + > + av_log(arg, level, "%s", msg); > + > + return 0; > +} > + > +static int librist_close(URLContext *h) > +{ > + RISTContext *s = h->priv_data; > + int ret = 0; > + > + s->peer = NULL; > + > + if (s->rist_ctx) > + ret = rist_destroy(s->rist_ctx); > + if (ret < 0) > + return risterr2ret(ret); > + s->rist_ctx = NULL; > + > + if (s->peer_config) > + free((void *)s->peer_config); > + s->peer_config = NULL; > + > + if (s->logging_settings) > + free((void *)s->logging_settings); Still need to get rid of these casts. > + s->logging_settings = NULL; > + > + return 0; > +} > + > +static int librist_open(URLContext *h, const char *uri, int flags) > +{ > + RISTContext *s = h->priv_data; > + int ret; > + > + ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr); > + if (ret < 0) > + return risterr2ret(ret); > + > + if (flags & AVIO_FLAG_WRITE) { > + ret = rist_sender_create(&s->rist_ctx, s->profile, 0, s->logging_settings); > + } else { > + ret = rist_receiver_create(&s->rist_ctx, s->profile, s->logging_settings); > + } > + if (ret < 0) > + goto err; > + > + ret = rist_parse_address(uri, (const struct rist_peer_config **)&s->peer_config); > + if (ret < 0) > + goto err; > + > + if (((s->encryption == 128 || s->encryption == 256) && !s->secret) || > + ((s->peer_config->key_size == 128 || s->peer_config->key_size == 256) && !s->peer_config->secret[0])) { > + av_log(h, AV_LOG_ERROR, "secret is mandatory if encryption is enabled\n"); > + ret = AVERROR(EINVAL); > + goto err; ret will be passed to risterr2ret(). > + } > + > + if (s->secret && s->peer_config->secret[0] == 0) > + av_strlcpy(s->peer_config->secret, s->secret, FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret))); > + > + if (s->secret && (s->encryption == 128 || s->encryption == 256)) > + s->peer_config->key_size = s->encryption; > + > + if (s->buffer_size) { > + s->peer_config->recovery_length_min = s->buffer_size; > + s->peer_config->recovery_length_max = s->buffer_size; > + } > + > + ret = rist_peer_create(s->rist_ctx, &s->peer, s->peer_config); > + if (ret < 0) > + goto err; > + > + ret = rist_start(s->rist_ctx); > + if (ret < 0) > + goto err; > + > + h->max_packet_size = s->packet_size; > + > + return 0; > + > +err: > + librist_close(h); > + > + return risterr2ret(ret); > +} > + > +static int librist_read(URLContext *h, uint8_t *buf, int size) > +{ > + RISTContext *s = h->priv_data; > + const struct rist_data_block *data_block; > + int ret; > + > + ret = rist_receiver_data_read(s->rist_ctx, &data_block, 5); Already mentioned: the 5 millisecond timeout not acceptable. Set it to 0 in non-blocking mode and as large as possible in blocking mode. > + if (ret < 0) > + return risterr2ret(ret); > + > + if (ret > 0) { > + size = FFMIN(size, data_block->payload_len); > + memcpy(buf, data_block->payload, size); > + return size; It looks like part of the payload can be silently discarded. It cannot be allowed. > + } > + > + return AVERROR(EAGAIN); > +} > + > +static int librist_write(URLContext *h, const uint8_t *buf, int size) > +{ > + RISTContext *s = h->priv_data; > + struct rist_data_block data_block = { 0 }; > + int ret; > + > + data_block.ts_ntp = 0; > + data_block.payload = buf; > + data_block.payload_len = size; > + ret = rist_sender_data_write(s->rist_ctx, &data_block); > + if (ret < 0) > + return risterr2ret(ret); > + else if (ret) > + return ret; > + else // remove this part when new librist release appears > + return size; > +} > + > +static const AVClass librist_class = { > + .class_name = "librist", > + .item_name = av_default_item_name, > + .option = librist_options, > + .version = LIBAVUTIL_VERSION_INT, > +}; > + > +const URLProtocol ff_librist_protocol = { > + .name = "rist", > + .url_open = librist_open, > + .url_read = librist_read, > + .url_write = librist_write, > + .url_close = librist_close, > + .priv_data_size = sizeof(RISTContext), > + .flags = URL_PROTOCOL_FLAG_NETWORK, > + .priv_data_class = &librist_class, > +}; > diff --git a/libavformat/protocols.c b/libavformat/protocols.c > index 7df18fbb3b..c4fc446bb6 100644 > --- a/libavformat/protocols.c > +++ b/libavformat/protocols.c > @@ -61,6 +61,7 @@ extern const URLProtocol ff_udp_protocol; > extern const URLProtocol ff_udplite_protocol; > extern const URLProtocol ff_unix_protocol; > extern const URLProtocol ff_libamqp_protocol; > +extern const URLProtocol ff_librist_protocol; > extern const URLProtocol ff_librtmp_protocol; > extern const URLProtocol ff_librtmpe_protocol; > extern const URLProtocol ff_librtmps_protocol; Regards,
On Thu, 24 Dec 2020, Nicolas George wrote: >> +static int librist_read(URLContext *h, uint8_t *buf, int size) >> +{ >> + RISTContext *s = h->priv_data; >> + const struct rist_data_block *data_block; >> + int ret; >> + > >> + ret = rist_receiver_data_read(s->rist_ctx, &data_block, 5); > > Already mentioned: the 5 millisecond timeout not acceptable. Set it to 0 > in non-blocking mode and as large as possible in blocking mode. Actually it should be POLLING_TIME as defined in libavformat/network.h for blocking mode if you want behaviour to be consistent with other protocols. The function cannot block indefinitely even in blocking mode to be able allow generic code in libavformat/avio.c:retry_transfer_wrapper to abort a blocking wait. Other protocols are directly using poll/select to wait for data with POLLING_TIME timeout, but since librist does not provide a way to poll for data, this is the best we can do (and return EAGAIN even in blocking mode if the POLLING_TIME elapses, because retry_transfer_wrapper will retry in that case anyway) Regards, Marton
Marton Balint (12020-12-25): > Actually it should be POLLING_TIME as defined in libavformat/network.h for > blocking mode if you want behaviour to be consistent with other protocols. > The function cannot block indefinitely even in blocking mode to be able > allow generic code in libavformat/avio.c:retry_transfer_wrapper to abort a > blocking wait. You are right, I forgot about it. It should be 0 in non-blocking mode and POLLING_TIME in blocking mode. Definitely not 5. > Other protocols are directly using poll/select to wait for data with > POLLING_TIME timeout, but since librist does not provide a way to poll for Note that it is far from a perfect solution. In particular, it makes our network protocols unsuitable for idle connections with embedded applications, because these timeouts prevent the device from sleeping. At some point, we will need to redesign our protocols API around an event loop. In fact, all our I/O and threading probably need to be merged into a single event loop and scheduler. > data, this is the best we can do (and return EAGAIN even in blocking mode if > the POLLING_TIME elapses, because retry_transfer_wrapper will retry in that > case anyway) It looks to me that librist is not ready for prime time. We could integrate it as experimental, to help it getting there. But I do not know if the people involved would be receptive. Regards,
Hello Nicolas, We already had a file descriptor signaling method on a private branch. Here is the link: https://code.videolan.org/rist/librist/-/commits/descriptor_method/ I assume this would work? Is the idea to add our file handle to a larger array on a master select loop inside ffmpeg? After the select is triggered, is it a requirement that the data also be read with a standard "read" call on the file descriptor or is calling an API acceptable? In the solution above, we use an API to retrieve the data. Sending the data within the file descriptor context would require serialization of the rist_data_block structure and having it deserialized by the calling application. Regards, Sergio ---------- Forwarded message --------- Date: Mon, 28 Dec 2020, 12:24 Subject: Re: [FFmpeg-devel] [PATCH] libavformat: add librist protocol To: FFmpeg development discussions and patches < ffmpeg-devel@ffmpeg.org> Marton Balint (12020-12-25): > Actually it should be POLLING_TIME as defined in libavformat/network.h for > blocking mode if you want behaviour to be consistent with other protocols. > The function cannot block indefinitely even in blocking mode to be able > allow generic code in libavformat/avio.c:retry_transfer_wrapper to abort a > blocking wait. You are right, I forgot about it. It should be 0 in non- blocking mode and POLLING_TIME in blocking mode. Definitely not 5. > Other protocols are directly using poll/select to wait for data with > POLLING_TIME timeout, but since librist does not provide a way to poll for Note that it is far from a perfect solution. In particular, it makes our network protocols unsuitable for idle connections with embedded applications, because these timeouts prevent the device from sleeping. At some point, we will need to redesign our protocols API around an event loop. In fact, all our I/O and threading probably need to be merged into a single event loop and scheduler. > data, this is the best we can do (and return EAGAIN even in blocking mode if > the POLLING_TIME elapses, because retry_transfer_wrapper will retry in that > case anyway) It looks to me that librist is not ready for prime time. We could integrate it as experimental, to help it getting there. But I do not know if the people involved would be receptive. Regards,
On Mon, 28 Dec 2020, Sergio M. Ammirata, Ph.D. wrote: > Hello Nicolas, > > We already had a file descriptor signaling method on a > private branch. Here is the link: > > > https://code.videolan.org/rist/librist/-/commits/descriptor_method/ > > I assume this would work? > > Is the idea to add our file handle to a larger array on a > master select loop inside ffmpeg? Just to make it clear, this is only a plan for the replacement of the current polling based interrupt logic, and it probably won't happen anytime soon. > After the select is triggered, is it a requirement that the > data also be read with a standard "read" call on the file > descriptor or is calling an API acceptable? In the solution > above, we use an API to retrieve the data. > Sending the data within the file descriptor context would > require serialization of the rist_data_block structure and > having it deserialized by the calling application. I'd say it is too early to say which method will fit ffmpeg's approach best, on the other hand ffmpeg will have to support many protocols with different polling capabilities, so I'd expect ffmpeg should be able to adapt to both. And maybe my comment made the impression that fd based polling is needed for proper ffmpeg support, but as far as I see it that is not really the case, at least not for the current ffmpeg code. We are just used to having a poll() call and a read() call, because the read() call does not usually provide timeout capability, so we always had to use poll() first. But in librist the read() call already have a timeout parameter, so we can simply use that instead of poll()+read(). Regards, Marton > > Regards, > > Sergio > > > ---------- Forwarded message --------- > Date: Mon, 28 Dec 2020, 12:24 > Subject: Re: [FFmpeg-devel] [PATCH] libavformat: add > librist protocol > To: FFmpeg development discussions and patches < > ffmpeg-devel@ffmpeg.org> > > > Marton Balint (12020-12-25): > >> Actually it should be POLLING_TIME as defined in > libavformat/network.h for > >> blocking mode if you want behaviour to be consistent with > other protocols. > >> The function cannot block indefinitely even in blocking > mode to be able > >> allow generic code in > libavformat/avio.c:retry_transfer_wrapper to abort a > >> blocking wait. > > > > You are right, I forgot about it. It should be 0 in non- > blocking mode > > and POLLING_TIME in blocking mode. Definitely not 5. > > > >> Other protocols are directly using poll/select to wait > for data with > >> POLLING_TIME timeout, but since librist does not provide > a way to poll for > > > > Note that it is far from a perfect solution. In particular, > it makes our > > network protocols unsuitable for idle connections with > embedded > > applications, because these timeouts prevent the device > from sleeping. > > At some point, we will need to redesign our protocols API > around an > > event loop. In fact, all our I/O and threading probably > need to be > > merged into a single event loop and scheduler. > > > >> data, this is the best we can do (and return EAGAIN even > in blocking mode if > >> the POLLING_TIME elapses, because retry_transfer_wrapper > will retry in that > >> case anyway) > > > > It looks to me that librist is not ready for prime time. We > could > > integrate it as experimental, to help it getting there. But > I do not > > know if the people involved would be receptive. > > > > Regards, > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Sergio M. Ammirata, Ph.D. (12020-12-28): > We already had a file descriptor signaling method on a > private branch. Here is the link: > > > https://code.videolan.org/rist/librist/-/commits/descriptor_method/ > > I assume this would work? > > Is the idea to add our file handle to a larger array on a > master select loop inside ffmpeg? > After the select is triggered, is it a requirement that the > data also be read with a standard "read" call on the file > descriptor or is calling an API acceptable? In the solution > above, we use an API to retrieve the data. IIUC, it adds a function where the application gives one end of a self-pipe (https://cr.yp.to/docs/selfpipe.html) to get notification when something is ready. It should work nicely. Draining the pipe explicitly or through an API call, both are fine, as long as the proper way is documented. You could also just give access to the actual socket file descriptor. It is what Xlib does, for example (https://linux.die.net/man/3/connectionnumber), it gives the socket to the X11 server to the application, the application poll()s it, and when it is readable, it can call XPending() / XNextEvent(). > Sending the data within the file descriptor context would > require serialization of the rist_data_block structure and > having it deserialized by the calling application. Of course, no need for that. As Marton said, FFmpeg is currently underdeveloped in the async protocols department. I want to change that, but that is not for today. But FFmpeg is not the only library around here. To make sure this new library's API is really usable by complex application, I think good advice would be to make sure it works with a few common event loops. I think probably GLib's main event loop (https://developer.gnome.org/glib/2.66/glib-The-Main-Event-Loop.html), because it is at the core of most GNOME applications. And possibly libev (http://software.schmorp.de/pkg/libev.html), because it is lightweight and simple yet efficient. Maybe the best idea would be to have doc/examples/gtk.c and doc/examples/libev.c to show how to properly use the API in an asynchronous application. Anyway, thanks for reacting so efficiently. <rant> What people do not realize is that threads do not make network programming simpler. We often see advice, about network programming, "just use threads", but it is a misconception. Threads are very useful for performance, but when it comes to network programming, they do not help (except for DNS). I must be more accurate: POSIX threads do not help with Unix-style sockets. The issue is that POSIX threads and Unix sockets are completely separate APIs, without any kind of integration. When a thread is blocked on a socket operation, it does not communicate with other threads. To implement things as basic as a timeout with threads, it requires at least another thread, and some kind of asynchronous cancellation operation, which is tricky, both for portability and for concurrency problems. Really, a loop around poll() or equivalent is the only working way to use Unix sockets. When we try to use threads with sockets, we end up having a poll() loop in each thread, and then we might as well have a single loop without threads. Also, event loops scale much better than threads. I write this under the assumption we want to do things properly, i.e. have an application that can sleep when there is nothing on the network. So many applications have a periodic wake-up just to check for timeouts and other interrupts; FFmpeg have one ten times per second. For embedded applications, it is terrible, because it prevents the device from going to deep sleep, and drains the battery. </rant> Regards,
Understood ... thanks for the advice. Is there else blocking the integration of the library based module into ffmpeg? We would prefer not to have it in "experimental mode". Regards, Sergio On Tue, 2020-12-29 at 23:03 +0100, Nicolas George wrote: > Sergio M. Ammirata, Ph.D. (12020-12-28): > > We already had a file descriptor signaling method on a > private branch. Here is the link: > > > https://code.videolan.org/rist/librist/-/commits/descriptor_method/ > > I assume this would work? > > Is the idea to add our file handle to a larger array on a > master select loop inside ffmpeg? > After the select is triggered, is it a requirement that the > data also be read with a standard "read" call on the file > descriptor or is calling an API acceptable? In the solution > above, we use an API to retrieve the data. > > IIUC, it adds a function where the application gives one end of a > self-pipe (https://cr.yp.to/docs/selfpipe.html) to get notification when > something is ready. It should work nicely. Draining the pipe explicitly > or through an API call, both are fine, as long as the proper way is > documented. > > You could also just give access to the actual socket file descriptor. It > is what Xlib does, for example > (https://linux.die.net/man/3/connectionnumber), it gives the socket to > the X11 server to the application, the application poll()s it, and when > it is readable, it can call XPending() / XNextEvent(). > > > Sending the data within the file descriptor context would > require serialization of the rist_data_block structure and > having it deserialized by the calling application. > > Of course, no need for that. > > As Marton said, FFmpeg is currently underdeveloped in the async > protocols department. I want to change that, but that is not for today. > > But FFmpeg is not the only library around here. To make sure this new > library's API is really usable by complex application, I think good > advice would be to make sure it works with a few common event loops. I > think probably GLib's main event loop > (https://developer.gnome.org/glib/2.66/glib-The-Main-Event-Loop.html), > because it is at the core of most GNOME applications. And possibly libev > (http://software.schmorp.de/pkg/libev.html), because it is lightweight > and simple yet efficient. > > Maybe the best idea would be to have doc/examples/gtk.c and > doc/examples/libev.c to show how to properly use the API in an > asynchronous application. > > Anyway, thanks for reacting so efficiently. > > <rant> > > What people do not realize is that threads do not make network > programming simpler. We often see advice, about network programming, > "just use threads", but it is a misconception. > > Threads are very useful for performance, but when it comes to network > programming, they do not help (except for DNS). I must be more accurate: > POSIX threads do not help with Unix-style sockets. > > The issue is that POSIX threads and Unix sockets are completely separate > APIs, without any kind of integration. When a thread is blocked on a > socket operation, it does not communicate with other threads. To > implement things as basic as a timeout with threads, it requires at > least another thread, and some kind of asynchronous cancellation > operation, which is tricky, both for portability and for concurrency > problems. > > Really, a loop around poll() or equivalent is the only working way to > use Unix sockets. When we try to use threads with sockets, we end up > having a poll() loop in each thread, and then we might as well have a > single loop without threads. Also, event loops scale much better than > threads. > > I write this under the assumption we want to do things properly, i.e. > have an application that can sleep when there is nothing on the network. > So many applications have a periodic wake-up just to check for timeouts > and other interrupts; FFmpeg have one ten times per second. For embedded > applications, it is terrible, because it prevents the device from going > to deep sleep, and drains the battery. > > </rant> > > Regards, > >
Sergio M. Ammirata, Ph.D. (12020-12-30): > Is there else blocking the integration of the library based > module into ffmpeg? The comments made by James and me need to be addressed. > We would prefer not to have it in "experimental mode". For that, you need to be really really sure that the API is stable. In particular, you will not be able to change the layout of the various _config structs. As a more concrete example, this: commit 29641ac74f89d9a8e6a187a9b218678f1ada27ab Author: Sergio Ammirata <sergio@ammirata.net> Date: 2020-12-25 14:15:22 -0500 Change the new free functions to accept pointer of pointer so they can be nulled cannot happen if librist is used by something serious. I would recommend not to rush things. Designing a good library API takes time. If you freeze it too early, you will probably get stuck with things you will regret later. Having it as "experimental" in FFmpeg is just a way of having more time to make things really good before freezing. Regards,
diff --git a/configure b/configure index 90914752f1..462f2dcf64 100755 --- a/configure +++ b/configure @@ -259,6 +259,7 @@ External library support: --enable-libpulse enable Pulseaudio input via libpulse [no] --enable-librabbitmq enable RabbitMQ library [no] --enable-librav1e enable AV1 encoding via rav1e [no] + --enable-librist enable RIST via librist [no] --enable-librsvg enable SVG rasterization via librsvg [no] --enable-librubberband enable rubberband needed for rubberband filter [no] --enable-librtmp enable RTMP[E] support via librtmp [no] @@ -1797,6 +1798,7 @@ EXTERNAL_LIBRARY_LIST=" libpulse librabbitmq librav1e + librist librsvg librtmp libshine @@ -3488,6 +3490,8 @@ unix_protocol_select="network" # external library protocols libamqp_protocol_deps="librabbitmq" libamqp_protocol_select="network" +librist_protocol_deps="librist" +librist_protocol_select="network" librtmp_protocol_deps="librtmp" librtmpe_protocol_deps="librtmp" librtmps_protocol_deps="librtmp" @@ -6404,6 +6408,7 @@ enabled libopus && { enabled libpulse && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new enabled librabbitmq && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection enabled librav1e && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new +enabled librist && require_pkg_config librist "librist >= 0.2" librist/librist.h rist_receiver_create enabled librsvg && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo enabled librtmp && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket enabled librubberband && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++" diff --git a/doc/protocols.texi b/doc/protocols.texi index de377a9546..aa682f1586 100644 --- a/doc/protocols.texi +++ b/doc/protocols.texi @@ -673,6 +673,38 @@ Example usage: -f rtp_mpegts -fec prompeg=l=8:d=4 rtp://@var{hostname}:@var{port} @end example +@section rist + +Reliable Internet Streaming Transport protocol + +The accepted options are: +@table @option +@item rist_profile +Supported values: +@table @samp +@item simple +@item main +This one is default. +@item advanced +@end table + +@item buffer_size +Set internal RIST buffer size for retransmission of data. + +@item pkt_size +Set internal RIST buffer size for receiving and sending data. + +@item log_level +Set loglevel for RIST logging messages. + +@item secret +Set override of encryption secret, by default is unset. + +@item encryption +Set encryption type, by default is disabled. +Acceptable values are 128 and 256. +@end table + @section rtmp Real-Time Messaging Protocol. diff --git a/libavformat/Makefile b/libavformat/Makefile index 97d868081b..799e16c59e 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -652,6 +652,7 @@ OBJS-$(CONFIG_UNIX_PROTOCOL) += unix.o # external library protocols OBJS-$(CONFIG_LIBAMQP_PROTOCOL) += libamqp.o +OBJS-$(CONFIG_LIBRIST_PROTOCOL) += librist.o OBJS-$(CONFIG_LIBRTMP_PROTOCOL) += librtmp.o OBJS-$(CONFIG_LIBRTMPE_PROTOCOL) += librtmp.o OBJS-$(CONFIG_LIBRTMPS_PROTOCOL) += librtmp.o diff --git a/libavformat/librist.c b/libavformat/librist.c new file mode 100644 index 0000000000..dacdd4a859 --- /dev/null +++ b/libavformat/librist.c @@ -0,0 +1,233 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * @file + * Reliable Internet Streaming Transport protocol + */ + +#include "libavutil/avassert.h" +#include "libavutil/opt.h" +#include "libavutil/parseutils.h" +#include "libavutil/time.h" + +#include "avformat.h" +#include "internal.h" +#include "network.h" +#include "os_support.h" +#include "url.h" + +#include <librist/librist.h> + +typedef struct RISTContext { + const AVClass *class; + + int profile; + int buffer_size; + int packet_size; + int log_level; + int encryption; + char *secret; + + struct rist_logging_settings *logging_settings; + struct rist_peer_config *peer_config; + struct rist_peer *peer; + struct rist_ctx *rist_ctx; +} RISTContext; + +#define D AV_OPT_FLAG_DECODING_PARAM +#define E AV_OPT_FLAG_ENCODING_PARAM +#define OFFSET(x) offsetof(RISTContext, x) +static const AVOption librist_options[] = { + { "rist_profile","set profile", OFFSET(profile), AV_OPT_TYPE_INT, {.i64=RIST_PROFILE_MAIN}, 0, 2, .flags = D|E, "profile" }, + { "simple", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_SIMPLE}, 0, 0, .flags = D|E, "profile" }, + { "main", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_MAIN}, 0, 0, .flags = D|E, "profile" }, + { "advanced", NULL, 0, AV_OPT_TYPE_CONST, {.i64=RIST_PROFILE_ADVANCED}, 0, 0, .flags = D|E, "profile" }, + { "buffer_size", "set buffer_size", OFFSET(buffer_size), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX, .flags = D|E }, + { "pkt_size", "set packet size", OFFSET(packet_size), AV_OPT_TYPE_INT, {.i64=9968}, 1, 9968, .flags = D|E }, + { "log_level", "set loglevel", OFFSET(log_level), AV_OPT_TYPE_INT, {.i64=-1}, -1, INT_MAX, .flags = D|E }, + { "secret", "set encryption secret",OFFSET(secret), AV_OPT_TYPE_STRING,{.str=NULL}, 0, 0, .flags = D|E }, + { "encryption","set encryption type",OFFSET(encryption), AV_OPT_TYPE_INT ,{.i64=0}, 0, INT_MAX, .flags = D|E }, + { NULL } +}; + +static int risterr2ret(int err) +{ + switch (err) { + case RIST_ERR_MALLOC: + return AVERROR(ENOMEM); + default: + return AVERROR_EXTERNAL; + } +} + +static int log_cb(void *arg, enum rist_log_level log_level, const char *msg) +{ + int level; + + switch (log_level) { + case RIST_LOG_ERROR: level = AV_LOG_ERROR; break; + case RIST_LOG_WARN: level = AV_LOG_WARNING; break; + case RIST_LOG_NOTICE: level = AV_LOG_VERBOSE; break; + case RIST_LOG_INFO: level = AV_LOG_INFO; break; + case RIST_LOG_DEBUG: level = AV_LOG_DEBUG; break; + case RIST_LOG_DISABLE: level = AV_LOG_QUIET; break; + case RIST_LOG_SIMULATE: level = AV_LOG_TRACE; break; + default: level = AV_LOG_PANIC; + } + + av_log(arg, level, "%s", msg); + + return 0; +} + +static int librist_close(URLContext *h) +{ + RISTContext *s = h->priv_data; + int ret = 0; + + s->peer = NULL; + + if (s->rist_ctx) + ret = rist_destroy(s->rist_ctx); + if (ret < 0) + return risterr2ret(ret); + s->rist_ctx = NULL; + + if (s->peer_config) + free((void *)s->peer_config); + s->peer_config = NULL; + + if (s->logging_settings) + free((void *)s->logging_settings); + s->logging_settings = NULL; + + return 0; +} + +static int librist_open(URLContext *h, const char *uri, int flags) +{ + RISTContext *s = h->priv_data; + int ret; + + ret = rist_logging_set(&s->logging_settings, s->log_level, log_cb, h, NULL, stderr); + if (ret < 0) + return risterr2ret(ret); + + if (flags & AVIO_FLAG_WRITE) { + ret = rist_sender_create(&s->rist_ctx, s->profile, 0, s->logging_settings); + } else { + ret = rist_receiver_create(&s->rist_ctx, s->profile, s->logging_settings); + } + if (ret < 0) + goto err; + + ret = rist_parse_address(uri, (const struct rist_peer_config **)&s->peer_config); + if (ret < 0) + goto err; + + if (((s->encryption == 128 || s->encryption == 256) && !s->secret) || + ((s->peer_config->key_size == 128 || s->peer_config->key_size == 256) && !s->peer_config->secret[0])) { + av_log(h, AV_LOG_ERROR, "secret is mandatory if encryption is enabled\n"); + ret = AVERROR(EINVAL); + goto err; + } + + if (s->secret && s->peer_config->secret[0] == 0) + av_strlcpy(s->peer_config->secret, s->secret, FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret))); + + if (s->secret && (s->encryption == 128 || s->encryption == 256)) + s->peer_config->key_size = s->encryption; + + if (s->buffer_size) { + s->peer_config->recovery_length_min = s->buffer_size; + s->peer_config->recovery_length_max = s->buffer_size; + } + + ret = rist_peer_create(s->rist_ctx, &s->peer, s->peer_config); + if (ret < 0) + goto err; + + ret = rist_start(s->rist_ctx); + if (ret < 0) + goto err; + + h->max_packet_size = s->packet_size; + + return 0; + +err: + librist_close(h); + + return risterr2ret(ret); +} + +static int librist_read(URLContext *h, uint8_t *buf, int size) +{ + RISTContext *s = h->priv_data; + const struct rist_data_block *data_block; + int ret; + + ret = rist_receiver_data_read(s->rist_ctx, &data_block, 5); + if (ret < 0) + return risterr2ret(ret); + + if (ret > 0) { + size = FFMIN(size, data_block->payload_len); + memcpy(buf, data_block->payload, size); + return size; + } + + return AVERROR(EAGAIN); +} + +static int librist_write(URLContext *h, const uint8_t *buf, int size) +{ + RISTContext *s = h->priv_data; + struct rist_data_block data_block = { 0 }; + int ret; + + data_block.ts_ntp = 0; + data_block.payload = buf; + data_block.payload_len = size; + ret = rist_sender_data_write(s->rist_ctx, &data_block); + if (ret < 0) + return risterr2ret(ret); + else if (ret) + return ret; + else // remove this part when new librist release appears + return size; +} + +static const AVClass librist_class = { + .class_name = "librist", + .item_name = av_default_item_name, + .option = librist_options, + .version = LIBAVUTIL_VERSION_INT, +}; + +const URLProtocol ff_librist_protocol = { + .name = "rist", + .url_open = librist_open, + .url_read = librist_read, + .url_write = librist_write, + .url_close = librist_close, + .priv_data_size = sizeof(RISTContext), + .flags = URL_PROTOCOL_FLAG_NETWORK, + .priv_data_class = &librist_class, +}; diff --git a/libavformat/protocols.c b/libavformat/protocols.c index 7df18fbb3b..c4fc446bb6 100644 --- a/libavformat/protocols.c +++ b/libavformat/protocols.c @@ -61,6 +61,7 @@ extern const URLProtocol ff_udp_protocol; extern const URLProtocol ff_udplite_protocol; extern const URLProtocol ff_unix_protocol; extern const URLProtocol ff_libamqp_protocol; +extern const URLProtocol ff_librist_protocol; extern const URLProtocol ff_librtmp_protocol; extern const URLProtocol ff_librtmpe_protocol; extern const URLProtocol ff_librtmps_protocol;
This work is sponsored by Open Broadcast Systems. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- configure | 5 + doc/protocols.texi | 32 ++++++ libavformat/Makefile | 1 + libavformat/librist.c | 233 ++++++++++++++++++++++++++++++++++++++++ libavformat/protocols.c | 1 + 5 files changed, 272 insertions(+) create mode 100644 libavformat/librist.c