Skip to content

Conversation

nytamin
Copy link
Member

@nytamin nytamin commented Aug 28, 2025

About the Contributor

This pull request is posted on behalf of the NRK.

Type of Contribution

This is a: Bug fix

Current Behavior

We have found a issue where the timings of pieces are not monotonic.
This only affects infinite pieces, and probably only when multiGatewayMode is enabled.

How to reproduce:

In a blueprints adlibAction:

// Stop the previous (infinite) piece
await context.stopPiecesOnLayers(['myLayer'])

await insertPiecesTyped(context, 'current', {
	[...],
	sourceLayerId: 'myLayer',
	enable: { start: 'now' },
	lifespan: PieceLifespan.OutOnShowStyleEnd,
})

This results in a virtual piece being inserted to stop the previous infinite OutOnShowStyleEnd piece, at time getCurrentTime()+calculateNowOffsetLatency (this is correct).
Then a pieceInstance is inserted at time "now", which is later resolved using getCurrentTime().
In our case, this ends up in that the virtual piece ends up after the inserted Piece, thus instantly ending it.

New Behavior

The same getCurrentTime()+calculateNowOffsetLatency time is used in both deNowifyMultiGatewayTimeline and in getTimelineRundown.

Testing

  • I have added one or more unit tests for this PR
  • I have updated the relevant unit tests
  • No unit test changes are needed for this PR

Affected areas

This PR affects the playout logic in general.

Time Frame

  • This is a critical bug for us, but not urgent to merge into upstream.

Other Information

Disclaimer: I have not gone through the other places where getCurrentTime() is used, so there might be other places calculateNowOffsetLatency should be used.

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

… resolving "now" and in onTimelineGenerate context
@nytamin nytamin requested a review from a team as a code owner August 28, 2025 11:01
@nytamin nytamin closed this Aug 28, 2025
@nytamin nytamin deleted the fix/resolve-timeline-wrong-time branch August 28, 2025 11:02
@nytamin nytamin restored the fix/resolve-timeline-wrong-time branch August 28, 2025 11:02
@nytamin nytamin reopened this Aug 28, 2025
@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...s/job-worker/src/playout/timeline/multi-gateway.ts 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@nytamin
Copy link
Member Author

nytamin commented Sep 2, 2025

@Julusian mentioned an issue with that we're now generating a now-time from a point-in-time before getTimelineRundown has run, which might cause issues since the variability in execution-time of getTimelineRundown will now play a part, which might not be ideal.

Copy link
Member

@Julusian Julusian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As hinted above, I think that this will cause some timing issues.

This means that the targetNowTime used when de-nowifiying the timeline has the latency of playout-gateway included in the calculation, but not the latency of building the timeline.

When fixing up the multi-gateway mode, I intentionally kept the now values in the timeline with a deNowify step at the end to avoid this. This does cost some complexity, but allowed for keeping the shorted scoped latency.

So either:

  1. This is the wrong solution, and the gap should be handled somewhere else
  2. The latency calculation should be changed to measure the generation time too. This will concern me, as we call a few blueprint methods in there, and there is a lot of potential for O(n^2) or similarly non-linear latency variance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants