Fundamental bug in DirectShow capture - wrong timestamps

[Dmitry Sinitsyn]

Summary of the bug:
File dshow_pin.c contains lines:

    if (devtype == VideoDevice) {
        /* PTS from video devices is unreliable. */
        IReferenceClock_GetTime(clock, &curtime);
    } else {
        IMediaSample_GetTime(sample, &curtime, &dummy);

As we can see for video stream this code uses current reference clock as PTS for video sample, while for audio sample it uses correct stream start time as PTS.
I do not know why developer thought that "PTS from video devices is unreliable" but nowadays my experience makes me sure that we should use sample stream start time as PTS for all types of streams.
I patched this code, tested and it works fine this way:

if (0 /*devtype == VideoDevice*/) {

You can see good and wrong timestamps using this run:

% ffmpeg -v verbose -debug_ts -f dshow -pixel_format yuyv422 -video_size 1920x1080 -framerate 30 -i "video=Logitech BRIO:audio=Микрофон (Logitech BRIO)" -y d:\Transcoding\manualCapture.mp4 > d:\Transcoding\manualCapture.log 2>&1

Look at "dshow passing through packet of type video" lines:

dshow passing through packet of type video size  4147200 timestamp 63723140000 orig timestamp 63723136716 graph timestamp 63723140000 diff 3284 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63723470000 orig timestamp 63723470049 graph timestamp 63723470000 diff -49 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63723790000 orig timestamp 63723803382 graph timestamp 63723790000 diff -13382 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63724260000 orig timestamp 63724136715 graph timestamp 63724260000 diff 123285 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63724580000 orig timestamp 63724470048 graph timestamp 63724580000 diff 109952 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63724900000 orig timestamp 63724803381 graph timestamp 63724900000 diff 96619 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63725220000 orig timestamp 63725136714 graph timestamp 63725220000 diff 83286 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63725540000 orig timestamp 63725470047 graph timestamp 63725540000 diff 69953 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63725860000 orig timestamp 63725803380 graph timestamp 63725860000 diff 56620 Logitech BRIO
dshow passing through packet of type video size  4147200 timestamp 63726180000 orig timestamp 63726136713 graph timestamp 63726180000 diff 43287 Logitech BRIO

Good stream times timestamps are "orig timestamp"
Bad currently used are "timestamp".
While diffs between "orig timestamps" are perfectly 333333 (as we have 30 fps) diffs between "timestamps" varies which is not surprising me.

FFmpeg itself usually successfully synchronizes such streams but for those who uses libavdevice internally this defect adds unnecessary difficulties.

[Carl Eugen Hoyos]

Please test current FFmpeg git head to make this a valid ticket.

If you have a patch to fix this issue, please send it - made with git format-patch - to the FFmpeg development mailing list.

[Dmitry Sinitsyn]

Ticket valid for current git head too

[Dmitry Sinitsyn]

Patch sent to FFmpeg-devel

[Diederick Niehorster]

I am interested in this issue too. Can you provide a link to your patch? Couldn't find it.

[Dmitry Sinitsyn]

Here is it:
​https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200605120239.268-1-user@Daewoo/

[Balling]

Did you figure out ​https://github.com/FFmpeg/FFmpeg/commit/b76a0e24f9effa64e48ff0567af0dc497dd99e84?

Also I think after you figure it out you can switch the default.

[Dmitry Sinitsyn]

I looked at the commit. It does not relate to video PTS.
Sorry I do not understand what did you mean.

[Balling]

Replying to Dmitry Sinitsyn:

I looked at the commit. It does not relate to video PTS.
Sorry I do not understand what did you mean.

But that IS PTS presentation timetsamp. He says that some PTS like 437650244077016960 which is negative are possible.

[Dmitry Sinitsyn]

What do you want to say?
I'm talking about video PTS calculation:
​https://github.com/FFmpeg/FFmpeg/blob/575e52272d42f4278c6620f1a999c41425db2094/libavdevice/dshow_pin.c#L315-L319

Here we correctly get audio PTS by using IMediaSample_GetTime and create FAKE video PTS.
Why?

[Balling]

Replying to Dmitry Sinitsyn:

Why?

Because it is unreliable? I mean see #8454.
fps=60.0002, fps=59.9402, fps=240.004, and also see #6279 fps=59.9999?? What? Those all should have been rounded to 3 decimals, I suppose, but whatever.

[Diederick Niehorster]

I've submitted a patch for review, https://ffmpeg.org/pipermail/ffmpeg-devel/2021-June/280879.html. I too have a device that provides fine timestamps. I have not changed the default, but made a comment about that it should be discussed in the commit message. Lets see what the list says

[Dmitry Sinitsyn]

Replying to Balling:

Because it is unreliable? I mean see #8454.

No. those numbers does not relate to Media Sample PTS.

IMediaSample::GetTime returns stream times at which this sample should begin and finish.
There are two available cases: times are good or times are absent.
Whole DirectShow based on these timestamps.
So I'm still against of ignoring video timestamps in that place of libav.
We must get timestamps directly by IMediaSample::GetTime.
May be we should be able to revert to old way, but normal way should be:
IMediaSample::GetTime for all samples, if time==NULL, then use reference clock.

[Dmitry Sinitsyn]

Replying to Diederick Niehorster:

I've submitted a patch

I submitted my patch a year ago without any reaction. Hope your will have it.

[Diederick Niehorster]

Replying to Dmitry Sinitsyn:

I submitted my patch ​http://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/263999.html a year ago without any reaction. Hope your will have it.

Ah, there ya go. Thats pretty much the same patch except that you have default on and i have default off :)

normal way should be:

IMediaSample::GetTime for all samples, if time==NULL, then use reference clock.

Ah, so is there a case missin? something like:

IMediaSample_GetTime(sample, &curtime, &dummy);
if (curtime == 0)
    curtime = graphtime;

where graphtime is set by IReferenceClock_GetTime(clock, &graphtime);
?

[Dmitry Sinitsyn]

Replying to Diederick Niehorster:

Ah, so is there a case missin? something like:

Yes. I think so now. That patch was my first attempt to make ffmpeg better. And I did not want to change it a lot.
Today I think it would be nice to add this case processing

[Diederick Niehorster]

Ok, lets be bold, i've send some more patches to the list to catch that

[Balling]

Replying to Dmitry Sinitsyn:

Replying to Diederick Niehorster:

Ah, so is there a case missin? something like:

Yes. I think so now. That patch was my first attempt to make ffmpeg

Okay, but you still do not know what to do with big initial negative PTS?

those numbers do not relate to Media Sample PTS.

Okay, but after that there will be two different ways to calculate framerate, is not it?

[Dmitry Sinitsyn]

Replying to Balling:

Replying to Dmitry Sinitsyn:

Replying to Diederick Niehorster:

Ah, so is there a case missin? something like:

Yes. I think so now. That patch was my first attempt to make ffmpeg

Okay, but you still do not know what to do with big initial negative PTS?

I've never seen "big initial negative PTS". But current workaround for this case is OK for me.

those numbers do not relate to Media Sample PTS.

Okay, but after that there will be two different ways to calculate framerate, is not it?

I still do not understand why do you mention about framerate here. It is from a different place - from stream info.
Timestamp generated by DirectShow source filter for each its media sample.

[Balling]

Replying to Dmitry Sinitsyn:

Timestamp generated by DirectShow source filter for each its media sample.

Do you know what Variable Framerate is? It is when time difference between frames are not the same. Now, the fps reported from stream info means constant framerate. After PTS will be read, the framerate can be no longer constant. You can record stream and check for yourself with

ffmpeg -i video -vf vfrdet -an -f null -

But current workaround for this case is OK for me.

Well, there may be some meaning to those PTS. Like some kind of SMPTE timecode or something like that. Metadata?

[Dmitry Sinitsyn]

Replying to Balling:

Do you know what Variable Framerate is?

Sure I know. Why don't you answer my question? We are talking about libav PTS acquiring from DirectShow. Why are you talking about framerate?
Correct PTS is especially important for variable framerate. And upstream filter knows this(must know) so it assigns correct timestamps to media samples. Thus we should just get them.

But current workaround for this case is OK for me.

Well, there may be some meaning to those PTS. Like some kind of SMPTE timecode or something like that. Metadata?

Most likely it is a bad capture filter. Timestamp can be valid or absent.

[Diederick Niehorster]

@Dmitry, does the patch look good to you?
​https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210602132230.2380-4-dcnieho@gmail.com/

[Dmitry Sinitsyn]

Replying to Diederick Niehorster:

@Dmitry, does the patch look good to you?
​https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210602132230.2380-4-dcnieho@gmail.com/

Yes. Good.

[Dmitry Sinitsyn]

@Diederick Niehorster,
any progress?

[Diederick Niehorster]

I'm afraid not, the patch is part of a series that requires some discussion in the project about where they want to take avdevice it seems. But since a new version is far away anyway, i guess there is time.

[Diederick Niehorster]

The patch was pushed as 7dc33aad..271e5598