Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 19/06/2019 15:36, Derek Buitenhuis wrote:
On 19/06/2019 06:43, Gyan wrote:
setting track_duration is inconsistent; some times it includes
duration and some times not.
It may be best to check the commits for these assignments to see if the
inconsistency is deliberate.
The track duration is written into the media header box for the track. I
also see it being used elsewhere to adjust dts. Do those roles remain
intact?
Does FATE pass?
Wouldn't the correct fix be to fix the places where duration is *not*
used?
Writing the track duration without taking into account the actual
packet duration of the last packet is just wrong. That's not the
track's duration, and is in fact very problematic for low frame
rate files, such a slide shows; QuickTime will cut off at the
end of the media and track duration, dropping possibly a whole
slide, for example.
I tested with this relatively innocent patch:
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 46d314ff17..719c491869 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5012,7 +5012,7 @@ static int mov_flush_fragment(AVFormatContext *s, int
force)
if (!ff_interleaved_peek(s, i, , 1)) {
if (track->dts_shift != AV_NOPTS_VALUE)
pkt.dts += track->dts_shift;
-track->track_duration = pkt.dts - track->start_dts;
+track->track_duration = pkt.dts - track->start_dts +
pkt.duration;
if (pkt.pts != AV_NOPTS_VALUE)
track->end_pts = pkt.pts;
else
but this broke the FATE test for movenc:
make fate-movenc SAMPLES=fate-suite/
LD libavformat/tests/movenc
TESTmovenc
--- ./tests/ref/fate/movenc 2019-03-24 10:21:55.0 +0100
+++ tests/data/fate/movenc 2019-06-20 14:27:27.0 +0200
@@ -134,12 +134,12 @@
3c2c3f98c8a047f0ecefff07570fd457 9299 large_frag
write_data len 1231, time nopts, type header atom ftyp
write_data len 684, time -3, type sync atom moof
-write_data len 504, time 80, type boundary atom moof
-write_data len 420, time 127, type boundary atom moof
-write_data len 668, time 157, type sync atom moof
-write_data len 440, time 223, type boundary atom moof
+write_data len 504, time 83, type boundary atom moof
+write_data len 512, time 130, type boundary atom moof
+write_data len 792, time 157, type sync atom moof
+write_data len 488, time 223, type boundary atom moof
write_data len 262, time nopts, type trailer atom -
-edd19deae2b70afcf2cd744b89b7013d 4209 vfr-noduration-interleave
+f579e7fec9c37179ed2def2f8930a093 4473 vfr-noduration-interleave
write_data len 1231, time nopts, type header atom ftyp
write_data len 916, time 0, type sync atom moof
write_data len 908, time 100, type sync atom moof
Test movenc failed. Look at tests/data/fate/movenc.err for details.
make: *** [fate-movenc] Error 1
/alfred
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 19/06/2019 15:36, Derek Buitenhuis wrote: On 19/06/2019 06:43, Gyan wrote: setting track_duration is inconsistent; some times it includes duration and some times not. It may be best to check the commits for these assignments to see if the inconsistency is deliberate. The track duration is written into the media header box for the track. I also see it being used elsewhere to adjust dts. Do those roles remain intact? Does FATE pass? Wouldn't the correct fix be to fix the places where duration is *not* used? Writing the track duration without taking into account the actual packet duration of the last packet is just wrong. That's not the track's duration, and is in fact very problematic for low frame rate files, such a slide shows; QuickTime will cut off at the end of the media and track duration, dropping possibly a whole slide, for example. Hi, thanks for your comments. I agree that track_duration should include duration, I will take a look at this tomorrow. but why do you require that dts > prev_dts + prev_dur when dur is unknown ? I think this check is too strict. a better check would be: dts > prev_dts a very simple example of how to demonstrate this: $ ffmpeg -y -loglevel info \ -listen 1 -i rtmp://127.0.0.1:1935/live \ -f dash out.mpd and then use ffmpeg to send FLV/RTMP stream to 127.0.0.1 after some seconds ffmpeg prints this warning: [mp4 @ 0x7faffe065600] Application provided duration: -2 / timestamp: 1761244 is out of range for mov/mp4 format /alfred ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 19/06/2019 06:43, Gyan wrote: >> setting track_duration is inconsistent; some times it includes >> duration and some times not. > It may be best to check the commits for these assignments to see if the > inconsistency is deliberate. > The track duration is written into the media header box for the track. I > also see it being used elsewhere to adjust dts. Do those roles remain > intact? > > Does FATE pass? Wouldn't the correct fix be to fix the places where duration is *not* used? Writing the track duration without taking into account the actual packet duration of the last packet is just wrong. That's not the track's duration, and is in fact very problematic for low frame rate files, such a slide shows; QuickTime will cut off at the end of the media and track duration, dropping possibly a whole slide, for example. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 18-06-2019 02:00 PM, Alfred E. Heggestad wrote:
On 17/06/2019 10:23, Gyan wrote:
On 17-06-2019 01:37 PM, Alfred E. Heggestad wrote:
From c69b63a7af5531257753754e64ac33b7ef530e75 Mon Sep 17 00:00:00 2001
From: "Alfred E. Heggestad"
Date: Mon, 17 Jun 2019 10:04:08 +0200
Subject: [PATCH] movenc: calculate track_duration without packet
duration
---
libavformat/movenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 46d314ff17..fa5833962b 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5486,7 +5486,7 @@ int ff_mov_write_packet(AVFormatContext *s,
AVPacket *pkt)
"this case.\n",
pkt->stream_index, pkt->dts);
}
- trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
+ trk->track_duration = pkt->dts - trk->start_dts;
Why?
trk->last_sample_is_subtitle_end = 0;
if (pkt->pts == AV_NOPTS_VALUE) {
the background for this change is the check done in check_pkt():
if (trk->entry) {
ref = trk->cluster[trk->entry - 1].dts;
} else if ( trk->start_dts != AV_NOPTS_VALUE
&& !trk->frag_discont) {
ref = trk->start_dts + trk->track_duration;
} else
ref = pkt->dts; // Skip tests for the first packet
if (trk->dts_shift != AV_NOPTS_VALUE) {
/* With negative CTS offsets we have set an offset to the DTS,
* reverse this for the check. */
ref -= trk->dts_shift;
}
duration = pkt->dts - ref;
if (pkt->dts < ref || duration >= INT_MAX) {
av_log(s, AV_LOG_ERROR, "Application provided duration:
%"PRId64" / timestamp: %"PRId64" is out of range for mov/mp4 format\n",
duration, pkt->dts
);
pkt->dts = ref + 1;
pkt->pts = AV_NOPTS_VALUE;
}
it requires that the DTS of the packet is larger than
"ref" which in some cases is equal to track_duration.
line 5015:
track->track_duration = pkt.dts - track->start_dts;
line 5489:
trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
line 5615:
trk->track_duration = pkt->dts - trk->start_dts;
setting track_duration is inconsistent; some times it includes
duration and some times not.
It may be best to check the commits for these assignments to see if the
inconsistency is deliberate.
The track duration is written into the media header box for the track. I
also see it being used elsewhere to adjust dts. Do those roles remain
intact?
Does FATE pass?
Gyan
here is an example with a stream of AVPacket's that increment
the DTS by approx. 10 (and duration is set to 0):
pkt: dts:
1 0
2 10
3 20
4 30
5 40
6 50
after packet 6 has arrived, the "ref" value in check_pkt will now
be calculated to 50 + 10 = 60. the code assumes a duration of 10.
when packet 7 arrives it has a DTS of 59 (which is valid).
but this packet will fail in check_pkt and print a warning,
because 59 < 60.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 17/06/2019 10:23, Gyan wrote:
On 17-06-2019 01:37 PM, Alfred E. Heggestad wrote:
From c69b63a7af5531257753754e64ac33b7ef530e75 Mon Sep 17 00:00:00 2001
From: "Alfred E. Heggestad"
Date: Mon, 17 Jun 2019 10:04:08 +0200
Subject: [PATCH] movenc: calculate track_duration without packet duration
---
libavformat/movenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 46d314ff17..fa5833962b 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5486,7 +5486,7 @@ int ff_mov_write_packet(AVFormatContext *s,
AVPacket *pkt)
"this case.\n",
pkt->stream_index, pkt->dts);
}
- trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
+ trk->track_duration = pkt->dts - trk->start_dts;
Why?
trk->last_sample_is_subtitle_end = 0;
if (pkt->pts == AV_NOPTS_VALUE) {
the background for this change is the check done in check_pkt():
if (trk->entry) {
ref = trk->cluster[trk->entry - 1].dts;
} else if ( trk->start_dts != AV_NOPTS_VALUE
&& !trk->frag_discont) {
ref = trk->start_dts + trk->track_duration;
} else
ref = pkt->dts; // Skip tests for the first packet
if (trk->dts_shift != AV_NOPTS_VALUE) {
/* With negative CTS offsets we have set an offset to the DTS,
* reverse this for the check. */
ref -= trk->dts_shift;
}
duration = pkt->dts - ref;
if (pkt->dts < ref || duration >= INT_MAX) {
av_log(s, AV_LOG_ERROR, "Application provided duration:
%"PRId64" / timestamp: %"PRId64" is out of range for mov/mp4 format\n",
duration, pkt->dts
);
pkt->dts = ref + 1;
pkt->pts = AV_NOPTS_VALUE;
}
it requires that the DTS of the packet is larger than
"ref" which in some cases is equal to track_duration.
line 5015:
track->track_duration = pkt.dts - track->start_dts;
line 5489:
trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
line 5615:
trk->track_duration = pkt->dts - trk->start_dts;
setting track_duration is inconsistent; some times it includes
duration and some times not.
here is an example with a stream of AVPacket's that increment
the DTS by approx. 10 (and duration is set to 0):
pkt: dts:
1 0
210
320
430
540
650
after packet 6 has arrived, the "ref" value in check_pkt will now
be calculated to 50 + 10 = 60. the code assumes a duration of 10.
when packet 7 arrives it has a DTS of 59 (which is valid).
but this packet will fail in check_pkt and print a warning,
because 59 < 60.
/alfred
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
On 17-06-2019 01:37 PM, Alfred E. Heggestad wrote:
From c69b63a7af5531257753754e64ac33b7ef530e75 Mon Sep 17 00:00:00 2001
From: "Alfred E. Heggestad"
Date: Mon, 17 Jun 2019 10:04:08 +0200
Subject: [PATCH] movenc: calculate track_duration without packet duration
---
libavformat/movenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 46d314ff17..fa5833962b 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5486,7 +5486,7 @@ int ff_mov_write_packet(AVFormatContext *s,
AVPacket *pkt)
"this case.\n",
pkt->stream_index, pkt->dts);
}
- trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
+ trk->track_duration = pkt->dts - trk->start_dts;
Why?
trk->last_sample_is_subtitle_end = 0;
if (pkt->pts == AV_NOPTS_VALUE) {
Gyan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] movenc: calculate track_duration without packet duration
From c69b63a7af5531257753754e64ac33b7ef530e75 Mon Sep 17 00:00:00 2001
From: "Alfred E. Heggestad"
Date: Mon, 17 Jun 2019 10:04:08 +0200
Subject: [PATCH] movenc: calculate track_duration without packet duration
---
libavformat/movenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 46d314ff17..fa5833962b 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5486,7 +5486,7 @@ int ff_mov_write_packet(AVFormatContext *s,
AVPacket *pkt)
"this case.\n",
pkt->stream_index, pkt->dts);
}
-trk->track_duration = pkt->dts - trk->start_dts + pkt->duration;
+trk->track_duration = pkt->dts - trk->start_dts;
trk->last_sample_is_subtitle_end = 0;
if (pkt->pts == AV_NOPTS_VALUE) {
--
2.20.1 (Apple Git-117)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
