Fix: quickloop segment countdown timers #14
Conversation
|
|
||
| if (this.playlistImpl.quickLoop?.running) this.playlistImpl.quickLoop.running = false | ||
| // reset quickloop: | ||
| this.setQuickLoopMarker('start', null) |
There was a problem hiding this comment.
if the loop is locked (defined by ingest) these calls will throw
| const currentSegment = this.playoutModel.findSegment(part.segmentId)?.segment | ||
| const currentRundownIndex = rundownIds.findIndex((id) => id === part.rundownId) |
There was a problem hiding this comment.
I'm not keen on the efficiency of these lookups.
the current implementation of findSegment is iterating through each segment of each rundown until it finds a match, which means this gets up to being a 3 layer deep loop.
Maybe a segmentMap should be built first to make the cost be consistent/predictable?
There was a problem hiding this comment.
In fact, does this even need to iterate over the parts, or could it just be iterating over the segments? skipping the checks about partRank.
There was a problem hiding this comment.
Perhaps this could be simplified further if an alternate version of findQuickLoopMarkerPosition was created which returned the part/segment/rundownId instead of just the ranks?
Or I would be tempted to write this a bit differently.
- Resolve the start and end markers to the segment they correspond to.
- For a segment marker, it already contains the id
- For a part marker, use
playoutModel.findPart()and get the segmentId from that - For a rundown marker, use
playoutModel.getRundown()and get the first/last segment from that.
- then build a list of the ordered segment ids
- do a
findIndexfor both the start and end segments that you found - Using those indices, do a slice of the segmentId array.
| const playlist = playoutModel.playlist | ||
| if (!playlist.activationId) throw new Error(`Playlist has no activationId!`) | ||
| const wasQuickLoopRunning = playoutModel.playlist.quickLoop?.running | ||
| const oldProps = playoutModel.playlist.quickLoop |
There was a problem hiding this comment.
It might be good for this to explicitly clone the props, as there is no guarantee that the implementation of setQuickLoopMarker will do so
| } | ||
|
|
||
| // reset segments that have been added to the loop and are not on-air | ||
| resetPartInstancesWithPieceInstances(context, playoutModel, { |
There was a problem hiding this comment.
It would be good to check if the segmentsToReset.filter results in an empty array, so that we can avoid the iteration over all the partinstances, and the mongo queries.
Don't fix this, I've already done so as part of SOFIE-153, which needs to do the same thing in another job, so pulls this out into a method
| */ | ||
| setQuickLoopMarker(type: 'start' | 'end', marker: QuickLoopMarker | null): void | ||
|
|
||
| getSegmentsBetweenQuickLoopMarker(start: QuickLoopMarker, end: QuickLoopMarker): SegmentId[] |
There was a problem hiding this comment.
it would be good to have some documentation on this method. almost every other method on this interface does
2 parts to this:
Unit tests look to be broken in the base branch (bbc-r52)