[FFmpeg-devel] [PATCH v4 1/1] avformat/mpegts: Add duration_probesize AVOption

[Stefano Sabatini]

On date Tuesday 2024-03-26 14:18:31 +0100, Nicolas Gaullier wrote:
> Yet another probesize option aimed at users interested
> in better durations probing for itself, or because using
> avformat_find_stream_info indirectly and requiring exact values: for
> concatdec for example, especially if streamcopying above it.
> The current code is a performance trade-off that can fail to get video
> stream durations in a scenario with high bitrates and buffering for
> files ending cleanly (as opposed to live captures): the physical gap
> between the last video packet and the last audio packet is very high in
> such a case.
> 
> Default behaviour is unchanged: 250k up to 250k << 6 (step by step).
> Setting this new option has two effects:
> - override the maximum probesize (currently 250k << 6)
> - reduce the number of steps to 1 instead of 6, this is to avoid
> detecting the audio "too early" and failing to reach a video packet.
> Even if a single audio stream duration is found but not the other
> audio/video stream durations, there will be a retry, so at the end the
> full user-overriden probesize will be used as expected by the user.
> 
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier at cji.paris>
> ---
>  doc/demuxers.texi     |  13 +++++
>  doc/formats.texi      |   2 +-
>  libavformat/demux.c   |  23 ++++++---
>  libavformat/mpegts.c  | 104 +---------------------------------------
>  libavformat/mpegts.h  | 108 +++++++++++++++++++++++++++++++++++++++++-
>  libavformat/version.h |   2 +-
>  6 files changed, 140 insertions(+), 112 deletions(-)
> 
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index b70f3a38d7..f88ae18a6e 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -992,6 +992,19 @@ streams move to different PIDs. Default value is 0.
>  @item max_packet_size
>  Set maximum size, in bytes, of packet emitted by the demuxer. Payloads above this size
>  are split across multiple packets. Range is 1 to INT_MAX/2. Default is 204800 bytes.
> +
> + at item duration_probesize
> +Set probing size, in bytes, for input duration estimation which requires a specific probing
> +for PTS at end of file.
> +It is aimed at users interested in better durations probing for itself, or indirectly
> +for specific use cases like using the concat demuxer.
> +Files with high bitrates and ending cleanly (as opposed to live captures), can lead
> +to a large physical gap between the last video packet and the last audio packet,
> +so many bytes have to be read in order to get a video stream duration.
> +Setting this value has a performance impact even for small files because the probing size is fixed.
> +Default behaviour is a trade-off, largely adaptive: the probing size may range from
> +250000 up to 16M, but it is not extended to get streams durations at all costs.
> +Must be an integer not lesser than 1, or 0 for default behaviour.
>  @end table
>  
>  @section mpjpeg
> diff --git a/doc/formats.texi b/doc/formats.texi
> index 69fc1457a4..e16cd88b2c 100644
> --- a/doc/formats.texi
> +++ b/doc/formats.texi
> @@ -225,7 +225,7 @@ Specifies the maximum number of streams. This can be used to reject files that
>  would require too many resources due to a large number of streams.
>  
>  @item skip_estimate_duration_from_pts @var{bool} (@emph{input})
> -Skip estimation of input duration when calculated using PTS.

> +Skip estimation of input duration if it requires an additional probing for pts at end of file.

nit++: PTS

>  At present, applicable for MPEG-PS and MPEG-TS.
>  
>  @item strict, f_strict @var{integer} (@emph{input/output})
> diff --git a/libavformat/demux.c b/libavformat/demux.c
> index 147f3b93ac..bc23786410 100644
> --- a/libavformat/demux.c
> +++ b/libavformat/demux.c
> @@ -47,6 +47,7 @@
>  #include "id3v2.h"
>  #include "internal.h"
>  #include "url.h"

> +#include "mpegts.h"

this is entangling generic and specific code, and it is something
which should be avoided (ideally the generic code should know nothing
about the specific demuxer)

Rather than this, I think the previous approach is more valid: even if
we have an option that it is only useful for a specific demuxer, its
implementation was still abstract and avoiding any reference to
internals of the mpegts demuxer - and maybe it might turn useful for
other formats - who knows.

What we need in that case is only clarify the use case for which it
was conceived (which might well only apply to a specific demuxer) at
the documentation level. To clarify, I was mostly fine with the
previous patch, but for the nits and for the documentation issue - but
if this turns out that the only use case is about the MPEGTS, probably
we should just mention that (as you was already doing).

[...]

Thanks.