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

Fri May 21 01:57:57 EEST 2021

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.

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

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

-- 
Sincerely | Mit freundlichen Grüßen

Michael Fabian 'Xaymar' Dirks
Software Designer & Engineer