diff mbox

[FFmpeg-devel] mov: Properly abide by the track's media duration

Message ID 20180418213538.GD20131@michaelspb
State Not Applicable
Headers show

Commit Message

Michael Niedermayer April 18, 2018, 9:35 p.m. UTC 
On Tue, Apr 17, 2018 at 08:59:48PM +0100, Derek Buitenhuis wrote:
> The track's media duration from the mdhd atom takes precedence
> over both the stts and elst atom for calculating and setting
> the track's total duraion.
> 
> Technically, we shouldn't be using the stts atom at all for
> calculating stream durations.
> 
> This fixes incorrect stream and final packet durations on files
> with edit lists that are longer than the media duration.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> Personally I'd have removed the incorrect setting of the stream
> duration in the stts reading code...
> ---
>  libavformat/mov.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

This breaks fate: (have not tested anything beyond this)

TEST    adtstoasc_ticket3715
Test adtstoasc_ticket3715 failed. Look at tests/data/fate/adtstoasc_ticket3715.err for details.
make: *** [fate-adtstoasc_ticket3715] Error 1

[...]

Comments

Jan Ekström April 18, 2018, 10:08 p.m. UTC | #1 
On Thu, Apr 19, 2018 at 12:35 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> This breaks fate: (have not tested anything beyond this)
>
> TEST    adtstoasc_ticket3715
> --- ./tests/ref/fate/adtstoasc_ticket3715       2018-04-17 14:20:30.500366780 +0200
> +++ tests/data/fate/adtstoasc_ticket3715        2018-04-18 23:34:00.751854625 +0200
> @@ -92,4 +92,4 @@
>  0,      83968,      83968,     1024,      465, 0xeb3ce0af
>  0,      84992,      84992,     1024,      326, 0x7be4a667
>  0,      86016,      86016,     1024,      339, 0x2cf4a71f
> -0,      87040,      87040,     1028,      258, 0xd4007ad4
> +0,      87040,      87040,     1024,      258, 0xd4007ad4
> Test adtstoasc_ticket3715 failed. Look at tests/data/fate/adtstoasc_ticket3715.err for details.
> make: *** [fate-adtstoasc_ticket3715] Error 1
>

Just an off-hand comment that given that audio frames in AAC tend to
have 1024 samples usually (or in more general, "1028 samples is quite
unlikely for AAC"), I'd say 1024 is correct for a duration of an AAC
frame. Looking at the hash, the actual data doesn't seem to have
gotten changed.

Best regards,
Jan
Derek Buitenhuis April 19, 2018, 12:51 p.m. UTC | #2 
On 4/18/2018 11:08 PM, Jan Ekström wrote:
> Just an off-hand comment that given that audio frames in AAC tend to
> have 1024 samples usually (or in more general, "1028 samples is quite
> unlikely for AAC"), I'd say 1024 is correct for a duration of an AAC
> frame. Looking at the hash, the actual data doesn't seem to have
> gotten changed.

Yes, I agree. I will resend with changes to FATE accounted for.

Going to wait until someone reviews the concept first.

CCed The author of the changed code in question (Sasi).

- Derek
diff mbox

Patch

--- ./tests/ref/fate/adtstoasc_ticket3715	2018-04-17 14:20:30.500366780 +0200
+++ tests/data/fate/adtstoasc_ticket3715	2018-04-18 23:34:00.751854625 +0200
@@ -92,4 +92,4 @@ 
 0,      83968,      83968,     1024,      465, 0xeb3ce0af
 0,      84992,      84992,     1024,      326, 0x7be4a667
 0,      86016,      86016,     1024,      339, 0x2cf4a71f
-0,      87040,      87040,     1028,      258, 0xd4007ad4
+0,      87040,      87040,     1024,      258, 0xd4007ad4