[FFmpeg-devel,PATCHv2] movenc: Write durations based on pts into mvhd/mdhd/tkhd/elst

On Tue, Dec 17, 2019 at 03:15:09PM +0200, Martin Storsjö wrote:
> Keep all the existing data fields as they are (there's lots and
> lots of nontrivial calculation and heuristics based on them in
> their current form), but derive the duration as the difference
> between the pts of the first packet to the maximum pts+duration
> (not necessarily the last packet); use this duration in any box
> where the actual presentation duration is supposed to be.
> 
> Fixes: 8420
> ---
> Fixed to fetch the duration for tmcd tracks from their designated
> source track.
> ---
>  libavformat/movenc.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)

I found another case that changes, again dont know which is more correct

make -j12 && ./ffmpeg -i ~/tickets/3453/mov_with_tmcd.mov -y  -bitexact -codec copy -map 0 -t 2 file.mov ; ./ffprobe -v 0 file.mov -show_packets -print_format compact > /tmp/before



file should be here:
https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3453/


[...]

On Fri, 20 Dec 2019, Michael Niedermayer wrote:

> On Tue, Dec 17, 2019 at 03:15:09PM +0200, Martin Storsjö wrote:
>> Keep all the existing data fields as they are (there's lots and
>> lots of nontrivial calculation and heuristics based on them in
>> their current form), but derive the duration as the difference
>> between the pts of the first packet to the maximum pts+duration
>> (not necessarily the last packet); use this duration in any box
>> where the actual presentation duration is supposed to be.
>>
>> Fixes: 8420
>> ---
>> Fixed to fetch the duration for tmcd tracks from their designated
>> source track.
>> ---
>>  libavformat/movenc.c | 35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> I found another case that changes, again dont know which is more correct
>
> make -j12 && ./ffmpeg -i ~/tickets/3453/mov_with_tmcd.mov -y  -bitexact -codec copy -map 0 -t 2 file.mov ; ./ffprobe -v 0 file.mov -show_packets -print_format compact > /tmp/before
>
> --- /tmp/before	2019-12-20 23:28:04.009327038 +0100
> +++ /tmp/after	2019-12-20 23:27:17.213326052 +0100
> @@ -188,7 +188,7 @@
> packet|codec_type=audio|stream_index=1|pts=88320|pts_time=1.840000|dts=88320|dts_time=1.840000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=408905|flags=K_
> packet|codec_type=data|stream_index=2|pts=46|pts_time=1.840000|dts=46|dts_time=1.840000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=413001|flags=K_
> packet|codec_type=audio|stream_index=1|pts=89344|pts_time=1.861333|dts=89344|dts_time=1.861333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=413005|flags=K_
> -packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=_D
> +packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=__
> packet|codec_type=audio|stream_index=1|pts=90240|pts_time=1.880000|dts=90240|dts_time=1.880000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=417365|flags=K_
> packet|codec_type=data|stream_index=2|pts=47|pts_time=1.880000|dts=47|dts_time=1.880000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=421461|flags=K_
> packet|codec_type=audio|stream_index=1|pts=91264|pts_time=1.901333|dts=91264|dts_time=1.901333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=421465|flags=K_

I would say the new behaviour is more correct here.

This command writes a truncated sequence; the sequence of muxed packets 
is this:

pts=0|pts_time=0.000000|dts=-1024|dts_time=-0.080000|duration=512|duration_time=0.040000
...
pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000
pts=25600|pts_time=2.000000|dts=24576|dts_time=1.920000|duration=512|duration_time=0.040000
pts=25088|pts_time=1.960000|dts=25088|dts_time=1.960000|duration=512|duration_time=0.040000

Based on DTS, this previously wrote a total duration of 1.96 + 0.04 - 
(-0.08) = 2.08.

This duration also was set in the edit list, which then made the last 
frame (in presentation order) with a pts of 2.08 to be outside of the edit 
list, and thus be marked discardable.

With the patch, the duration of the file gets written as 2.08 + 0.04 - 
0.00 = 2.12, and thus the last frame no longer is marked as discardable.


If the reordering sequene wasn't truncated, if the following packet also 
would be included, the duration would end up calculated the same with both 
DTS and PTS:

pts=26112|pts_time=2.040000|dts=25600|dts_time=2.000000|duration=512|duration_time=0.040000


// Martin

On Wed, 8 Jan 2020, Martin Storsjö wrote:

