[FFmpeg-devel] [PATCH v5 1/1] avformat/demux: Add duration_probesize AVOption

[Stefano Sabatini]

On date Thursday 2024-03-28 18:57:36 +0100, Nicolas Gaullier wrote:
> Yet another probesize used to get the durations when
> estimate_timings_from_pts is required. It is 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/APIchanges              |  3 +++
>  doc/formats.texi            | 19 ++++++++++++++++++-
>  libavformat/avformat.h      | 16 ++++++++++++++--
>  libavformat/demux.c         | 13 ++++++++-----
>  libavformat/options_table.h |  1 +
>  libavformat/version.h       |  2 +-
>  6 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index aa102b4925..f709db536d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-03-28 - xxxxxxxxxx - lavf 61.3.100 - avformat.h
> +  Add AVFormatContext.duration_probesize.
> +
>  2024-03-27 - xxxxxxxxxx - lavu 59.10.100 - frame.h
>    Add AVSideDataDescriptor, enum AVSideDataProps, and
>    av_frame_side_data_desc().
> diff --git a/doc/formats.texi b/doc/formats.texi
> index 69fc1457a4..3fe7fa9d8d 100644
> --- a/doc/formats.texi
> +++ b/doc/formats.texi
> @@ -225,9 +225,26 @@ 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.
>  At present, applicable for MPEG-PS and MPEG-TS.
>  
> + at item duration_probesize @var{integer} (@emph{input})
> +Set probing size, in bytes, for input duration estimation when it actually requires
> +an additional probing for PTS at end of file (at present: MPEG-PS and MPEG-TS).
> +It is aimed at users interested in better durations probing for itself, or indirectly
> +because using the concat demuxer, for example.

> +The typical use case is an MPEG-TS CBR with a high bitrate, high video buffering and
> +ending cleaning with similar PTS for video and audio: in such a scenario, the large
> +physical gap between the last video packet and the last audio packet makes it necessary
> +to read many bytes in order to get the video stream duration.
> +Another use case is where the default probing behaviour only reaches a single video frame which is
> +not the last one of the stream due to frame reordering, so the duration is not accurate.


> +Setting the duration_probesize has a performance impact even for small files because the probing
> +size is fixed.

nit++:
setting the @option{duration_probesize}
or ...
setting this option

> +Default behaviour is a general purpose 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.

I'm a bit concerned if we should really mention these values, since
they are currently hardcoded and this might result in inconsistent
documentation in case of update (probably we can only mention that it
is adaptive therefore avoiding to expose the internal thresholds).

> +Must be an integer not lesser than 1, or 0 for default behaviour.
> +

[...]

Looks good to me otherwise, thanks.