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

[Martin Storsjö]

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. 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.

> 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.

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