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

AY-6975 Consolidate retimes computation. #1087

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

robin-ynput
Copy link
Contributor

@robin-ynput robin-ynput commented Jan 15, 2025

Changelog Description

help resolve ynput/ayon-hiero#29
help resolve ynput/ayon-flame#28

  • FreezeFrame does not work due to wong available media range boudary computation
  • Negative speed is not always accurate
  • TimeWrap:
    • default setup seems to go through OK
    • when length attribute is used/animated publish does got go through because the retime operation is expressed as otio.AnyVector which is none-serializable by integrate asset plugin
    • when input frame is not animated (freeze frame if I'm correct), it fails with an index error during otio subset collect.
  • LinearTimeWarp produce correct output clip (tried speed = 0.5 and speed = 2), retime information can be properly gathered from Nuke.
  • Retiming is not properly applied on reviewable.

Additional info

This needs to be tested alongside:

Testing notes:

  1. Run the automated tests
    .\tools\manage.ps1 run pytest .\tests\

@ynbot
Copy link
Contributor

ynbot commented Jan 15, 2025

Task linked: AY-6975 Broken retiming calculation

@robin-ynput robin-ynput self-assigned this Jan 15, 2025
@robin-ynput robin-ynput added type: bug Something isn't working tests PR contains new unit or integration test or improves the existing ones labels Jan 15, 2025
@jakubjezek001
Copy link
Member

Tested current dev and all is passing fine

===================================== 28 passed, 17 warnings in 1.11s =====================================

Comment on lines +423 to +426
offset_duration = conformed_source_range.duration.value * abs(time_scalar)
offset_duration -= 1 # duration 1 frame -> freeze frame -> end = start + 0
offset_duration = max(0, offset_duration) # negative duration = frozen frame
media_out_trimmed = media_in_trimmed + offset_duration
Copy link
Member

@iLLiCiTiT iLLiCiTiT Jan 22, 2025

Choose a reason for hiding this comment

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

Based on the code changes, can happen that conformed_source_range.duration.value * abs(time_scalar) is less than 0? If not then this would be preferred:

        offset_duration = conformed_source_range.duration.value * abs(time_scalar)
        # Offset duration by 1 for media out frame
        # - only if duration is not single frame (start frame != end frame)
        if offset_duration > 0: 
            offset_duration -= 1

From code reading perspecive more clear what is being handled.

NOTE: comment duration 1 frame -> freeze frame -> end = start + 0 was confusing for me, but feel free to keep it if it makes more sense.

Comment on lines +444 to +457
frame_range[idx] = round(frame_number + tw["lookup"][idx] * time_scalar)
# Consecutive timewarp, apply on the previous result
else:
new_idx = round(idx + tw["lookup"][idx])

if not 0 <= new_idx < len(frame_range):
# TODO: implementing this would need to actually have
# retiming engine resolve process within AYON, resolving wraps
# as curves, then projecting those into the previous media_range.
raise NotImplementedError(
"Unsupported consecutive timewarps (out of computed range)"
)

frame_range[idx] = frame_range[new_idx]
Copy link
Member

@iLLiCiTiT iLLiCiTiT Jan 22, 2025

Choose a reason for hiding this comment

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

Reading if not 0 <= new_idx < len(frame_range): is pretty challenging...

Suggested change
frame_range[idx] = round(frame_number + tw["lookup"][idx] * time_scalar)
# Consecutive timewarp, apply on the previous result
else:
new_idx = round(idx + tw["lookup"][idx])
if not 0 <= new_idx < len(frame_range):
# TODO: implementing this would need to actually have
# retiming engine resolve process within AYON, resolving wraps
# as curves, then projecting those into the previous media_range.
raise NotImplementedError(
"Unsupported consecutive timewarps (out of computed range)"
)
frame_range[idx] = frame_range[new_idx]
frame_range[idx] = round(frame_number + tw["lookup"][idx] * time_scalar)
continue
# Consecutive timewarp, apply on the previous result
new_idx = round(idx + tw["lookup"][idx])
if 0 <= new_idx < len(frame_range):
frame_range[idx] = frame_range[new_idx]
continue
# TODO: implementing this would need to actually have
# retiming engine resolve process within AYON, resolving wraps
# as curves, then projecting those into the previous media_range.
raise NotImplementedError(
"Unsupported consecutive timewarps (out of computed range)"
)

If you preffer first raise exception then please use reversed condition if 0 > new_idx >= len(frame_range): (without not).

Comment on lines 434 to 438
frame_range = [media_in_trimmed]
in_frame = media_in_trimmed + time_scalar
while in_frame <= media_out_trimmed:
frame_range.append(in_frame)
in_frame += time_scalar
Copy link
Member

@iLLiCiTiT iLLiCiTiT Jan 22, 2025

Choose a reason for hiding this comment

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

If time_scalar is int then this can be simplified to:

Suggested change
frame_range = [media_in_trimmed]
in_frame = media_in_trimmed + time_scalar
while in_frame <= media_out_trimmed:
frame_range.append(in_frame)
in_frame += time_scalar
frame_range = list(range(
media_in_trimmed,
media_out_trimmed + 1,
time_scalar
))

If is float then this is better.

repre = self._create_representation(
frame_start, frame_end, collection=collection)
else:
filename, = tuple(collection)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same?

Suggested change
filename, = tuple(collection)
filename = collection[0]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Resolve size/XL tests PR contains new unit or integration test or improves the existing ones type: bug Something isn't working
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

Bring back publishing of clip retimes AY-6975_Broken retiming calculation
4 participants