Hi Martin, On Sat, 19 Dec 2020 at 15:10, Martin Storsjö <mar...@martin.st> wrote: > > On Fri, 11 Dec 2020, Josh Allmann wrote: > > > On Fri, 11 Dec 2020 at 14:07, Martin Storsjö <mar...@martin.st> wrote: > >> > >> On Fri, 11 Dec 2020, Josh Allmann wrote: > >> > >> > A negative start_dts value (eg, delay from edit lists) typically yields > >> > a duration larger than end_pts. During edit list processing, the > >> > delay is removed again, yielding the correct duration within the elst. > >> > > >> > However, other duration-carrying atoms (tkhd, mvhd, mdhd) still have > >> > the delay incorporated into their durations. This is incorrect. > >> > > >> > Fix this by withholding delay from the duration if edit lists are used. > >> > This also simplifies edit-list processing a bit, since the delay > >> > does not need to be removed from the calculated duration again. > >> > --- > >> > > >> > The mov spec says that the tkhd duration is "derived from the track's > >> > edits" [1] and the duratons of the other atoms (mvhd, mdhd) are in turn > >> > taken from the longest track. So it seems that incorporating the delay > >> > into the track duration is a bug in itself when the edit list has the > >> > correct duration, and this propagates out tothe other top-level > >> > durations. > >> > > >> > Unsure of how this change interacts with other modes that may expect > >> > negative timestamps such as CMAF, so the patch errs on the side of > >> > caution and only takes effect if edit lists are used. Can loosen that > >> > up if necessary. > >> > > >> > [1] > >> > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-BBCEIDFA > >> > > >> > libavformat/movenc.c | 13 ++++++++----- > >> > 1 file changed, 8 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > >> > index 7db2e28840..31441a9f6c 100644 > >> > --- a/libavformat/movenc.c > >> > +++ b/libavformat/movenc.c > >> > @@ -2831,7 +2831,14 @@ static int64_t calc_pts_duration(MOVMuxContext > >> > *mov, MOVTrack *track) > >> > if (track->end_pts != AV_NOPTS_VALUE && > >> > track->start_dts != AV_NOPTS_VALUE && > >> > track->start_cts != AV_NOPTS_VALUE) { > >> > - return track->end_pts - (track->start_dts + track->start_cts); > >> > + int64_t dur = track->end_pts, delay = track->start_dts + > >> > track->start_cts; > >> > + /* Note, this delay is calculated from the pts of the first > >> > sample, > >> > + * ensuring that we don't reduce the duration for cases with > >> > + * dts<0 pts=0. */ > >> > >> If you have a stream starting with dts<0 pts=0, you'll have start_pts = > >> start_dts + start_cts = 0. That gives delay=0 after your modification. But > >> the comment says "don't reduce the duration for cases with pts=0" - where > >> the delay variable would be zero anyway? > >> > > > > I'm not quite sure what you mean - that the comment is outdated? > > Or that this modification would perhaps not behave as expected? > > > > For what it's worth, the cases I'm concerned with have start_pts < 0. > > > >> > >> > >> I don't manage to follow the reasoning and explanation in the commit > >> message. To be able to concretely reason about this issue at all, we need > >> to look at a concrete example. Can you provide a sample input file and a > >> reproducible command, and point out which exact field in the muxer output > >> of that case that you consider wrong? > >> > > > > Had to create a trac to find somewhere to host the sample. Tried to put > > some details there but the formatting seems messed up and I can't figure > > out how to edit, apologies. So here is some more info - > > > > Input sample: > > > > https://trac.ffmpeg.org/raw-attachment/ticket/9028/test-timecode.mp4 > > > > Run the following for a transmuxed clip from 3s for a 5s duration: > > > > ffmpeg -ss 3 -i test-timecode.mp4 -t 5 -c copy out.mp4 > > > > Note that the actual cut location is mid-GOP, so there's a 1s pts delay > > at the beginning of the output file with negative pts. > > > > ffprobe shows: > > > > ffprobe -show_streams -show_format out.mp4 2>&1 | grep duration= > > > > duration=5.166992 # stream duration - correct > > duration=6.167000 # format duration - incorrect > > > > mp4dump'ing out.mp4 gives this: > > > > # incorrect: duration should be sum of elst durations > > [tkhd] size=12+80, flags=3 > > duration = 6167 > > > > # correct > > [elst] size=12+16 > > entry_count = 1 > > entry/segment duration = 5167 > > > > # incorrect; derived from track duration (tkhd) > > [mvhd] size=12+96 > > timescale = 1000 > > duration = 6167 > > Ok, now I've finally had time to dig into this. It does indeed seem like > what we produce right now is incorrect. > > I don't think your patch does the right thing for cases that start with > pts > 0. For those cases, the value returned by calc_pts_duration to > mov_write_edts_tag would need to only cover the sample data itself, but > for the other header durations would need to include the extra offset > (adding the extra pts to it). And overall, the patch feels to me as it > achieves it in a way that doesn't fit with how my mental model for this > code is set up. > > I've made an alternative patch that I'll post momentarily, where I try to > address the issue, but in a way that fits the way I understand the code. > > For your particular example code, it produces the same output as your > patch, but I believe that it should do the right thing for cases that > start with pts > 0 as well. (I practically didn't test that, but if > someone would have time to do it, I'd appreciate it!) >
Thank you for taking the time to look into this! Tested with your alternative patch and it does seem to be an improvement. I was able to find a case where it didn't seem to do the correct thing (described below), but this is not a regression; master doesn't do the correct thing, and neither does my patch. I haven't looked much at what the code is doing beyond running these tests, but could find some time to dig in early 2021 if it's not fixed by then. First, a working sample with pts > 0 . This has a 600-frame GOP (20s @ 30fps) to show that cutting mid-GOP works correctly. https://trac.ffmpeg.org/raw-attachment/ticket/9028/gop-600.mp4 Command to generate a clipped sample with edit list. This spans two GOPs. ffmpeg -ss 17.316666999999999 -i gop-600.mp4 -t 5 -c copy -movflags +faststart -y cut-mp4-600.mp4 Things look good with ffprobe, and playback works well with ffplay; it starts right around the 17-second mark. The sample is a timecode pattern so it is easy to verify. If the sample is a mpegts (rather than a mp4, all other things identical), then things are a bit odd: https://trac.ffmpeg.org/raw-attachment/ticket/9028/gop-600.ts Cut the sample with: ffmpeg -ss 17.316666999999999 -i gop-600.ts -t 5 -c copy -movflags +faststart -y cut-ts-600.mp4 Probing gives the correct duration with a start time of 2.683. However, playback starts at the 20 second mark. (Same timecode pattern, so easy to visually verify.) Incidentally, it seems that 20 - 2.683 = 17.316, which is the original cut position. ffprobe cut-ts-600.mp4 gives "Duration: 00:00:05.12, start: 2.683000" The trac ticket has a bit more info (including commands for how to generate the samples) but this is the gist of it. Thanks again and happy holidays. Josh _______________________________________________ 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".
