Skip to content

Conversation

@cedrik-fuoco-adsk
Copy link
Contributor

@cedrik-fuoco-adsk cedrik-fuoco-adsk commented May 21, 2025

Incorrect EDL in multi-fps playlists

Linked issues

n/a

Summarize your change.

The calculation of the frame and out properties in the EDL was incorrect for playlists containing clips with varying frame rates.

In _get_in_out_frame, the values for the in and out frames were not what RV is expecting. For all playlists, the in values must be the start_time of the clip at its rate - not the rate of the sequence/timeline (which is the rate of the first clip).

For the out value:

  • In multi-fps playlists, it should be start_time at the clip’s rate plus the duration at the sequence/timeline rate.
  • In single-fps playlists, it should be the end time (inclusive) at the clip’s rate (no conversion needed).

For the frame value in the EDL, I based the implementation on what RV is doing here:

int accum = 1;
for (int i = 0; i < ins.size(); i++)
{
const ImageRangeInfo& info = m_rangeInfos[i];
const int start = useCutInfo ? info.cutIn : info.start;
const int end = useCutInfo ? info.cutOut : info.end;
(*m_edlGlobalIn)[i] = accum;
(*m_edlSource)[i] = i;
(*m_edlSourceIn)[i] = start;
(*m_edlSourceOut)[i] = end;
accum += (end - start + 1);
}

Describe the reason for the change.

The current implementation of the EDL calculations for the frames and out were wrong. The values were not what RV was expecting. The original issue was that the clip were stuck at the last frame of the clip because the EDL values are wrong.

Note:
There are some cases in a multi-fps playlists where RV and CR does not interpret a frame calculation the same way. For example, RV will do a calculation similar to a floor() while CR will be a ceil()

Describe what you have tested and on which operating system.

MacOS, Linux

Add a list of changes, and note any that might need special attention during the review.

If possible, provide screenshots.

Signed-off-by: Cédrik Fuoco <[email protected]>
@pbergeron-adsk pbergeron-adsk self-requested a review May 28, 2025 07:58
Copy link
Contributor

@pbergeron-adsk pbergeron-adsk left a comment

Choose a reason for hiding this comment

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

Ok, but can you specify in your pr comment what are the other issues you intend to fix in other PRs?

@cedrik-fuoco-adsk
Copy link
Contributor Author

It's one specific issue. There are some cases in a multi-fps playlists where RV and CR does not interpret a frame calculation the same way. For example, RV will do a calculation similar to a floor() while CR will be a ceil(). There are no fix until we decide which one is right. @bernie-laberge and me decided to deal with that later.

Copy link
Contributor

@bernie-laberge bernie-laberge left a comment

Choose a reason for hiding this comment

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

LGTM
Please add the repro steps of SG-38200 for the post checkin test just to be safe.

@cedrik-fuoco-adsk cedrik-fuoco-adsk dismissed pbergeron-adsk’s stale review May 30, 2025 11:13

Comment has been added

@cedrik-fuoco-adsk cedrik-fuoco-adsk added do not merge Do not merge the PR and removed do not merge Do not merge the PR labels Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Do not merge the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants