Thank you for your detailed explaination. Now I agree your patch is better.
On Mon, Aug 09, 2021 at 09:36:12PM +0300, Martin Storsjö wrote: > Hi, > > On Mon, 9 Aug 2021, Hu Weiwen wrote: > > > Even if FF_MOV_FLAG_FRAG_EVERY_FRAME is set, don't flush if no frame > > available. > > > > This fixes an issue that we overwrite the track duration, causing it to be > > out-of-sync with the last written packet in previous fragment. > > > > Signed-off-by: Hu Weiwen <seh...@mail.scut.edu.cn> > > --- > > Hi Martin, > > > > I can confirm your patch "movenc: Don't try to fix the fragment end > > duration if none will be written"[1] does fix my issue reported in [2]. > > But I think my current patch would be a better fix. It is more > > self-explanatory, and more consistent in the case of > > FF_MOV_FLAG_FRAG_KEYFRAME. > > This patch is indeed more straightforward, but it misses a couple cases. If > the current muxed packet is a keyframe, but we have no queued packets in > this track, we'd end up with the same bug again. I don't get this. We already checked trk->entry in the FF_MOV_FLAG_FRAG_KEYFRAME case. > And the other way around, > if we'd have a packet queued in another track, we won't flush that fragment > right here, which is a notable behaviour change from how > FF_MOV_FLAG_FRAG_EVERY_FRAME behaves right now. I agree. > > Also, I think my original patch [2] still has its value. "frag_start" > > seems to be redundant, and it is only updated relative to its previous > > value. This is bad because any potential error updating it will have > > infinite impact on future packets. > > I disagree here. If we have a bug where we update things making them > inconsistent, we should fix that bug. These other patches that are discussed > is one case of that. > > This code is quite complex and it's almost a decade since I wrote most of > it, so I don't recall offhand exactly how it behaves in all cases - but it's > designed to try to make the output consistent and correct for a number of > weird cases. > > If the variable really is strictly redundant, you can send a patch which can > be proven to not alter behaviour anywhere under any circumstances. But if > it's made with the intent to be more robust, then it's also a possible > behaviour change, and in that case I prefer not changing behaviour blindly. I agree. I will try to prove. > But separately, as I said, I'm totally open to a patch to add an option to > make it stop adjusting the start of the next fragment (making tfdt the > authoritative source, possibly differing from the sum of earlier durations). > > // Martin _______________________________________________ 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".
