| Message ID | 1472050524-6611-1-git-send-email-derek.buitenhuis@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Derek! 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: > This breaks files with legitimate single-entry edit lists, > and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, > has no link to any known sample in its commit message, nor I suspect the commit message links to this sample: http://samples.ffmpeg.org/ffmpeg-bugs/roundup/issue910/ This reminds me that we should try to find the roundup mailing list archives, I don't think there is an online copy anymore;-( > does it actually fix the problem properly, but instead has > a one-off heuristic to try and "fix" them at the expense > of breaking legitimate files. Please remove the personal aspects from your commit message. [...} > Example file: > http://chromashift.org/samples/delay_problem.mp4 > > Having the DTS delay output on the first packet itself > is important for things like cutting files. > > The behavioral change can be seen with: > $ ffprobe -show_packets delay_problem.mp4 Could you explain in a less technical way (for me to understand) what is wrong with FFmpeg and this file currently? Thank you, Carl Eugen
2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: > I am sending this patch in a professional context, as it is > a problem I have run into at work, with legitimate files, > produced by x264 and L-SMASH. Does this patch also fix the issue? http://ffmpeg.org/pipermail/ffmpeg-devel/2016-August/197817.html Carl Eugen
On 8/24/2016 6:50 PM, Carl Eugen Hoyos wrote: >> This breaks files with legitimate single-entry edit lists, >> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, >> has no link to any known sample in its commit message, nor > > I suspect the commit message links to this sample: > http://samples.ffmpeg.org/ffmpeg-bugs/roundup/issue910/ Sample is good to have, but it's unclear to me what it was meant to actually fix... > This reminds me that we should try to find the roundup mailing > list archives, I don't think there is an online copy anymore;-( [...] >> does it actually fix the problem properly, but instead has >> a one-off heuristic to try and "fix" them at the expense >> of breaking legitimate files. > > Please remove the personal aspects from your commit message. I will remove this part. >> Example file: >> http://chromashift.org/samples/delay_problem.mp4 >> >> Having the DTS delay output on the first packet itself >> is important for things like cutting files. >> >> The behavioral change can be seen with: >> $ ffprobe -show_packets delay_problem.mp4 > > Could you explain in a less technical way (for me to understand) > what is wrong with FFmpeg and this file currently? The DTS of the first packet should be -36 due to the edit list in the file, but the heuristic erroneously removes it and sets it to AV_NOPTS_VALUE (the effect in ffprobe is that the first packet is just entirely missing the DTS field. Negative DTS are normal in MP4 files and can reflect, for example, decoding delay due to frame reordering. Also for example, if you do: $ ffprobe -ignore_editlist 1 -show_packets delay_problem.mp4 You'll see that if the edit list is not applied, the DTS is 0, and the PTS is 36. It follows that shifting everything backwards by 36, which is how we currently handle edit lists, should end up with a DTS of -36. Cheers, - Derek
On 8/24/2016 7:00 PM, Carl Eugen Hoyos wrote: > Does this patch also fix the issue? > http://ffmpeg.org/pipermail/ffmpeg-devel/2016-August/197817.html It doesn't. The patch I sent fixes a problem with a current workaround in the mov demuxer. Edit lists are just one way it could have triggered it. Another way, for example, is a mov with a larger-than-normal gap between the first DTS and PTS. The patch linked above is a new implementation of edit lists, which won't necessarily fix the same thing, except by coincidence. Cheers, - Derek
On Wed, Aug 24, 2016 at 03:55:24PM +0100, Derek Buitenhuis wrote: > This breaks files with legitimate single-entry edit lists, > and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, > has no link to any known sample in its commit message, nor > does it actually fix the problem properly, but instead has > a one-off heuristic to try and "fix" them at the expense > of breaking legitimate files. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > Let's get this out of the way first: I'm not 'returning to FFmpeg'. > > I do not intend to be involved in the community again, nor its > overall direction and discussions, short of a miracle occurring > at VideoLAN Dev Days. > > Let's please keep this to technical repleis only. I will ignore > anything else. > > I am sending this patch in a professional context, as it is > a problem I have run into at work, with legitimate files, > produced by x264 and L-SMASH. > > Example file: > http://chromashift.org/samples/delay_problem.mp4 > > Having the DTS delay output on the first packet itself > is important for things like cutting files. > > The behavioral change can be seen with: > $ ffprobe -show_packets delay_problem.mp4 > > Lastly, I would appreciate it if any replies to this patch set > use 'Reply All', to CC me directly, because I am not currently > subscribed to this mailing list, and getting proper IDs to use > in the In-Reply-To field isn't easy now that Gmane is dead. > > Cheers, > - Derek > --- > libavformat/mov.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 1bc3800..54c63ad 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -2802,12 +2802,8 @@ static void mov_build_index(MOVContext *mov, AVStream *st) > sc->time_offset = start_time - empty_duration; > current_dts = -sc->time_offset; > if (sc->ctts_count>0 && sc->stts_count>0 && > - sc->ctts_data[0].duration / FFMAX(sc->stts_data[0].duration, 1) > 16) { > - /* more than 16 frames delay, dts are likely wrong > - this happens with files created by iMovie */ > - sc->wrong_dts = 1; > + sc->ctts_data[0].duration / FFMAX(sc->stts_data[0].duration, 1) > 16) > st->codecpar->video_delay = 1; > - } IIRC the removed code tried to detect a reorder delay that is not possible in a valid file due to the profile constraints. Aka dts and pts are too far appart for the largest amount of buffers allowed in any codec. Quite possibly this limit or the check itself have become wrong over time ... Its even possible there has been some misunderstanding in the buffer cts/pts/dts limitations. patch probably ok iam a bit unsure about "st->codecpar->video_delay = 1;" though [...]
On 8/24/2016 10:54 PM, Michael Niedermayer wrote: > IIRC the removed code tried to detect a reorder delay that is not > possible in a valid file due to the profile constraints. Aka dts and > pts are too far appart for the largest amount of buffers allowed in > any codec. Basing this on timestamps after applying an edit list shift doesn't seem right at all. Maybe edts support didn't exist when it was added? > Quite possibly this limit or the check itself have become wrong > over time ... > Its even possible there has been some misunderstanding in the buffer > cts/pts/dts limitations. > > patch probably ok [...] > iam a bit unsure about "st->codecpar->video_delay = 1;" though This was added at the same time, yes. It also looks wrong to me, but I was not entirely sure. Do you think I should remove it? - Derek
On Thu, Aug 25, 2016 at 03:25:56PM +0100, Derek Buitenhuis wrote: > On 8/24/2016 10:54 PM, Michael Niedermayer wrote: > > IIRC the removed code tried to detect a reorder delay that is not > > possible in a valid file due to the profile constraints. Aka dts and > > pts are too far appart for the largest amount of buffers allowed in > > any codec. > > Basing this on timestamps after applying an edit list shift doesn't > seem right at all. Maybe edts support didn't exist when it was added? it seems code from around teh time contained only code to skip over the edit list data and print a warning but didnt use it at all > > > Quite possibly this limit or the check itself have become wrong > > over time ... > > Its even possible there has been some misunderstanding in the buffer > > cts/pts/dts limitations. > > > > patch probably ok > > [...] > > > iam a bit unsure about "st->codecpar->video_delay = 1;" though > > This was added at the same time, yes. It also looks wrong to me, > but I was not entirely sure. Do you think I should remove it? i honestly dont know but its probably best to remove in a seperate patch so if it breaks something bisect would immedeatly point to which of the 2 changes caused it [...]
On 8/25/2016 3:40 PM, Michael Niedermayer wrote: > but its probably best to remove in a seperate patch so if it breaks > something bisect would immedeatly point to which of the 2 changes > caused it Sounds good. If you think this patch is OK, please push it with this part of the commit message removed, as per Carl's request: ", nor does it actually fix the problem properly, but instead has a one-off heuristic to try and "fix" them at the expense of breaking legitimate files." - Derek.
On Thu, Aug 25, 2016 at 03:49:22PM +0100, Derek Buitenhuis wrote: > On 8/25/2016 3:40 PM, Michael Niedermayer wrote: > > but its probably best to remove in a seperate patch so if it breaks > > something bisect would immedeatly point to which of the 2 changes > > caused it > > Sounds good. > > If you think this patch is OK, please push it with this part the patch removes all uses of wrong_dts, the field should be removed too [...]
2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: > This breaks files with legitimate single-entry edit lists, > and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, I believe "Hack" is not acceptable on this mailing list anymore, please remove it from the commit message. > has no link to any known sample in its commit message I believe a sample is linked in the original commit message, please remove the sentence. Thank you, Carl EugenI
On Thu, Aug 25, 2016 at 05:45:03PM +0200, Michael Niedermayer wrote: > On Thu, Aug 25, 2016 at 03:49:22PM +0100, Derek Buitenhuis wrote: > > On 8/25/2016 3:40 PM, Michael Niedermayer wrote: > > > but its probably best to remove in a seperate patch so if it breaks > > > something bisect would immedeatly point to which of the 2 changes > > > caused it > > > > Sounds good. > > > > If you think this patch is OK, please push it with this part > > the patch removes all uses of wrong_dts, the field should be > removed too oops i forgot cc-ing you, iam not used to reply-all on the ML [...]
On 8/25/2016 4:52 PM, Michael Niedermayer wrote: >> the patch removes all uses of wrong_dts, the field should be >> > removed too > oops i forgot cc-ing you, iam not used to reply-all on the ML OK. I thought it was used in the FLV demuxer too, but it seems it has it's own copy inside the FLV context. My bad. New patches sent. - Derek
On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: > 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: >> This breaks files with legitimate single-entry edit lists, > >> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, > > I believe "Hack" is not acceptable on this mailing list anymore, please > remove it from the commit message. Care to point where this was discussed or agreed by the bulk of developers? I don't recall anything about it. A quick search for "hack" shows a lot of recent results in both the ML and the git repo, so I'm not sure why you assumed it's "not acceptable" anymore. > >> has no link to any known sample in its commit message > > I believe a sample is linked in the original commit message, > please remove the sentence. Forgot to CC him, so he didn't read your email. > > Thank you, Carl EugenI > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 8/25/2016 5:34 PM, James Almer wrote: > On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: >> 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: >>> This breaks files with legitimate single-entry edit lists, >> >>> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, >> >> I believe "Hack" is not acceptable on this mailing list anymore, please >> remove it from the commit message. > > Care to point where this was discussed or agreed by the bulk of developers? > I don't recall anything about it. > A quick search for "hack" shows a lot of recent results in both the ML and > the git repo, so I'm not sure why you assumed it's "not acceptable" anymore. What's a better word? "incorrect workaround"? Am I not allow to say some code is wrong? Because the code is defacto incorrect. > >> >>> has no link to any known sample in its commit message >> >> I believe a sample is linked in the original commit message, >> please remove the sentence. It is not. A filename is not a link. There is no way to locate the sample solely from the commit message. The description is accurate. - Derek
On 8/25/2016 1:46 PM, Derek Buitenhuis wrote: > On 8/25/2016 5:34 PM, James Almer wrote: >> On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: >>> 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: >>>> This breaks files with legitimate single-entry edit lists, >>> >>>> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, >>> >>> I believe "Hack" is not acceptable on this mailing list anymore, please >>> remove it from the commit message. >> >> Care to point where this was discussed or agreed by the bulk of developers? >> I don't recall anything about it. >> A quick search for "hack" shows a lot of recent results in both the ML and >> the git repo, so I'm not sure why you assumed it's "not acceptable" anymore. > > What's a better word? "incorrect workaround"? Am I not allow to say > some code is wrong? Because the code is defacto incorrect. Hack is perfectly fine. But if you want to change it, saying it's a faulty, wrong or incorrect workaround is also fine. > >> >>> >>>> has no link to any known sample in its commit message >>> >>> I believe a sample is linked in the original commit message, >>> please remove the sentence. > > It is not. A filename is not a link. There is no way to locate the > sample solely from the commit message. The description is accurate. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Thu, Aug 25, 2016 at 05:46:57PM +0100, Derek Buitenhuis wrote: > On 8/25/2016 5:34 PM, James Almer wrote: > > On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: > >> 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: > >>> This breaks files with legitimate single-entry edit lists, > >> > >>> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, > >> > >> I believe "Hack" is not acceptable on this mailing list anymore, please > >> remove it from the commit message. > > > > Care to point where this was discussed or agreed by the bulk of developers? > > I don't recall anything about it. > > A quick search for "hack" shows a lot of recent results in both the ML and > > the git repo, so I'm not sure why you assumed it's "not acceptable" anymore. > > What's a better word? "incorrect workaround"? Am I not allow to say > some code is wrong? Because the code is defacto incorrect. this is not really my area and i might misguess what was meant so i can only speak about what my feeling woudl suggest "Hack" might suggest the author had knowledge of the code being not the correct solution when adding it. Maybe this felt offensive [...]
On 8/25/16, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Aug 25, 2016 at 05:46:57PM +0100, Derek Buitenhuis wrote: >> On 8/25/2016 5:34 PM, James Almer wrote: >> > On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: >> >> 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis >> >> <derek.buitenhuis@gmail.com>: >> >>> This breaks files with legitimate single-entry edit lists, >> >> >> >>> and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, >> >> >> >> I believe "Hack" is not acceptable on this mailing list anymore, >> >> please >> >> remove it from the commit message. >> > >> > Care to point where this was discussed or agreed by the bulk of >> > developers? >> > I don't recall anything about it. >> > A quick search for "hack" shows a lot of recent results in both the ML >> > and >> > the git repo, so I'm not sure why you assumed it's "not acceptable" >> > anymore. >> >> What's a better word? "incorrect workaround"? Am I not allow to say >> some code is wrong? Because the code is defacto incorrect. > > this is not really my area and i might misguess what was meant so > i can only speak about what my feeling woudl suggest > > "Hack" might suggest the author had knowledge of the code being not > the correct solution when adding it. Maybe this felt offensive Come one, I will commit this patch with "hack" word, if nobody beats me.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 1bc3800..54c63ad 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2802,12 +2802,8 @@ static void mov_build_index(MOVContext *mov, AVStream *st) sc->time_offset = start_time - empty_duration; current_dts = -sc->time_offset; if (sc->ctts_count>0 && sc->stts_count>0 && - sc->ctts_data[0].duration / FFMAX(sc->stts_data[0].duration, 1) > 16) { - /* more than 16 frames delay, dts are likely wrong - this happens with files created by iMovie */ - sc->wrong_dts = 1; + sc->ctts_data[0].duration / FFMAX(sc->stts_data[0].duration, 1) > 16) st->codecpar->video_delay = 1; - } } if (!unsupported && st->codecpar->codec_id == AV_CODEC_ID_AAC && start_time > 0) @@ -5352,8 +5348,6 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt) sc->ctts_index++; sc->ctts_sample = 0; } - if (sc->wrong_dts) - pkt->dts = AV_NOPTS_VALUE; } else { int64_t next_dts = (sc->current_sample < st->nb_index_entries) ? st->index_entries[sc->current_sample].timestamp : st->duration;
This breaks files with legitimate single-entry edit lists, and the hack, introduced in f03a081df09f9c4798a17d7e24446ed47924b11b, has no link to any known sample in its commit message, nor does it actually fix the problem properly, but instead has a one-off heuristic to try and "fix" them at the expense of breaking legitimate files. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- Let's get this out of the way first: I'm not 'returning to FFmpeg'. I do not intend to be involved in the community again, nor its overall direction and discussions, short of a miracle occurring at VideoLAN Dev Days. Let's please keep this to technical repleis only. I will ignore anything else. I am sending this patch in a professional context, as it is a problem I have run into at work, with legitimate files, produced by x264 and L-SMASH. Example file: http://chromashift.org/samples/delay_problem.mp4 Having the DTS delay output on the first packet itself is important for things like cutting files. The behavioral change can be seen with: $ ffprobe -show_packets delay_problem.mp4 Lastly, I would appreciate it if any replies to this patch set use 'Reply All', to CC me directly, because I am not currently subscribed to this mailing list, and getting proper IDs to use in the In-Reply-To field isn't easy now that Gmane is dead. Cheers, - Derek --- libavformat/mov.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)