GNOME Bugzilla – Bug 705066
mpegvideoparse: fix timestamps
Last modified: 2018-11-03 13:17:21 UTC
Created attachment 250353 [details] Filter that fixes mpeg timestamps Mpegvideoparse currently doesn't do anything about fixing broken / missing timestamps. This affects playback for many files in my case. I'm currently using custom filter after mpegvideoparse to do this, but perhaps the functionality should be added to the parser? If someone could look at the code and confirm I can provide a patch. Although unlike my filter mpegvideoparser probably can't get away with throwing away D frames so this will need to be addressed. Does anyone have a stream with D frames?
Funny thing, I just updated the patch to do just that at bug #632222.. Well, if you use the second patch and use "retimestamp=TRUE", but it's still not perfect. I think that in the general case, one should use the timestamps from the upper layer instead of from inside the MPEG2-ES stream, if the upper layers provide useful timestamps. *** This bug has been marked as a duplicate of bug 632222 ***
This patch really works differently than yours. I calculate the timestamps by figuring out the presentation order of the frames and then interpolating the time. It should work for all cases.
Also your patch doesn't seem to deal with with frame reordering at all. I don't take the timestamp from GOP header, it should come from the demuxer. But the timestamps from demuxer are often jumpy or missing (usually for reordered frames), that's what needs to be fixed.
*** This bug has been marked as a duplicate of bug 632222 ***
I'm trying to address the case where the MPEG2-ES stream is transmitted as-is (not muxed), that's why I read the timestamps from the GOP. I would generally assume that the timestamps from the demuxer, etc are better. For example, rtpjitterbuffer and tsdemux apply clock skew correction (for the live playback case). And by using the temporal reference number (TSN), you don't have to worry about decoding-vs-presentation order, the TSN are presentation times. Only adding up the durations, which was my initial approach, means that you have to keep up with P-frames, etc. Also means it won't work if you have any losses
Actually, I spoke to soon. I see you take the tsn from picheader, I used to do the same thing before but had bad experience with it for some files.
Also simply calculating PTS using TSN * duration_from_framerate will not work for telecine.
Anyway, regardless of how the timestamps are determined, the question is whether the parser is allowed to change timestamps at all? Can it fill missing timestamps? What if there are already timestamps from upstream but are not perfect? (i.e. I've seen wrong timestamps on reordered frames)
I think I might be overly pessimistic about the container timestamps. I changed the code to just fill in empty timestamps (don't change container timestamps) and have no problem with my test videos so far.
Created attachment 250381 [details] Filter that fixes mpeg timestamps Updated version. Does not overwrite demuxer timestamps (only fills in missing ones). Does not introduce latency if timestamps are present on input buffers.
Created attachment 250385 [details] Updated filter Updated version. Does not overwrite demuxer timestamps (only fills in missing ones). Does not introduce latency if timestamps are present on input buffers.
I think it's quite unlikely this would go in as a separate element considering we already have mpegvideoparse :( Can you make it a patch against current mpegvideoparse from git ?
Created attachment 250439 [details] Updated filter Fixed some problems from last iteration.
(In reply to comment #12) > I think it's quite unlikely this would go in as a separate element considering > we already have mpegvideoparse :( Can you make it a patch against current > mpegvideoparse from git ? The issue is, that at this point I'm not entirely sure what the videoparse needs to be doing. Having as separate filter makes it much easier to test. Once the filter does what parser should be doing (which I'm still not sure what that is - will explain in next comment) I can make it a patch for mpegvideoparse.
So currently the way the filter works it only adds missing timestamps, doesn't touch timestamps that come from upstream. This works well for most files (where the timestamps from upstream are sparse, but correct). Some files are not so nice however. For example https://s3.amazonaws.com/MatejK/Samples/V.VOB It has only timestamps on i-frames (which is usual). The timestamp of second iFrame is 0:28:19.420000000 But the actual presentation time (when calculated from all frames that are displayed before it) is 0:28:19.940000000 In current state, the filter ignores the calculated value and just uses the one from upstream. This will of course cause errors during playback, as the timestamps jump back. Now without the filter (where the timestamps are sparse), libav just ignores the timestamps and goes on, so the playback works. With the filter however, when all timesamps are set, libav honors the timestamps so the playback is actually worse. I need to check the demuxer to see if the timestamps it gives are correct, but for now I assume they are. I don't see any solution for this. wtay says that the parser should respect upstream timestamps, but when we do, the file is unplayable (even though it plays in other players, as they obviously ignore those, at least when the TS are going backwards)
Nevermind, the file is good. mpegvideoparse doesn't support field pictures :(
Disregard the comment about V.VOB, it was a false alarm. mpegtimestampparse was outputing twice as many buffers than it should have ( https://bugzilla.gnome.org/show_bug.cgi?id=705144 )
I meant mpegvideoparse of course.
Ok, so what should happen here now? What's left to be done, how does that relate to the other mpegvideoparse timestamp bug?
Well, someone should take the code from the filter attached here and put it to mpegvideoparse. That someone probably being me. I have one issue with this though, I need access to queued pages in baseparser, but there is no easy way to do that currently. As for the other issue, there is partial overlap (interpolating of missing timestamps), not sure about the rest (using timestamps from seq. header, etc).
Ok, feel free to add whatever API is needed to baseparse :)
@Matej Do you plan to put the code inside mpegvideparse ?
I really don't think I'm going to be able to do that in foreseeable future.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/105.