Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AB session issues #1261

Merged

Conversation

Julusian
Copy link
Member

About the Contributor

This pull request is posted on behalf of the BBC

Type of Contribution

This is a: Bug fix / Feature

This is a couple of intertwined changes. It might be possible to pull the feature out into its own PR if wanted, but its a pretty isolated opt-in functionality and overlaps in area with the fixes.

Each change is a separate commit in this PR.

Current/New Behavior

There is a limitation in the AB logic, where an infinite piece is not allowed to share a session with any other piece. This limitation was added originally because a sessionName was described as needing to be unique within a segment, once out of that segment, it is hard to be sure that the name won't be reused unless care is taken to ensure it is globally unique.
This PR allows sessions to opt into being treated as a globally unique id. This allows for skipping a lot of the complex session matching logic performed across parts, as we can be confident that any other references to the same id must be a continuation of a session.
This can also be used for some hold-like behaviour, where a piece gets copied between parts during onSetAsNext, prior to this change that would sometimes work, depending on where the two parts were located in the rundown.

This fixes a bug where pieces with a postrollDuration were being added to the timeline, referencing the end of their part, which did not have an end time set. This added some noise to the timeline objects, and confused sofie when doing getResolvedPieces, as the pieces ended up reporting as having an end time based on the planned duration of the part. This in turn would confuse the AB resolver, as it would see all the sessions as ending when the part reached its expectedDuration, which could cause players to be reallocated while they were still being used.

And another fixed bug, where adlibbing a piece using ab with a fixed duration into a part would not cause the ab of lookahead objects to be reassigned correctly, which would result in the next clip disappearing from the multiviewer until the adlibbed clip ended. I think that during a take it would end up swapping to the other player, without sufficient time to preload

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

Time Frame

Other Information

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.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 88.70056% with 20 lines in your changes missing coverage. Please review.

Project coverage is 57.91%. Comparing base (8e16b94) to head (250b5d7).
Report is 38 commits behind head on release52.

Files with missing lines Patch % Lines
...ackages/job-worker/src/playout/abPlayback/index.ts 25.00% 18 Missing ⚠️
...rc/blueprints/context/OnTimelineGenerateContext.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           release52    #1261      +/-   ##
=============================================
- Coverage      61.36%   57.91%   -3.45%     
=============================================
  Files            449      526      +77     
  Lines          76483    85035    +8552     
  Branches        4936     4422     -514     
=============================================
+ Hits           46933    49249    +2316     
- Misses         29422    35732    +6310     
+ Partials         128       54      -74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nytamin nytamin changed the base branch from release51 to release52 September 25, 2024 08:08
@nytamin nytamin merged commit cdb6c90 into nrkno:release52 Sep 25, 2024
41 checks passed
nytamin added a commit that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants