feat: lookahead offset #413
Conversation
justandras
added
enhancement
WalkthroughAdded optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/timeline-state-resolver/src/integrations/casparCG/index.ts:
- Around line 316-323: The computation for seekOffsetByLookahead incorrectly
uses truthiness checks and treats 0 as falsy; update the logic in the
seekOffsetByLookahead assignment to explicitly check for undefined on
content.seek and layerProps.lookaheadOffset (e.g., use typeof or !== undefined),
add the two values when both are present (treat undefined as 0 for addition),
return the single numeric value when one is defined, and return undefined only
when both are undefined; locate the code by the seekOffsetByLookahead variable
and references to content.seek and layerProps.lookaheadOffset to apply this fix.
In @packages/timeline-state-resolver/src/integrations/quantel/state.ts:
- Around line 127-134: The ternary logic computing inPointSeekOffsetByLookahead
treats 0 as falsy and returns wrong results; change it to explicitly check for
undefined: treat content.inPoint and layer.lookaheadOffset as numbers (use e.g.
const inPoint = content.inPoint ?? undefined and const offset =
layer.lookaheadOffset ?? undefined), compute the sum when either is defined
(using 0 for the missing operand) and return undefined only when both are
undefined; update the inPointSeekOffsetByLookahead expression to use these
explicit undefined checks instead of truthiness.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/timeline-state-resolver-types/src/mapping.tspackages/timeline-state-resolver/src/__mocks__/objects.tspackages/timeline-state-resolver/src/integrations/casparCG/index.tspackages/timeline-state-resolver/src/integrations/quantel/state.tspackages/timeline-state-resolver/src/service/stateHandler.ts
🔇 Additional comments (5)
packages/timeline-state-resolver/src/__mocks__/objects.ts (1)
28-28: LGTM!The addition of
lookaheadOffset: undefinedcorrectly extends the mock object to include the new optional property, maintaining consistency with the updated type definitions.packages/timeline-state-resolver-types/src/mapping.ts (1)
29-30: LGTM!The addition of the optional
lookaheadOffsetproperty is well-documented and correctly typed. The comment clearly indicates it's only valid whenisLookaheadis true, which aligns with the usage patterns in the integrations.packages/timeline-state-resolver/src/integrations/quantel/state.ts (1)
149-149: Verify the conditional logic for inPoint assignment.The assignment uses
!isLookahead ? content.inPoint : inPointSeekOffsetByLookahead, which means:
- Non-lookahead clips use
content.inPointdirectly- Lookahead clips use the calculated offset
This logic appears correct, but ensure this aligns with the intended behavior where lookahead objects should respect the offset while regular playback should not.
packages/timeline-state-resolver/src/service/stateHandler.ts (1)
118-118: LGTM!The addition of
lookaheadOffset: objExt.lookaheadOffsetcorrectly propagates the property from the timeline object to the device timeline state object. Thesatisfies Complete<DeviceTimelineStateObject>constraint ensures type safety.packages/timeline-state-resolver/src/integrations/casparCG/index.ts (1)
336-336: Verify the conditional logic for seek assignment.The assignment uses
!layerProps.isLookahead ? content.seek : seekOffsetByLookahead, which means:
- Non-lookahead media uses
content.seekdirectly- Lookahead media uses the calculated offset
This logic appears correct and aligns with the intended behavior for the "Set next from here" feature, where lookahead previews need to seek to the offset position.
| const seekOffsetByLookahead = | ||
| content.seek && layerProps.lookaheadOffset | ||
| ? content.seek + layerProps.lookaheadOffset | ||
| : content.seek | ||
| ? content.seek | ||
| : layerProps.lookaheadOffset | ||
| ? layerProps.lookaheadOffset | ||
| : undefined |
There was a problem hiding this comment.
Fix truthiness check to handle 0 values correctly.
The current logic uses truthiness checks (content.seek && layerProps.lookaheadOffset) which incorrectly treats 0 as falsy. Since 0 is a valid seek value (seeking to the beginning of a clip), this will cause incorrect behavior when content.seek is 0.
For example:
seek=0, lookaheadOffset=undefined→ currently returnsundefinedinstead of0seek=0, lookaheadOffset=10→ currently returns10instead of10(works by accident)
This is the same issue as in the Quantel integration.
🔧 Proposed fix using explicit undefined checks
- const seekOffsetByLookahead =
- content.seek && layerProps.lookaheadOffset
- ? content.seek + layerProps.lookaheadOffset
- : content.seek
- ? content.seek
- : layerProps.lookaheadOffset
- ? layerProps.lookaheadOffset
- : undefined
+ const seekOffsetByLookahead =
+ content.seek !== undefined || layerProps.lookaheadOffset !== undefined
+ ? (content.seek ?? 0) + (layerProps.lookaheadOffset ?? 0)
+ : undefinedThis approach:
- Correctly handles
0as a valid seek value - Adds both values when present (treating
undefinedas0for addition) - Returns
undefinedonly when both areundefined - Is significantly more readable
🤖 Prompt for AI Agents
In @packages/timeline-state-resolver/src/integrations/casparCG/index.ts around
lines 316 - 323, The computation for seekOffsetByLookahead incorrectly uses
truthiness checks and treats 0 as falsy; update the logic in the
seekOffsetByLookahead assignment to explicitly check for undefined on
content.seek and layerProps.lookaheadOffset (e.g., use typeof or !== undefined),
add the two values when both are present (treat undefined as 0 for addition),
return the single numeric value when one is defined, and return undefined only
when both are undefined; locate the code by the seekOffsetByLookahead variable
and references to content.seek and layerProps.lookaheadOffset to apply this fix.
| const inPointSeekOffsetByLookahead = | ||
| content.inPoint && layer.lookaheadOffset | ||
| ? content.inPoint + layer.lookaheadOffset | ||
| : content.inPoint | ||
| ? content.inPoint | ||
| : layer.lookaheadOffset | ||
| ? layer.lookaheadOffset | ||
| : undefined |
There was a problem hiding this comment.
Fix truthiness check to handle 0 values correctly.
The current logic uses truthiness checks (content.inPoint && layer.lookaheadOffset) which incorrectly treats 0 as falsy. Since 0 is a valid seek/inPoint value (seeking to the beginning), this will cause incorrect behavior when content.inPoint is 0.
For example:
inPoint=0, lookaheadOffset=undefined→ currently returnsundefinedinstead of0inPoint=0, lookaheadOffset=5→ currently returns5instead of5(works by accident)
🔧 Proposed fix using explicit undefined checks
- const inPointSeekOffsetByLookahead =
- content.inPoint && layer.lookaheadOffset
- ? content.inPoint + layer.lookaheadOffset
- : content.inPoint
- ? content.inPoint
- : layer.lookaheadOffset
- ? layer.lookaheadOffset
- : undefined
+ const inPointSeekOffsetByLookahead =
+ content.inPoint !== undefined || layer.lookaheadOffset !== undefined
+ ? (content.inPoint ?? 0) + (layer.lookaheadOffset ?? 0)
+ : undefinedThis approach:
- Correctly handles
0as a valid value - Adds both values when present (treating
undefinedas0for addition) - Returns
undefinedonly when both areundefined - Is more readable than nested ternaries
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const inPointSeekOffsetByLookahead = | |
| content.inPoint && layer.lookaheadOffset | |
| ? content.inPoint + layer.lookaheadOffset | |
| : content.inPoint | |
| ? content.inPoint | |
| : layer.lookaheadOffset | |
| ? layer.lookaheadOffset | |
| : undefined | |
| const inPointSeekOffsetByLookahead = | |
| content.inPoint !== undefined || layer.lookaheadOffset !== undefined | |
| ? (content.inPoint ?? 0) + (layer.lookaheadOffset ?? 0) | |
| : undefined |
🤖 Prompt for AI Agents
In @packages/timeline-state-resolver/src/integrations/quantel/state.ts around
lines 127 - 134, The ternary logic computing inPointSeekOffsetByLookahead treats
0 as falsy and returns wrong results; change it to explicitly check for
undefined: treat content.inPoint and layer.lookaheadOffset as numbers (use e.g.
const inPoint = content.inPoint ?? undefined and const offset =
layer.lookaheadOffset ?? undefined), compute the sum when either is defined
(using 0 for the missing operand) and return undefined only when both are
undefined; update the inPointSeekOffsetByLookahead expression to use these
explicit undefined checks instead of truthiness.
| /** Only valid when isLookahead is true. Offsets the lookahead when a part or piece is queued with an offset */ | ||
| lookaheadOffset?: number |
There was a problem hiding this comment.
I think the meaning of what "offsetting the look-ahead" means here needs to be clarified.
| /** Only valid when isLookahead is true. Offsets the lookahead when a part or piece is queued with an offset */ | |
| lookaheadOffset?: number | |
| /** Only valid when isLookahead is true. If the nature of the content represented by the timeline object has a mutable timing dimension, present the content in a state it should be in after `lookaheadOffset` of it's contents has been played. */ | |
| lookaheadOffset?: number |
2 participants
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a: Feature
Bug fix / Feature / Code improvement / Documentation improvement / Other (please specify)
Current Behavior
There is no way to know about time offsets for the next part on lookaheads.
New Behavior
Added the lookaheadOffset property which is calculated by core. Implemented support for CasparCG and Quantel media.
Testing Instructions
Use the Set next from here feature on a CasparCG clip (pr with the feature: Sofie-Automation/sofie-core#1573)
The lookahead should seek to the desired time to provide accurate previews and prevent flicker on take.
Status
Overview
This pull request adds support for a new
lookaheadOffsetproperty that enables precise time offset handling during lookahead operations. This feature allows CasparCG and Quantel media handlers to seek to the correct position when previewing upcoming parts, improving preview accuracy and preventing visual flicker during take transitions.Changes Made
Type System
lookaheadOffset: numberproperty toTSRTimelineObjPropsinterface to allow core to convey time offsets for lookahead objectsType Definition Updates
__mocks__/objects.tsto include the newlookaheadOffsetproperty inDeviceTimelineStateObjectlookaheadOffsetfrom resolved timeline objects to the device timeline stateCasparCG Integration
seekOffsetByLookaheadcalculation for media layers during lookahead modeisLookaheadis true), the seek position is calculated by combiningcontent.seekandlayerProps.lookaheadOffsetQuantel Integration
inPointSeekOffsetByLookaheadcalculation for clip positioning during lookaheadlookaheadOffsettocontent.inPointTesting
The feature has been tested using the "Set next from here" functionality on CasparCG clips, which ensures that lookahead properly seeks to the desired time for accurate previews without visual artifacts during part transitions.
Notes