Review of attachment 321528 [details] [review]:

Are you sure this shouldn't be fixed in collect pad ? The only use for this generic clip function is for muxers.

(In reply to Nicolas Dufresne (stormer) from comment #1)
> Review of attachment 321528 [details] [review] [review]:
> 
> Are you sure this shouldn't be fixed in collect pad ? The only use for this
> generic clip function is for muxers.

I would not know to be honest. We only build flvmux out of all the muxers, so we would not be able to detect any issues that might arise from such a change.

Collectpads are the most evil thing in GStreamer bye the way, and should be removed as soon as possible! :)

I'm well aware, I'll have a look, it would be unfortunate to create an inconsistency between muxers here.

Also let's take a look at what GstAggregator would be doing here... as the upcoming replacement for collectpads.

Any comments or other news here? Is it a regression?

(In reply to Sebastian Dröge (slomo) from comment #5)
> Any comments or other news here? Is it a regression?

For us it was a huge regression coming from 0.10. YouTube and Flash stopped working due to timestamps going negative and hence entering into the RTMP extended timestamp-domain, messing up pretty much any RTMP implementation.

0.10 is quite old :) I won't consider this a blocker for 1.8.0 then, it can still be fixed for 1.8.1.

Unless Nicolas changes his opinion, the patch would have to be updated. If that happens fast enough we can consider this for 1.8.0, otherwise not.

