diff mbox series

[FFmpeg-devel] avformat/libsrt: Change latency option to milliseconds

Message ID 47b48107-4679-ca98-5b8c-d6552ff5f0e0@obsproject.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/libsrt: Change latency option to milliseconds | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

pkv April 7, 2020, 4:21 p.m. UTC
Hi,

The latency option (and related options) are currently in microseconds 
in FFmpeg (avformat/libsrt.c).
The libsrt API documents them in milliseconds while FFmpeg doc does not 
specify any unit.
This patch changes these options to milliseconds and updates the FFmpeg doc.

The microsecond implicit unit has caused confusion for both FFmpeg and 
obs-studio users:
- https://github.com/Haivision/srt/issues/1223
- https://obsproject.com/mantis/view.php?id=1617

Regards

pkv
From 02a3000b91b81aa0c68ea1c01c2f88c8a4f3c754 Mon Sep 17 00:00:00 2001
From: pkv <pkv@obsproject.com>
Date: Tue, 7 Apr 2020 18:08:22 +0200
Subject: [PATCH] avformat/libsrt: Change latency option to milliseconds

The latency option (and related options) are currently in microseconds.
The libsrt API documents them in milliseconds while FFmpeg doc does not
specify anything.
This patch changes these options to milliseconds and updates the FFmpeg doc.
The microsecond unit has caused confusion for both FFmpeg and obs-studio users:
- https://github.com/Haivision/srt/issues/1223
- https://obsproject.com/mantis/view.php?id=1617
---
 doc/protocols.texi   | 2 +-
 libavformat/libsrt.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Nicolas George April 7, 2020, 4:42 p.m. UTC | #1
pkv (12020-04-07):
> The latency option (and related options) are currently in microseconds in
> FFmpeg (avformat/libsrt.c).
> The libsrt API documents them in milliseconds while FFmpeg doc does not
> specify any unit.
> This patch changes these options to milliseconds and updates the FFmpeg doc.
> 
> The microsecond implicit unit has caused confusion for both FFmpeg and
> obs-studio users:
> - https://github.com/Haivision/srt/issues/1223
> - https://obsproject.com/mantis/view.php?id=1617

I think it should stay in microseconds but be declared as a DURATION
option.

Regards,
pkv April 7, 2020, 5:13 p.m. UTC | #2
Le 07/04/2020 à 6:42 pm, Nicolas George a écrit :
> pkv (12020-04-07):
>> The latency option (and related options) are currently in microseconds in
>> FFmpeg (avformat/libsrt.c).
>> The libsrt API documents them in milliseconds while FFmpeg doc does not
>> specify any unit.
>> This patch changes these options to milliseconds and updates the FFmpeg doc.
>>
>> The microsecond implicit unit has caused confusion for both FFmpeg and
>> obs-studio users:
>> - https://github.com/Haivision/srt/issues/1223
>> - https://obsproject.com/mantis/view.php?id=1617
> I think it should stay in microseconds but be declared as a DURATION
> option.
>
> 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".

Hello,

you mean AV_OPT_TYPE_DURATION ?

what's the reason for keeping these options in microseconds ?

(to the extent the unit is documented it does not really matter, in 
truth; it's just more convenient in ms).

Best
Nicolas George April 7, 2020, 5:50 p.m. UTC | #3
pkv (12020-04-07):
> you mean AV_OPT_TYPE_DURATION ?

Exactly.

> what's the reason for keeping these options in microseconds ?
> 
> (to the extent the unit is documented it does not really matter, in truth;
> it's just more convenient in ms).

Some timings do require a better accuracy, more will do as computers get
more powerful, and consistency is convenient.

In this instance, AV_OPT_TYPE_DURATION is undoubtedly the proper type
for this option, and it is defined in microseconds.

Regards,
Nicolas George April 8, 2020, 9:48 a.m. UTC | #4
Maxim Sharabayko (12020-04-08):
> You say the ffmpeg's SRT latency option should stay in microseconds.
> A motivation for this is to avoid confusion and mismatch between ffmpeg versions, isn't it?
> If so, then what if an additional option "latencyms" would be introduced, and "latency" option becomes deprecated?

You would be right if we wanted to switch to milliseconds. But it is a
duration, and as such it should be AV_OPT_TYPE_DURATION and in
microseconds.

> SRT users got used to having the "latency" option in milliseconds in
> every application, that is why this ffmpeg behavior is a bit
> confusing.

We are thinking of FFmpeg users first here.

> On 07.04.20, 18:43, "Nicolas George" <george@nsup.org> wrote:

Remember that top-posting is forbidden on this mailing-list. If you
don't know what it means, look it up.

Regards,
diff mbox series

Patch

diff --git a/doc/protocols.texi b/doc/protocols.texi
index e510019f2d..b0e018b1ea 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -1287,7 +1287,7 @@  IP Type of Service. Applies to sender only. Default value is 0xB8.
 IP Time To Live. Applies to sender only. Default value is 64.
 
 @item latency
-Timestamp-based Packet Delivery Delay.
+Timestamp-based Packet Delivery Delay in msec.
 Used to absorb bursts of missed packet retransmissions.
 This flag sets both @option{rcvlatency} and @option{peerlatency}
 to the same value. Note that prior to version 1.3.0
diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
index 2d6fc4b7e7..b4a4e94d45 100644
--- a/libavformat/libsrt.c
+++ b/libavformat/libsrt.c
@@ -302,9 +302,9 @@  static int libsrt_set_options_pre(URLContext *h, int fd)
 {
     SRTContext *s = h->priv_data;
     int yes = 1;
-    int latency = s->latency / 1000;
-    int rcvlatency = s->rcvlatency / 1000;
-    int peerlatency = s->peerlatency / 1000;
+    int64_t latency = s->latency;
+    int64_t rcvlatency = s->rcvlatency;
+    int64_t peerlatency = s->peerlatency;
     int connect_timeout = s->connect_timeout;
 
     if ((s->mode == SRT_MODE_RENDEZVOUS && libsrt_setsockopt(h, fd, SRTO_RENDEZVOUS, "SRTO_RENDEZVOUS", &yes, sizeof(yes)) < 0) ||