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

[James Almer]

On 5/20/2021 7:57 PM, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-21 00:37, James Almer wrote:
>> 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.
>>
> Directly above it:
> 
> Muxers should treat all guidelines marked SHOULD in this section as 
> MUST. This will foster consistency across WebM files in the real world.

Oh FFS, don't use rfc2119 keywords verbatim in your spec if you're not 
compliant with it.

> 
> I missed it as well as it just didn't look important at first.

Thanks for clearing that out.

> 
>>>>>
>>>>>>
>>>>>> 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.
> 
>