Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
On 8/25/16, Michael Niedermayerwrote: > 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 >> >> : >> >>> 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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: > >>> 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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: 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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: >>> 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
On 8/25/2016 12:45 PM, Carl Eugen Hoyos wrote: > 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis: >> 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are best at talking, realize last or never when they are wrong. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
2016-08-24 16:55 GMT+02:00 Derek Buitenhuis: > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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> --- > 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
2016-08-24 16:55 GMT+02:00 Derek Buitenhuis: > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mov: Remove ancient heuristic hack
Hi Derek! 2016-08-24 16:55 GMT+02:00 Derek Buitenhuis: > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel