[FFmpeg-devel] [PATCH 1/2] avformat/movenc: initialize pts/dts/duration of timecode packet

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> On Fri, Mar 11, 2022 at 05:16:09PM +0100, Andreas Rheinhardt wrote:
>> lance.lmwang at gmail.com:
>>> On Fri, Mar 11, 2022 at 03:04:32PM +0100, Andreas Rheinhardt wrote:
>>>> lance.lmwang at gmail.com:
>>>>> On Wed, Mar 02, 2022 at 09:58:31PM +0800, lance.lmwang at gmail.com wrote:
>>>>>> From: Limin Wang <lance.lmwang at gmail.com>
>>>>>>
>>>>>> Fix below error message when timecode packet is written.
>>>>>> "Application provided duration: -9223372036854775808 / timestamp: -9223372036854775808 is out of range for mov/mp4 format"
>>>>>>
>>>>>> try to reproduce by:
>>>>>> ffmpeg -y -f lavfi -i color -metadata "timecode=00:00:00:00" -t 1 test.mov
>>>>>>
>>>>>> Note although error message is printed, the timecode packet will be written anyway. So
>>>>>> the patch 2/2 will try to change the log level to warning.
>>>>>>
>>>>>> The first two test case of fate-lavf-ismv have timecode setting, so the crc of ref data is different.
>>>>>> Fixes ticket #9488
>>>>>>
>>>>>> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>>>>>> ---
>>>>>>  libavformat/movenc.c | 2 ++
>>>>>>  tests/ref/lavf/ismv  | 4 ++--
>>>>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>>> index 4c86891..74b94cd 100644
>>>>>> --- a/libavformat/movenc.c
>>>>>> +++ b/libavformat/movenc.c
>>>>>> @@ -6383,6 +6383,8 @@ static int mov_create_timecode_track(AVFormatContext *s, int index, int src_inde
>>>>>>      pkt->data = data;
>>>>>>      pkt->stream_index = index;
>>>>>>      pkt->flags = AV_PKT_FLAG_KEY;
>>>>>> +    pkt->pts = pkt->dts = av_rescale_q(tc.start, av_inv_q(rate), (AVRational){1,mov->movie_timescale});
>>>>>> +    pkt->duration = av_rescale_q(1, av_inv_q(rate), (AVRational){1,mov->movie_timescale});
>>>>>>      pkt->size = 4;
>>>>>>      AV_WB32(pkt->data, tc.start);
>>>>>>      ret = ff_mov_write_packet(s, pkt);
>>>>>> diff --git a/tests/ref/lavf/ismv b/tests/ref/lavf/ismv
>>>>>> index ac7f72b..723b432 100644
>>>>>> --- a/tests/ref/lavf/ismv
>>>>>> +++ b/tests/ref/lavf/ismv
>>>>>> @@ -1,7 +1,7 @@
>>>>>> -48fb8d7a5d19bd60f3a49ccf4b7d6593 *tests/data/lavf/lavf.ismv
>>>>>> +7a24b73c096ec0f13f0f7a2d9101c4c1 *tests/data/lavf/lavf.ismv
>>>>>>  313169 tests/data/lavf/lavf.ismv
>>>>>>  tests/data/lavf/lavf.ismv CRC=0x9d9a638a
>>>>>> -d19cd8e310a2e94fe0a0d11c5dc29217 *tests/data/lavf/lavf.ismv
>>>>>> +79646383fd099d45ad0d0c2791c601dd *tests/data/lavf/lavf.ismv
>>>>>>  322075 tests/data/lavf/lavf.ismv
>>>>>>  tests/data/lavf/lavf.ismv CRC=0xe8130120
>>>>>>  3b6023766845b51b075aed474c00f73c *tests/data/lavf/lavf.ismv
>>>>>> -- 
>>>>>> 1.8.3.1
>>>>>>
>>>>>
>>>>> will apply the patch set tomorrow unless there are any objections.
>>>>>
>>>>
>>>> You have not really answered whether the current files or the new files
>>>> are spec-incompliant; you have just reported that one byte is different.
>>>
>>> Sorry, I think I have said both current and new file is spec-compliant in the last
>>> email. 
>>>
>>
>> You stated that you think that both files are valid, but you also said
>> that you don't even know what this byte that is different actually means.
>>
>>> By Quicktime file format specs:
>>> Section Timecode Sample Description, all tmcd field isn't used pts/dts.
>>>
>>> As for where is the different for one byte, it's caused by pkt->duration. The
>>> old is 0(uninitialized), after the patch it's 33(1 frame duration).  
>>>
>>
>> The text about Timecode Sample Description reads as follows: "Frame
>> duration: A 32-bit integer that indicates how long each frame lasts in
>> real time." This implies that only one of the two files can be
>> spec-compliant. I am not a mov/ISOBMFF expert, but it seems to me that
>> the current way of doing things is wrong. But I wonder about whether
>> your patch is correct for vfr content. Doesn't the property of being vfr
>> need to be reflected in the timecodes somehow (with different durations
>> for different packets)?
> 
> Andreas, I have updated the patch and remove the fate difference which is
> caused by duration, do you have any other comments for v2 patch?
> 

No.

- Andreas