Draft: Clock context to handle PTS/PCR discontinuity

Cf. discussion during Workshop 2021-12-06/07 #26378 (closed)

The issue

The goal is to keep the data related to the PTS as long as possible inside the playback workflow. To do that, the clock internal state needs to be kept through the whole pipeline, and not scratched synchronously.

As an example scenario: during a media player playback:

• a change in the clock (discontinuity) is performed
• currently, the clock internal state is reset, so every picture that has been decoded and not displayed has now an invalid PTS (because the PTS cannot be converted to the system time anymore). So they are discarded, or the output might wait a huge amount of time before displaying them (depending if the new PTS increase or decrease)
• however, if we could keep the current state for these pictures (and not for the future ones), we could be able to display them.

Sample with a PCR discontinuity: https://streams.videolan.org/issues/26378/intentional_pcr_discontinuity-short.ts

The solution

The es_out can trigger the creation of a new clock_context via vlc_clock_main_SetFirstPCR. For now, it is only triggered when the input_clock detect a discontinuity, but we could also let demux modules send a new es_out control to trigger it.

The clock_id will be propagated from the main clock owned by the es_out to the output modules, held by block_t or picture_t , and passed from packetizers to decoders to filters to outputs

TODO

It's a draft, if the solution is accepted, I will have to :

• Fix all decoders to forward the clock_id
• Fix all packetizers to forward the clock_id
• Handle clock_id in subpicture_t and spu renderer
• Fix some aout modules to send PTS directly from aout_TimingReport (and not a ts based on 0)
• Fix some aout modules to send the clock_id from aout_TimingReport

changed milestone to %4.0

added Component::Core: Clock label

changed title from Draft: Clock contex to handle PTS/PCR discontinuity to Draft: Clock context to handle PTS/PCR discontinuity

write only variable

Not a big fan of extending the whole frame structure for a fringe case. Why not use ancillaries and only attach them to the blocks starting a new timeline. Just like a new SPS for a new sequence.

It would also prevent doing the same for pictures.

Missing ModuleThread_GetDisplayDate

If it's set by the core, it should probably be set after pf_packetize.

I don't understand this substraction.

It's a stitch between the old pts base and the new pts. We subtract the amount of written samples to the new time base because the timing report will continue to increase and include this amount of written samples.

I struggle to understand the actual problem you are trying to fix TBH. Care to share an example?

For now a PCR discontinuity or a seek will trigger a flush in the core anyway.

I struggle to understand the actual problem you are trying to fix TBH. Care to share an example?

I just added in the MR description: https://streams.videolan.org/issues/26378/intentional_pcr_discontinuity-short.ts

PCR discontinuity at 16sec. Works with VLC 3.0. Video "freeze" in VLC 4.0 (since the vout will wait for an invalid date)

For now a PCR discontinuity or a seek will trigger a flush in the core anyway.

A seek, yes, but not a PCR discontinuity. The demux can set the block flags as DISCONTINUITY, but it won't be handled by the clock or the es_out.

We don't want to flush in that case since there are no input discontinuity, the video and audio decoders can continue to decoder (and we don't want to lose output frames).

I think @fcartegnie has some other valid use case.

In general terms, this MR fix the fact that clock changes are currently synchronous whereas the pipeline is asynchronously using it.

One alternative could have been to handle a sort drain midstream, but it is not possible (and will never be) since:

• You need to flush after a drain (since most decoders and output can't handle a resume from a drain). Therefore, you might lose some context and will need to send a key frame first.
• Re-buffering midstream after a drain, not impossible but we don't want to handle that

or now a PCR discontinuity or a seek will trigger a flush in the core anyway.

only RESET_PCR does

changed the description

TBH, I don't understand why we need a new clock ID, and I have the same prejudice against adding a field in so many objects for a corner case. But if we really need this, I would expect a reference counted object rather than an integer.

AFAICT, if we want to retain timestamps from before a clock reset after clock reset, it is necessary and sufficient to ensure that the timestamp ranges across resets do not overlap. Sufficiency is kind of obvious. Necessity stems from how stream output modules are (not) going to like timestamps going backwards.

Easier said than done, of course.

TBH, I don't understand why we need a new clock ID, and I have the same prejudice against adding a field in so many objects for a corner case.

I have a version where the clock chooses the correct context where the difference output_ts - clock_context->first_pcr is the smallest. It's definitely the simplest solution, but maybe too naive ?

But if we really need this, I would expect a reference counted object rather than an integer.

It will increase the complexity. With this MR, the clock retains all contexts, and remove a specific one when all clocks passed to the next context.

It will increase the complexity. With this MR, the clock retains all contexts, and remove a specific one when all clocks passed to the next context.

Not if we use ancillaries that are naturally refcounted and we share the clock context directly

We will need to add ancillaries support in subpictures too.

For now, ancillaries are mostly created by decoders. No decoders or packetizer modules will copy the ancillaries from the input block to the output.

I have a version where the clock chooses the correct context where the difference output_ts - clock_context->first_pcr is the smallest. It's definitely the simplest solution, but maybe too naive ?

I'm personally not a big fan of this because we have no way to ensure that a stream will not have a PCR going backward. This would only fix one use case.

For now, ancillaries are mostly created by decoders. No decoders or packetizer modules will copy the ancillaries from the input block to the output.

They do copy ICC though. It's not really different of how you would copy the clock id from the frame to the picture.

I could try the ancillary approach, if we all agree on the great picture: are you OK to add support for PCR discontinuity without data discontinuity ?

Note that it is already handled (OK, maybe by luck) in VLC 3.0.

Necessity stems from how stream output modules are (not) going to like timestamps going backwards.

Going too far forward to trigger an $enteranybigvaluehere wait doesn't help either

I have a version where the clock chooses the correct context where the difference output_ts - clock_context->first_pcr is the smallest. It's definitely the simplest solution, but maybe too naive ?

Open GOPs have a long history of pts lower than dts with some containers

Since there's lots of duration computation using the non converted timestamps, this is going to be fun with still bogus durations.

mentioned in merge request !5202 (merged)

mentioned in merge request !5473 (closed)

mentioned in merge request !5626 (merged)

mentioned in merge request !5630 (merged)

Superseded by !5630 (merged)

closed