[FFmpeg-devel] [PATCH 4/5] Prefer pts over dts in timestamp correction

[Alexander Strange]

On Sep 13, 2010, at 5:05 PM, Michael Niedermayer wrote:

> On Sun, Sep 12, 2010 at 11:06:34PM -0400, Alexander Strange wrote:
> [...]
>> - When ffmpeg.c is flushing output pictures it generates fake dts by
>> adding one timebase unit to next_pts. In that mp4 file, that seems to
>> be the wrong unit; the dts starts incrementing by 5 at the end where
>> all the real times increment by 30000 or so. With this patch, it works
>> after the patch to ffmpeg.c is applied.
>> An alternate fix would be to just ignore next_pts when flushing output
>> pictures, or a better approximation (r_frame_rate?) for next_pts.
> 
> if next_pts values are off by such huge amount then this smells like a
> bug and this should be fixed (being independant of this thread)
> also next_pts is used when all other things are missing so its important
> that it is reasonable

Yes, definitely. Using avg_frame_rate looks reasonable, I just need to try some other files.

https://roundup.ffmpeg.org/issue2252

> 
> 
>> - IIRC when playing an h264 PAFF file with field packets, if dts is
>> used then the timestamp of the second field is used for the output
>> picture instead of the first field. I can't find a sample for that,
>> though.
> 
> there was a mpeg2 problem like this i dont remember that for paff
> 
> about dts shifts, the following things should be done:
> 1. our h264 decoder should when the delay it uses differs from the spec
>   delay, export this value in AVCodecContext. This difference already occurs
>   so we need this independant of -mt

I'll try to look at it.

> 2. our demuxers should be tested for returned dts being correct, formats like
>   mpeg-ts that litterally store dts can be used as reference after making
>   sure we dont modify dts by mistake somewhere.

'ffprobe_g -show_packets' vs. -fflags +nofillin+noparse is good for this.
Doesn't test decoders though. 

> 3. in cases where dts are not known either due to lack of the needed
>   code or lack of knowledge of how to do it they should be set to
>   AV_NOPTS_VALUE.

Well, that knowledge might belong in libavcodec, which can't change dts values.

> implementing 3 alone fixes the issue. (litterally a if(h264) dts=none or a
> if(h264) always use pts until our h264 timestamping is fully implemented)
> Changing between < and <= just shifts a threshold and a single damaged pts
> timestamp would make that insufficient thus it appears very fragile to me.
> Though ultimately i dont care at all if we have < or <= there it does no
> harm (so i wont stop you commiting that change) but it does not fix anything

I think it's still a good small change even though it clearly doesn't fix a lot of things.
In particular, looking at ffmpeg.c, next_pts can't be right for VFR files.
But right now ffmpeg always uses it to assign times to frames being flushed at the end of decode.
This patch + using the function in ffmpeg (next patch) would fix it.

Applied with commit message made more humble.