[FFmpeg-devel] [PATCH] libavformat: add librist protocol
Paul B Mahol
onemda at gmail.com
Sun Jan 31 13:06:23 EET 2021
On Sun, Jan 31, 2021 at 12:00 PM Marton Balint <cus at passwd.hu> wrote:
>
>
> On Tue, 26 Jan 2021, Paul B Mahol wrote:
>
> > This work is sponsored by Open Broadcast Systems.
> >
> > Signed-off-by: Paul B Mahol <onemda at gmail.com>
> > ---
> > configure | 5 +
> > doc/protocols.texi | 32 +++++
> > libavformat/Makefile | 1 +
> > libavformat/librist.c | 251 ++++++++++++++++++++++++++++++++++++++++
> > libavformat/protocols.c | 1 +
> > 5 files changed, 290 insertions(+)
> > create mode 100644 libavformat/librist.c
>
>
> [...]
>
>
> > --- a/doc/protocols.texi
> > +++ b/doc/protocols.texi
> > @@ -690,6 +690,38 @@ Example usage:
> > -f rtp_mpegts -fec prompeg=l=8:d=4 rtp://@var{hostname}:@var{port}
> > @end example
> >
> > + at section rist
> > +
> > +Reliable Internet Streaming Transport protocol
> > +
> > +The accepted options are:
> > + at table @option
> > + at item rist_profile
> > +Supported values:
> > + at table @samp
> > + at item simple
> > + at item main
> > +This one is default.
> > + at item advanced
> > + at end table
> > +
> > + at item buffer_size
> > +Set internal RIST buffer size for retransmission of data.
> > +
> > + at item pkt_size
> > +Set internal RIST buffer size for receiving and sending data.
>
> You should mention the default here. And by the way, why have you selected
> 9968 as default? I thought primaryly mpegts is targeted, so maybe 1316
> makes more sense no?
>
>
NO
> What you can also do is to make -1 the default, and then make it 1316 for
> write mode and 9968 otherwise, if really there is a common use for bigger
> packets.
>
>
NO
> [..]
>
> > +static int librist_close(URLContext *h)
> > +{
> > + RISTContext *s = h->priv_data;
> > + int ret = 0;
> > +
> > + s->read_peer = NULL;
> > + s->write_peer = NULL;
> > +
> > + if (s->write_ctx)
> > + ret = rist_destroy(s->read_ctx);
> > + if (s->read_ctx)
> > + ret = rist_destroy(s->read_ctx);
> > + s->read_ctx = NULL;
> > + s->write_ctx = NULL;
> > +
> > + return risterr2ret(ret);
>
> This returns an error even if there is no rist error.
> Maybe you should simply return 0, and avoid assigning rets.
>
> > +}
> > +
> > +static int librist_open(URLContext *h, const char *uri, int flags)
> > +{
> > + RISTContext *s = h->priv_data;
> > + struct rist_logging_settings *logging_settings =
> &s->logging_settings;
> > + struct rist_peer_config *peer_config = &s->peer_config;
> > + int ret;
> > +
> > + ret = rist_logging_set(&logging_settings, s->log_level, log_cb, h,
> NULL, NULL);
> > + if (ret < 0)
> > + return risterr2ret(ret);
> > +
> > + if (flags & AVIO_FLAG_WRITE)
> > + ret = rist_sender_create(&s->write_ctx, s->profile, 0,
> logging_settings);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (flags & AVIO_FLAG_READ)
> > + ret = rist_receiver_create(&s->read_ctx, s->profile,
> logging_settings);
> > + if (ret < 0)
> > + goto err;
>
> You should move these context creating downward to other read/write mode
> checks.
>
NO
>
> > +
> > + ret = rist_peer_config_defaults_set(peer_config);
> > + if (ret < 0)
> > + goto err;
> > +
> > + ret = rist_parse_address(uri, (const struct rist_peer_config
> **)&peer_config);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (((s->encryption == 128 || s->encryption == 256) && !s->secret)
> ||
> > + ((peer_config->key_size == 128 || peer_config->key_size == 256)
> && !peer_config->secret[0])) {
> > + av_log(h, AV_LOG_ERROR, "secret is mandatory if encryption is
> enabled\n");
> > + librist_close(h);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (s->secret && peer_config->secret[0] == 0)
> > + av_strlcpy(peer_config->secret, s->secret,
> FFMIN(RIST_MAX_STRING_SHORT - 1, strlen(s->secret)));
>
> Simply av_strlcpy(peer_config->secret, s->secret, RIST_MAX_STRING_SHORT).
>
Really? Are you sure?
>
> > +
> > + if (s->secret && (s->encryption == 128 || s->encryption == 256))
> > + peer_config->key_size = s->encryption;
> > +
> > + if (s->buffer_size) {
> > + peer_config->recovery_length_min = s->buffer_size;
> > + peer_config->recovery_length_max = s->buffer_size;
> > + }
> > +
> > + if (flags & AVIO_FLAG_READ)
> > + ret = rist_peer_create(s->read_ctx, &s->read_peer,
> &s->peer_config);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (flags & AVIO_FLAG_WRITE)
> > + ret = rist_peer_create(s->write_ctx, &s->write_peer,
> &s->peer_config);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (flags & AVIO_FLAG_READ)
> > + ret = rist_start(s->read_ctx);
> > + if (ret < 0)
> > + goto err;
> > + if (flags & AVIO_FLAG_WRITE)
> > + ret = rist_start(s->write_ctx);
> > + if (ret < 0)
> > + goto err;
>
> Use a single check:
>
> if (flags & AVIO_FLAG_READ) {
> rist_receiver_create()
> rist_peer_create()
> rist_start()
> }
> if (flags & AVIO_FLAG_WRITE) {
> rist_sender_create()
> rist_peer_create()
> rist_start()
> }
>
> Also make sure you are not leaking s->read_ctx/s->write_ctx on errors.
>
> And does it really make sense to use READ and WRITE at the same time? Can
> it work? Nicolas's original suggestion was to simply return error if both
> flags (AVIO_FLAG_READWRITE) are set.
>
I think it can work.
>
> > +
> > + 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->read_ctx, &data_block,
> POLLING_TIME);
> > + if (ret < 0)
> > + return risterr2ret(ret);
> > +
> > + if (ret > 0 && data_block->payload) {
>
> Is it possible that data_block->payload is NULL? If not, then either
> remove the check or make it an assert. If yes, then you are leaking
> data_block if it is indeed NULL.
>
> > + if (data_block->payload_len > size)
> > + av_log(h, AV_LOG_WARNING, "Part of datagram lost due to
> insufficient buffer size\n");
> > + size = FFMIN(size, data_block->payload_len);
> > + memcpy(buf, data_block->payload, size);
> > + rist_receiver_data_block_free(&data_block);
> > + return size;
> > + }
> > +
> > + return AVERROR(EAGAIN);
> > +}
> > +
>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list