[FFmpeg-devel] [PATCH] avformat/matroskaenc: Allow changing the time stamp precision via option

[James Almer]

On 5/20/2021 7:33 PM, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-21 00:30, Michael Fabian 'Xaymar' Dirks wrote:
>> On 2021-05-21 00:25, James Almer wrote:
>>> On 5/20/2021 7:20 PM, Lynne wrote:
>>>> May 20, 2021, 20:08 by jamrial at gmail.com:
>>>>
>>>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>>>
>>>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>>>
>>>>>>> On 5/20/2021 2:18 PM, michael.dirks at xaymar.com wrote:
>>>>>>>
>>>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks at xaymar.com>
>>>>>>>>
>>>>>>>> Adds "timestamp_precision" to the available option for 
>>>>>>>> Matroska/WebM
>>>>>>>> muxing. The option enables users and developers to change the 
>>>>>>>> precision
>>>>>>>> of the time stamps in the Matroska/WebM container up to 1 
>>>>>>>> nanosecond,
>>>>>>>> which can aid with the proper detection of constant and variable 
>>>>>>>> rate
>>>>>>>> content.
>>>>>>>>
>>>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks 
>>>>>>>> <michael.dirks at xaymar.com>
>>>>>>>> ---
>>>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>>>> --- a/doc/muxers.texi
>>>>>>>> +++ b/doc/muxers.texi
>>>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that 
>>>>>>>> this option does not flip the bitmap
>>>>>>>>    which has to be done manually beforehand, e.g. by using the 
>>>>>>>> vflip filter.
>>>>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>>>>    + at item timestamp_precision
>>>>>>>> +Sets the timestamp precision up to 1 nanosecond for 
>>>>>>>> Matroska/WebM, which can
>>>>>>>> +improve detection of constant rate content in demuxers. Note 
>>>>>>>> that some poorly
>>>>>>>> +implemented demuxers may require a timestamp precision of 1 
>>>>>>>> millisecond, so
>>>>>>>> +increasing it past that point may result in playback issues. 
>>>>>>>> Higher precision
>>>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>>>
>>>>>>>
>>>>>>> Like i said, the default must be the one defined in the spec. 
>>>>>>> This version not only would need FATE test updates, it also like 
>>>>>>> you described makes all new files by default have a lot of 
>>>>>>> overhead from having one block per cluster.
>>>>>>>
>>>>>>
>>>>>> I am aware of what you said and I am also aware of Lynne said. I 
>>>>>> do not know who has the actual final say in this, all I know is 
>>>>>> that the maintainers for matroskaenc.c are "David Conrad" and 
>>>>>> "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>>>>
>>>>>
>>>>> Lynne agreed on IRC that it can remain as 1ms.
>>>>>
>>>> If there's a chance it could be changed to 1us (the common internal 
>>>> lavf timebase),
>>>> I'd like for it to be, but not without a small partial study of 
>>>> compatibility with common
>>>> implementations.
>>>> Specifically, the one case I've heard where non-1ms timebases had 
>>>> issues was with VLC's
>>>> demuxer, where apparently giving an external .mks file as subtitles 
>>>> with a non-1ms timebase
>>>> caused the player to OOM. If this is/was still true, for shame, for 
>>>> shame.
>>>
>>> 1us is incompatible with the default value for cluster_time_limit. 
>>> It's in fact incompatible with any value for it, since it will force 
>>> one block per cluster.
>>> If the user wants that, then they are free to set 1us, but the 
>>> default of one option should not invalidate the default of another. 
>>> It's the entire point the warning exists for and is printed.
>>>
>>>>
>>>> Also, the patch is missing some error-checking. WebM hard-requires a 
>>>> 1ms timebase.
>>>> You should likely error out if the demuxer acts in WebM mode and the 
>>>> timebase is set
>>>> to a different value.
>>>
>>> Good catch.
>>
>> I may be misreading the WebM specification, but it only says it SHOULD 
>> be set 1.000.000 nanoseconds, not MUST, which going by the rest of the 
>> wording does not mean it is required, just very very recommended. 
>> Perhaps a warning for WebM would be better than an error-exit?
>>
> I was misreading the specification. Shouldn't try to read technical 
> documentation when tired. Will add the error-exit, ignore the patch sent 
> before Lynnes message arrived here.

No, apparently you were right. In 
https://www.webmproject.org/docs/container/ there's a line that says 
"The TimecodeScale element SHOULD be set to a default of 1.000.000 
nanoseconds.", which means it's recommended but not required. Or is 
there another source for the spec?

Also, the spec uses TimecodeScale and TimestampScale interchangeably, 
which makes things a bit confusing.

>>>
>>>>
>>>> As for the whole "1ms is good enough for everything!", well, it's 
>>>> not for 960fps VFR video
>>>> and that's all I'm going to say. Where you get 960fps VFR videos? 
>>>> Smartphones.
>>>> Even a cheap one from 5 years ago can shoot at 240fps. They're 
>>>> really common, and
>>>> depending on what strategy you use to convert the video, rounding 
>>>> jitter can result in some
>>>> horrible slow-mo conversions.
>>>>
>>>>
>>>>> Probably should have said it here, but i guess neither him
>>>>>
>>>> she. Try to remember, this hasn't been the first time.
>>>
>>> Sorry, i just default to that in general. Will try to not make that 
>>> mistake again.
>>
>