Skip to content

Commit 0fe74b4

Browse files
JulusianJulusian
committed
fix: unexpected timeline updates while playing final part in rundown SOFIE-3371 (#1237)
1 parent 195a7d9 commit 0fe74b4

File tree

3 files changed

+104
-68
lines changed

3 files changed

+104
-68
lines changed

‎packages/job-worker/src/ingest/__tests__/updateNext.test.ts‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -344,15 +344,15 @@ describe('ensureNextPartIsValid', () => {
344344
})
345345
}
346346
async function ensureNextPartIsValid() {
347-
await runJobWithPlayoutCache(context, { playlistId: rundownPlaylistId }, null, async (cache) =>
347+
return runJobWithPlayoutCache(context, { playlistId: rundownPlaylistId }, null, async (cache) =>
348348
ensureNextPartIsValidRaw(context, cache)
349349
)
350350
}
351351

352352
test('Start with null', async () => {
353353
await resetPartIds(null, null)
354354

355-
await ensureNextPartIsValid()
355+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
356356

357357
expect(setNextPartMock).toHaveBeenCalledTimes(1)
358358
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -365,7 +365,7 @@ describe('ensureNextPartIsValid', () => {
365365
test('Missing next PartInstance', async () => {
366366
await resetPartIds('mock_part_instance3', 'fake_part')
367367

368-
await ensureNextPartIsValid()
368+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
369369

370370
expect(setNextPartMock).toHaveBeenCalledTimes(1)
371371
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -380,14 +380,14 @@ describe('ensureNextPartIsValid', () => {
380380
test('Missing current PartInstance with valid next', async () => {
381381
await resetPartIds('fake_part', 'mock_part_instance4')
382382

383-
await ensureNextPartIsValid()
383+
await expect(ensureNextPartIsValid()).resolves.toBeFalsy()
384384

385385
expect(setNextPartMock).toHaveBeenCalledTimes(0)
386386
})
387387
test('Missing current and next PartInstance', async () => {
388388
await resetPartIds('fake_part', 'not_real_either')
389389

390-
await ensureNextPartIsValid()
390+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
391391

392392
expect(setNextPartMock).toHaveBeenCalledTimes(1)
393393
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -400,21 +400,21 @@ describe('ensureNextPartIsValid', () => {
400400
test('Ensure correct PartInstance doesnt change', async () => {
401401
await resetPartIds('mock_part_instance3', 'mock_part_instance4')
402402

403-
await ensureNextPartIsValid()
403+
await expect(ensureNextPartIsValid()).resolves.toBeFalsy()
404404

405405
expect(setNextPartMock).not.toHaveBeenCalled()
406406
})
407407
test('Ensure manual PartInstance doesnt change', async () => {
408408
await resetPartIds('mock_part_instance3', 'mock_part_instance5', true)
409409

410-
await ensureNextPartIsValid()
410+
await expect(ensureNextPartIsValid()).resolves.toBeFalsy()
411411

412412
expect(setNextPartMock).not.toHaveBeenCalled()
413413
})
414414
test('Ensure non-manual PartInstance does change', async () => {
415415
await resetPartIds('mock_part_instance3', 'mock_part_instance5', false)
416416

417-
await ensureNextPartIsValid()
417+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
418418

419419
expect(setNextPartMock).toHaveBeenCalledTimes(1)
420420
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -427,7 +427,7 @@ describe('ensureNextPartIsValid', () => {
427427
test('Ensure manual but missing PartInstance does change', async () => {
428428
await resetPartIds('mock_part_instance3', 'fake_part', true)
429429

430-
await ensureNextPartIsValid()
430+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
431431

432432
expect(setNextPartMock).toHaveBeenCalledTimes(1)
433433
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -440,7 +440,7 @@ describe('ensureNextPartIsValid', () => {
440440
test('Ensure manual but floated PartInstance does change', async () => {
441441
await resetPartIds('mock_part_instance7', 'mock_part_instance8', true)
442442

443-
await ensureNextPartIsValid()
443+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
444444

445445
expect(setNextPartMock).toHaveBeenCalledTimes(1)
446446
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -453,7 +453,7 @@ describe('ensureNextPartIsValid', () => {
453453
test('Ensure floated PartInstance does change', async () => {
454454
await resetPartIds('mock_part_instance7', 'mock_part_instance8', false)
455455

456-
await ensureNextPartIsValid()
456+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
457457

458458
expect(setNextPartMock).toHaveBeenCalledTimes(1)
459459
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -490,7 +490,7 @@ describe('ensureNextPartIsValid', () => {
490490

491491
await resetPartIds(null, instanceId, false)
492492

493-
await ensureNextPartIsValid()
493+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
494494

495495
expect(setNextPartMock).toHaveBeenCalledTimes(1)
496496
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -527,7 +527,7 @@ describe('ensureNextPartIsValid', () => {
527527

528528
await resetPartIds(null, instanceId, true)
529529

530-
await ensureNextPartIsValid()
530+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
531531

532532
expect(setNextPartMock).toHaveBeenCalledTimes(1)
533533
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -568,7 +568,7 @@ describe('ensureNextPartIsValid', () => {
568568

569569
await resetPartIds('mock_part_instance1', instanceId, false)
570570

571-
await ensureNextPartIsValid()
571+
await expect(ensureNextPartIsValid()).resolves.toBeFalsy()
572572

573573
expect(setNextPartMock).toHaveBeenCalledTimes(0)
574574
})
@@ -601,7 +601,7 @@ describe('ensureNextPartIsValid', () => {
601601
try {
602602
// make sure it finds the part we expect
603603
await resetPartIds('mock_part_instance9', null, false)
604-
await ensureNextPartIsValid()
604+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
605605

606606
expect(setNextPartMock).toHaveBeenCalledTimes(1)
607607
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -619,7 +619,7 @@ describe('ensureNextPartIsValid', () => {
619619
await context.mockCollections.Parts.remove(part._id)
620620

621621
// make sure the next part gets cleared
622-
await ensureNextPartIsValid()
622+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
623623

624624
expect(setNextPartMock).toHaveBeenCalledTimes(1)
625625
expect(setNextPartMock).toHaveBeenCalledWith(
@@ -634,4 +634,26 @@ describe('ensureNextPartIsValid', () => {
634634
await context.mockCollections.Parts.remove(part._id)
635635
}
636636
})
637+
638+
test('Current part is last in rundown, next is missing', async () => {
639+
await resetPartIds('mock_part_instance9', 'fake_part_instance', false)
640+
641+
await expect(ensureNextPartIsValid()).resolves.toBeTruthy()
642+
643+
expect(setNextPartMock).toHaveBeenCalledTimes(1)
644+
expect(setNextPartMock).toHaveBeenCalledWith(
645+
expect.objectContaining({}),
646+
expect.objectContaining({ PlaylistId: rundownPlaylistId }),
647+
null,
648+
false
649+
)
650+
})
651+
652+
test('Current part is last in rundown, no-op to update', async () => {
653+
await resetPartIds('mock_part_instance9', null, false)
654+
655+
await expect(ensureNextPartIsValid()).resolves.toBeFalsy()
656+
657+
expect(setNextPartMock).toHaveBeenCalledTimes(0)
658+
})
637659
})

‎packages/job-worker/src/ingest/commit.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,7 @@ export async function CommitIngestOperation(
242242
await pSaveIngest
243243

244244
// do some final playout checks, which may load back some Parts data
245+
// Note: This should trigger a timeline update, one is already queued in the `deferAfterSave` above
245246
await ensureNextPartIsValid(context, playoutCache)
246247

247248
// save the final playout changes
@@ -511,9 +512,9 @@ export async function updatePlayoutAfterChangingRundownInPlaylist(
511512
)
512513
}
513514

514-
await ensureNextPartIsValid(context, playoutCache)
515+
const shouldUpdateTimeline = await ensureNextPartIsValid(context, playoutCache)
515516

516-
if (playoutCache.Playlist.doc.activationId) {
517+
if (playoutCache.Playlist.doc.activationId || shouldUpdateTimeline) {
517518
triggerUpdateTimelineAfterIngestData(context, playoutCache.PlaylistId)
518519
}
519520
})

‎packages/job-worker/src/ingest/updateNext.ts‎

Lines changed: 63 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -8,78 +8,91 @@ import {
88
import { JobContext } from '../jobs'
99
import { setNextPart } from '../playout/setNext'
1010
import { isPartPlayable } from '@sofie-automation/corelib/dist/dataModel/Part'
11-
import { updateTimeline } from '../playout/timeline/generate'
1211

1312
/**
1413
* Make sure that the nextPartInstance for the current Playlist is still correct
1514
* This will often change the nextPartInstance
1615
* @param context Context of the job being run
1716
* @param cache Playout Cache to operate on
17+
* @returns Whether the timeline should be updated following this operation
1818
*/
19-
export async function ensureNextPartIsValid(context: JobContext, cache: CacheForPlayout): Promise<void> {
19+
export async function ensureNextPartIsValid(context: JobContext, cache: CacheForPlayout): Promise<boolean> {
2020
const span = context.startSpan('api.ingest.ensureNextPartIsValid')
2121

2222
// Ensure the next-id is still valid
2323
const playlist = cache.Playlist.doc
24-
if (playlist?.activationId) {
25-
const { currentPartInstance, nextPartInstance } = getSelectedPartInstancesFromCache(cache)
24+
if (!playlist?.activationId) {
25+
span?.end()
26+
return false
27+
}
28+
29+
const { currentPartInstance, nextPartInstance } = getSelectedPartInstancesFromCache(cache)
30+
31+
if (
32+
playlist.nextPartInfo?.manuallySelected &&
33+
nextPartInstance?.part &&
34+
isPartPlayable(nextPartInstance.part) &&
35+
nextPartInstance.orphaned !== 'deleted'
36+
) {
37+
// Manual next part is almost always valid. This includes orphaned (adlib-part) partinstances
38+
span?.end()
39+
return false
40+
}
41+
42+
// If we are close to an autonext, then leave it to avoid glitches
43+
if (isTooCloseToAutonext(currentPartInstance) && nextPartInstance) {
44+
span?.end()
45+
return false
46+
}
47+
48+
const allPartsAndSegments = getOrderedSegmentsAndPartsFromPlayoutCache(cache)
49+
50+
if (currentPartInstance && nextPartInstance) {
51+
// Check if the part is the same
52+
const newNextPart = selectNextPart(
53+
context,
54+
playlist,
55+
currentPartInstance,
56+
nextPartInstance,
57+
allPartsAndSegments
58+
)
2659

2760
if (
28-
playlist.nextPartInfo?.manuallySelected &&
29-
nextPartInstance?.part &&
30-
isPartPlayable(nextPartInstance.part) &&
31-
nextPartInstance.orphaned !== 'deleted'
61+
// Nothing should be nexted
62+
!newNextPart ||
63+
// The nexted-part should be different to what is selected
64+
newNextPart.part._id !== nextPartInstance.part._id ||
65+
// The nexted-part Instance is no longer playable
66+
!isPartPlayable(nextPartInstance.part)
3267
) {
33-
// Manual next part is almost always valid. This includes orphaned (adlib-part) partinstances
68+
// The 'new' next part is before the current next, so move the next point
69+
await setNextPart(context, cache, newNextPart ?? null, false)
70+
3471
span?.end()
35-
return
72+
return true
3673
}
74+
} else if (!nextPartInstance || nextPartInstance.orphaned === 'deleted') {
75+
// Don't have a nextPart or it has been deleted, so autoselect something
76+
const newNextPart = selectNextPart(
77+
context,
78+
playlist,
79+
currentPartInstance ?? null,
80+
nextPartInstance ?? null,
81+
allPartsAndSegments
82+
)
3783

38-
// If we are close to an autonext, then leave it to avoid glitches
39-
if (isTooCloseToAutonext(currentPartInstance) && nextPartInstance) {
84+
if (!newNextPart && !cache.Playlist.doc?.nextPartInfo) {
85+
// No currently nexted part, and nothing was selected, so nothing to update
4086
span?.end()
41-
return
87+
return false
4288
}
4389

44-
const allPartsAndSegments = getOrderedSegmentsAndPartsFromPlayoutCache(cache)
45-
46-
if (currentPartInstance && nextPartInstance) {
47-
// Check if the part is the same
48-
const newNextPart = selectNextPart(
49-
context,
50-
playlist,
51-
currentPartInstance,
52-
nextPartInstance,
53-
allPartsAndSegments
54-
)
55-
56-
if (
57-
// Nothing should be nexted
58-
!newNextPart ||
59-
// The nexted-part should be different to what is selected
60-
newNextPart.part._id !== nextPartInstance.part._id ||
61-
// The nexted-part Instance is no longer playable
62-
!isPartPlayable(nextPartInstance.part)
63-
) {
64-
// The 'new' next part is before the current next, so move the next point
65-
await setNextPart(context, cache, newNextPart ?? null, false)
90+
await setNextPart(context, cache, newNextPart ?? null, false)
6691

67-
await updateTimeline(context, cache)
68-
}
69-
} else if (!nextPartInstance || nextPartInstance.orphaned === 'deleted') {
70-
// Don't have a nextPart or it has been deleted, so autoselect something
71-
const newNextPart = selectNextPart(
72-
context,
73-
playlist,
74-
currentPartInstance ?? null,
75-
nextPartInstance ?? null,
76-
allPartsAndSegments
77-
)
78-
await setNextPart(context, cache, newNextPart ?? null, false)
79-
80-
await updateTimeline(context, cache)
81-
}
92+
span?.end()
93+
return true
8294
}
8395

8496
span?.end()
97+
return false
8598
}

0 commit comments

Comments
 (0)