Opened 2 years ago

Closed 6 months ago

#8620 closed defect (fixed)

Fundamental bug in DirectShow capture - wrong timestamps

Reported by: Dmitry Sinitsyn Owned by:
Priority: normal Component: avdevice
Version: unspecified Keywords: dshow dts
Cc: Diederick Niehorster Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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.

Attachments (1)

manualCapture.log (122.4 KB ) - added by Dmitry Sinitsyn 2 years ago.

Download all attachments as: .zip

Change History (28)

by Dmitry Sinitsyn, 2 years ago

Attachment: manualCapture.log added

comment:1 by Dmitry Sinitsyn, 2 years ago

Keywords: pts added

comment:2 by Carl Eugen Hoyos, 2 years ago

Keywords: dshow dts added; dshow_pin.c pts removed
Priority: criticalnormal
Version: 4.2unspecified

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.

comment:3 by Dmitry Sinitsyn, 2 years ago

Ticket valid for current git head too

Last edited 2 years ago by Dmitry Sinitsyn (previous) (diff)

comment:4 by Dmitry Sinitsyn, 2 years ago

Patch sent to FFmpeg-devel

comment:5 by Diederick Niehorster, 13 months ago

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

comment:6 by Diederick Niehorster, 13 months ago

Cc: Diederick Niehorster added

comment:8 by Balling, 13 months ago

Status: newopen

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

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

comment:9 by Dmitry Sinitsyn, 13 months ago

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

in reply to:  9 comment:10 by Balling, 13 months ago

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.

comment:11 by Dmitry Sinitsyn, 13 months ago

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?

Last edited 13 months ago by Dmitry Sinitsyn (previous) (diff)

in reply to:  11 ; comment:12 by Balling, 13 months ago

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.

Last edited 13 months ago by Balling (previous) (diff)

comment:13 by Diederick Niehorster, 13 months ago

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

in reply to:  12 comment:14 by Dmitry Sinitsyn, 13 months ago

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.

in reply to:  13 ; comment:15 by Dmitry Sinitsyn, 13 months ago

Replying to Diederick Niehorster:

I've submitted a patch

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.

Last edited 13 months ago by Dmitry Sinitsyn (previous) (diff)

in reply to:  15 ; comment:16 by Diederick Niehorster, 13 months ago

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);
?

in reply to:  16 ; comment:17 by Dmitry Sinitsyn, 13 months ago

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

comment:18 by Diederick Niehorster, 13 months ago

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

in reply to:  17 ; comment:19 by Balling, 13 months ago

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?

in reply to:  19 ; comment:20 by Dmitry Sinitsyn, 13 months ago

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.

Last edited 13 months ago by Dmitry Sinitsyn (previous) (diff)

in reply to:  20 ; comment:21 by Balling, 13 months ago

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?

in reply to:  21 comment:22 by Dmitry Sinitsyn, 13 months ago

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.

comment:23 by Diederick Niehorster, 13 months ago

in reply to:  23 comment:24 by Dmitry Sinitsyn, 13 months ago

comment:25 by Dmitry Sinitsyn, 10 months ago

@Diederick Niehorster,
any progress?

comment:26 by Diederick Niehorster, 10 months ago

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.

comment:27 by Diederick Niehorster, 6 months ago

Resolution: fixed
Status: openclosed

The patch was pushed as 7dc33aad..271e5598

Note: See TracTickets for help on using tickets.