Created attachment 371327 [details] [review]
video: Allow ignoring PTS in the past

Add a new boolean property that tell the video decoder baseclass to simply
ignore PTS set in the past, rather than latching it to the previous PTS.

This is desirable in certain circumstance. One specific case is when the
buffer's PTS is dictated by an external (remote) entity, sending packets
over RTP but sets wrong PTS on certain packets. An RTP jitterbuffer
upstream to the decoder will not help since it will drop such buffers and
that could break the decoder. Instead, the decoder should be told not to
correct the PTS on the buffers in such cases and let the elements
downstream to the decoder decide what to do with the buffer.

Also to keep in mind that the audio decoder baseclass does not attempt to
correct PTS in the past.

This patch also adds a simple testcase for this new property.

A property does not seem nice here. Either this is the correct behaviour or not, or we should implement something more clever in the base class to decide when to behave like this and when not.

Nothing would set properties on elements in autoplugged pipelines.

(In reply to Sebastian Dröge (slomo) from comment #2)
> A property does not seem nice here. Either this is the correct behaviour or
> not, or we should implement something more clever in the base class to
> decide when to behave like this and when not.

I would actually suggest just removing this behaviour. It does not seem like base class should be doing this. If there is a way to be clever about this, I'm all ears.

> Nothing would set properties on elements in autoplugged pipelines.

Subclasses could but they really shouldn't need to.

Also what is the usecase for this correction? Should the decoder really be doing this?

*** Bug 738472 has been marked as a duplicate of this bug. ***

(In reply to Zeeshan Ali (Khattak) from comment #4)
> Also what is the usecase for this correction? Should the decoder really be
> doing this?

Probably not. Edward might remember

We had a bit of discussion on this during the hackfest. Edwards was very reluctant to drop the code in question since if a buffer gets to that point, there is something very wrong happening upstream to the decoder and needs to be somehow addressed. Edward suggested maybe the timestamp should in this case be set to CLOCK_TIME_NONE.

While I, Sebastian and Olivier agree that something needs to be done, we don't think it's the decoder's job to be fixing the timestamp and should let upstream and downstream elements handle past timestamps. Especially since decoder in this case is not really fixing anything.

Sebastian didn't like the idea of setting timestamp to _CLOCK_TIME_NONE since it will break other elements downstream.

We will hopefully have another discussion today about this and have a conclusion.

1) The only thing that should change in gstvideodecoder is to replace the GST_WARNING_OBJECT() by a GST_ELEMENT_WARNING() so the application/user is properly informed

2) Upstream elements should detect completely bogus PTS (like 5 years in the future) and set GST_CLOCK_TIME_NONE instead.

(In reply to Edward Hervey from comment #8)
> 1) The only thing that should change in gstvideodecoder is to replace the
> GST_WARNING_OBJECT() by a GST_ELEMENT_WARNING() so the application/user is
> properly informed

One thing that is still unanswered is why is decoder fixing the PTS? Does it solve any problem? If the rationale is for the problem of insane PTS to be not ignored, than it should send an error instead of emitting a warning perhaps?

-- 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-base/issues/440.