I do assume your change is correct even though I laked the time to dig into it. There is no evidence that this issue does not affect all muxers using this same clipping function, so I can only conclude this is patching at the wrong place (plus it's a huge copy paste). My opinion still stands and it would be easier if the author would take care to update his patch to be applied on GstCollectPad, if not, I can only assume this is not urgent.

(In reply to Håvard Graff (hgr) from comment #6)
> (In reply to Sebastian Dröge (slomo) from comment #5)
> > Any comments or other news here? Is it a regression?
> 
> For us it was a huge regression coming from 0.10. YouTube and Flash stopped
> working due to timestamps going negative and hence entering into the RTMP
> extended timestamp-domain, messing up pretty much any RTMP implementation.

Can you explain, or document, few ways to get a stream without PTS ? This is fairly uncommon. When PTS is missing, it means we don't know the CTS. Best we can do is assume it's zero (like a H264 stream without B-Frames). Assuming that CTS is 0 means that we assume PTS == DTS, and clipping on either PTS or DTS shall make no difference. This was obviously the case in 0.10, as there was only GST_BUFFER_TIMESSTAMP iirc.

Review of attachment 321528 [details] [review]:

Ok, digging a bit, clipping on DTS is always wrong. DTS can be outside of the segment and that's completly valid. This would break this simple pipeline:

  videotestsrc ! x264enc ! flvmux

Created attachment 322937 [details] [review]
collectpads: Assume PTS is equal DTS if PTS is missing

This is the best guess we can make if such a buffer reached the collect
pad. This is uncommon, we do expect parsers to have tried and fixed that
if possible (or needed).

Comment on attachment 322937 [details] [review]
collectpads: Assume PTS is equal DTS if PTS is missing

I didn't test this one, but I believe it's a better workaround to stream missing PTS but having DTS. This requires a unit test of course, I just wrote that quickly to propose an alternative solution.

Håvard, does that also work for your cases?

Let's consider this as a blocker for now and see what we can do here. Might reconsider ignoring this for 1.8.0 later

(In reply to Sebastian Dröge (slomo) from comment #13)
> Håvard, does that also work for your cases?

The supplied test captures exactly what does not work for us. If test passes, we are happy! :)

(In reply to Håvard Graff (hgr) from comment #15)
> (In reply to Sebastian Dröge (slomo) from comment #13)
> > Håvard, does that also work for your cases?
> 
> The supplied test captures exactly what does not work for us. If test
> passes, we are happy! :)

If would be kind to submit a patch with the unit test split from the fix.

Created attachment 323070 [details] [review]
test only

The current suggestion by Nicolas is very satisfactory for us! (and test passes!)

Comment on attachment 322937 [details] [review]
collectpads: Assume PTS is equal DTS if PTS is missing

As Tim stated on IRC, this is not trivially obvious. We can give it some time and merge for 1.8.1.

One more question: is that pts=dts then passed to the muxer as pts, or is it just used in collectpads for collectpads purposes?

Demoting, not a blocker for 1.8.0

(In reply to Tim-Philipp Müller from comment #20)
> One more question: is that pts=dts then passed to the muxer as pts, or is it
> just used in collectpads for collectpads purposes?

It is passed to the demuxer. Otherwise all demuxers will have to do the same (some already do that). A stream without PTS is not really valid, unless PTS == DTS. I prefer to define it this way than to fail if PTS is missing. It's also what is generally assumed by elements producing this.

Attachment 322937 [details] pushed as 9b0d42c - collectpads: Assume PTS is equal DTS if PTS is missing

Comment on attachment 323070 [details] [review]
test only

commit 2b8b5f22465850b306b93c8317ac0f4ff10ca432
Author: David Buchmann <david.buchmann@gmail.com>
Date:   Fri Mar 4 09:42:44 2016 +0100

    flvmux: Test to verify flvmux handles DTS with GST_CLOCK_TIME NONE
    
    https://bugzilla.gnome.org/show_bug.cgi?id=762207

This actually does not seem to do the right thing in general, and we should probably defer this to the muxers as only they can know if doing this assumption is ok for their use case or not.

Problematic for example is if you have a stream that has B frames but only DTS. We would then generate a stream that has wrong PTS. And a stream with only DTS but no PTS is perfectly valid, a decoder/parser can make up sensible PTS for it.

The other problematic case is if there is a stream that sometimes does not have PTS (e.g. only for the very first buffer) but always DTS. We would make up PTS that might be completely off based on the actual PTS that are known.


I think this should be reverted and replaced with either proper PTS/DTS conversion in h264parse, or muxer specific solutions if there's something a muxer can do about it.

(In reply to Sebastian Dröge (slomo) from comment #25)
> This actually does not seem to do the right thing in general, and we should
> probably defer this to the muxers as only they can know if doing this
> assumption is ok for their use case or not.
> 
> Problematic for example is if you have a stream that has B frames but only
> DTS. We would then generate a stream that has wrong PTS. And a stream with
> only DTS but no PTS is perfectly valid, a decoder/parser can make up
> sensible PTS for it.
> 
> The other problematic case is if there is a stream that sometimes does not
> have PTS (e.g. only for the very first buffer) but always DTS. We would make
> up PTS that might be completely off based on the actual PTS that are known.
> 
> 
> I think this should be reverted and replaced with either proper PTS/DTS
> conversion in h264parse, or muxer specific solutions if there's something a
> muxer can do about it.

I'm not fully convinced by the argument. First, CollectPad does not send buffers to parsers or a decoders. For encoded data, it's an endpoint before encapsulation of the timestamp information. If some of the timestamp information is missing at this point, it means there is no parser or decoder that where capable to figuring-out (Matroska demux as a source is a valid case). You only have two choices left, make a guess or fail. The in-between is what we had before, which results in worst case scenario (very broken mux). From my knowledge, this guess applies at least to FLV and QT. The only muxer I really know it does not apply, is Matroska, which will use the DTS anyway (in which case this guess has not side effect). For QT, this guess is already found in the muxer itself. I don't mind if we go back to hacking FLV muxer, but I do think it's unfortunate.

About the first timestamp hack, I still believe this hack should not have existed. While its easy for sink, it's far from elegant to any other elements that require timestamp information.

I made the same argument as Sebastian on irc to hgr as well.

I think this kind of guessing should be made explicit in the muxer.

I think mp4mux (maybe also flvmux) should reject input that has DTS set, but not PTS. I don't think we should create broken files by making up possibly wrong PTS (which is what we *might* be doing here afaict, if the input has B-frames).

I think h264parse should do this fix-up for streams where DTS is set, but not PTS (it knows whether there are B-frames or not).

This would still make Håvards low-latency-streaming/no-B-frames case work.

For other cases, we would error out and someone would have to make h264parse do proper parsing to fix this up (see bug that exists for that), which is of course non-trivial.

Just to recap, this means a stream with DTS but no PTS is NOT valid for FLV and QT muxer (at least), and should result in error. That will likely cause few regression. Håvards unit test need to be reverted, or at least it should be checking that such stream cause an error, not a valid FLV. Håvards will also need to figure-out why he got buffer with DTS and no PTS while in his case. Would be nice to know if this is from a prorietary element or an element in GStreamer.

Had some more discussion on IRC. A possible plan could be:

In 1.8, move the two lines patch into flvmux, keep the unit test as-is
In 1.9, implement setting PTS for the non-b-frame case in h264parse (and other parses where it's evident that PTS and DTS are equal), error out in flvdemux (and qtdemux) if PTS is missing, and add an h264parse element to the unit test.

Any thought ?

This plan sounds like a good idea. Also note that hopefully for 1.10 finally all the muxers are using aggregator too.

Nicolas, any news here? What's the status?

I didn't progress on this one. It's not hard to implement, Which one is to hurry ? The 1.8 or the 1.9 part ?

1.8 and then 1.9 :) Thanks!

Created attachment 329398 [details] [review]
flvmux: Assume PTS is DTS when PTS is missing

This fixes issue for encoders that only sets the DTS. We assume that
there was no re-ordering when that happens.

Created attachment 329399 [details] [review]
flvmux: Test to verify flvmux handles DTS with GST_CLOCK_TIME NONE

Those are for 1.8. What is annoying is that the unit test is not valid any-more for the new plan for 1.9 (do that PTS/DTS thing in parser, and fail otherwise).

So what *is* the plan here now? :) Is there anything we can do for 1.10, anything we should do? Is it all too late anyway and things work well enough?

I'd propose to take those two patches, which fixes the original issue (flv only) and merge that. You did the same for QT recently.

Go ahead then

Comment on attachment 329399 [details] [review]
flvmux: Test to verify flvmux handles DTS with GST_CLOCK_TIME NONE

That's already in 1.9

Attachment 329398 [details] pushed as ad9e9be - flvmux: Assume PTS is DTS when PTS is missing