[FFmpeg-devel] [PATCH] avformat/movenc: Don't auto flush fragment if no frame available

[胡玮文]

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 <sehuww at 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