> On Fri, 20 Dec 2019, Michael Niedermayer wrote:
>
>> On Tue, Dec 17, 2019 at 03:15:09PM +0200, Martin Storsjö wrote:
>>> Keep all the existing data fields as they are (there's lots and
>>> lots of nontrivial calculation and heuristics based on them in
>>> their current form), but derive the duration as the difference
>>> between the pts of the first packet to the maximum pts+duration
>>> (not necessarily the last packet); use this duration in any box
>>> where the actual presentation duration is supposed to be.
>>>
>>> Fixes: 8420
>>> ---
>>> Fixed to fetch the duration for tmcd tracks from their designated
>>> source track.
>>> ---
>>>  libavformat/movenc.c | 35 ++++++++++++++++++++++++++++-------
>>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> I found another case that changes, again dont know which is more correct
>>
>> make -j12 && ./ffmpeg -i ~/tickets/3453/mov_with_tmcd.mov -y  -bitexact 
> -codec copy -map 0 -t 2 file.mov ; ./ffprobe -v 0 file.mov -show_packets 
> -print_format compact > /tmp/before
>>
>> --- /tmp/before	2019-12-20 23:28:04.009327038 +0100
>> +++ /tmp/after	2019-12-20 23:27:17.213326052 +0100
>> @@ -188,7 +188,7 @@
>> 
> packet|codec_type=audio|stream_index=1|pts=88320|pts_time=1.840000|dts=88320|dts_time=1.840000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=408905|flags=K_
>> 
> packet|codec_type=data|stream_index=2|pts=46|pts_time=1.840000|dts=46|dts_time=1.840000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=413001|flags=K_
>> 
> packet|codec_type=audio|stream_index=1|pts=89344|pts_time=1.861333|dts=89344|dts_time=1.861333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=413005|flags=K_
>> 
> -packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=_D
>> 
> +packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=__
>> 
> packet|codec_type=audio|stream_index=1|pts=90240|pts_time=1.880000|dts=90240|dts_time=1.880000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=417365|flags=K_
>> 
> packet|codec_type=data|stream_index=2|pts=47|pts_time=1.880000|dts=47|dts_time=1.880000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=421461|flags=K_
>> 
> packet|codec_type=audio|stream_index=1|pts=91264|pts_time=1.901333|dts=91264|dts_time=1.901333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=421465|flags=K_
>
> I would say the new behaviour is more correct here.

So, if there's no new remarks to this patch, I'd like to push e.g. 
tomorrow.

// Martin

On Thu, Jan 09, 2020 at 11:32:03PM +0200, Martin Storsjö wrote:
> On Wed, 8 Jan 2020, Martin Storsjö wrote:
> 
> >On Fri, 20 Dec 2019, Michael Niedermayer wrote:
> >
> >>On Tue, Dec 17, 2019 at 03:15:09PM +0200, Martin Storsjö wrote:
> >>>Keep all the existing data fields as they are (there's lots and
> >>>lots of nontrivial calculation and heuristics based on them in
> >>>their current form), but derive the duration as the difference
> >>>between the pts of the first packet to the maximum pts+duration
> >>>(not necessarily the last packet); use this duration in any box
> >>>where the actual presentation duration is supposed to be.
> >>>
> >>>Fixes: 8420
> >>>---
> >>>Fixed to fetch the duration for tmcd tracks from their designated
> >>>source track.
> >>>---
> >>> libavformat/movenc.c | 35 ++++++++++++++++++++++++++++-------
> >>> 1 file changed, 28 insertions(+), 7 deletions(-)
> >>
> >>I found another case that changes, again dont know which is more correct
> >>
> >>make -j12 && ./ffmpeg -i ~/tickets/3453/mov_with_tmcd.mov -y  -bitexact
> >-codec copy -map 0 -t 2 file.mov ; ./ffprobe -v 0 file.mov -show_packets
> >-print_format compact > /tmp/before
> >>
> >>--- /tmp/before	2019-12-20 23:28:04.009327038 +0100
> >>+++ /tmp/after	2019-12-20 23:27:17.213326052 +0100
> >>@@ -188,7 +188,7 @@
> >>
> >packet|codec_type=audio|stream_index=1|pts=88320|pts_time=1.840000|dts=88320|dts_time=1.840000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=408905|flags=K_
> >>
> >packet|codec_type=data|stream_index=2|pts=46|pts_time=1.840000|dts=46|dts_time=1.840000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=413001|flags=K_
> >>
> >packet|codec_type=audio|stream_index=1|pts=89344|pts_time=1.861333|dts=89344|dts_time=1.861333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=413005|flags=K_
> >>
> >-packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=_D
> >>
> >+packet|codec_type=video|stream_index=0|pts=26624|pts_time=2.080000|dts=24064|dts_time=1.880000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=776|pos=416589|flags=__
> >>
> >packet|codec_type=audio|stream_index=1|pts=90240|pts_time=1.880000|dts=90240|dts_time=1.880000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=4096|pos=417365|flags=K_
> >>
> >packet|codec_type=data|stream_index=2|pts=47|pts_time=1.880000|dts=47|dts_time=1.880000|duration=1|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=4|pos=421461|flags=K_
> >>
> >packet|codec_type=audio|stream_index=1|pts=91264|pts_time=1.901333|dts=91264|dts_time=1.901333|duration=896|duration_time=0.018667|convergence_duration=N/A|convergence_duration_time=N/A|size=3584|pos=421465|flags=K_
> >
> >I would say the new behaviour is more correct here.
> 
> So, if there's no new remarks to this patch, I'd like to push e.g. tomorrow.

ok with me

Thanks

[...]