| Message ID | 20210809142342.153566-1-sehuww@mail.scut.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/movenc: Don't auto flush fragment if no frame available | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
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@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
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@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
On Tue, 10 Aug 2021, 胡玮文 wrote:
> Thank you for your detailed explaination. Now I agree your patch is better.
Ok, I pushed that one then.
// Martin
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 57062f45c57..72fe8df12c2 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -5929,7 +5929,7 @@ static int mov_write_single_packet(AVFormatContext *s, AVPacket *pkt) (mov->flags & FF_MOV_FLAG_FRAG_KEYFRAME && par->codec_type == AVMEDIA_TYPE_VIDEO && trk->entry && pkt->flags & AV_PKT_FLAG_KEY) || - (mov->flags & FF_MOV_FLAG_FRAG_EVERY_FRAME)) { + (mov->flags & FF_MOV_FLAG_FRAG_EVERY_FRAME && trk->entry)) { if (frag_duration >= mov->min_fragment_duration) { // Set the duration of this track to line up with the next // sample in this track. This avoids relying on AVPacket
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@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. 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. [1]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210805123421.10527-1-martin@martin.st/ [2]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210718102232.1382376-1-sehuww@mail.scut.edu.cn/ libavformat/movenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)