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

Fix wrong retimed detection on image sequence clip. #897

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

robin-ynput
Copy link
Contributor

@robin-ynput robin-ynput commented Sep 17, 2024

Changelog Description

This PR addresses the bug described on #895 regarding retiming OTIO clip from image sequence.

  • Need to conform source_range to available_range rate
  • Retiming logic was when input media does not start 0 (image sequence, metadata...)
  • Ensure embedded TC are treated correctly
  • Need to preserve accurate discreet frame number when dealing with img sequences

Testing notes:

Environment prep

  1. copy paste items form both tool.poetry.dependencies and ayon.runtimeDependencies sections from ayon-core\client\pyproject.toml into root level pyproject.toml under tool.poetry.dev-dependencies
  2. remove ayon-core\poetry.lock
  3. run .\tools\manage.ps1 create-env (windows)

Your .poetry and .venv should be installed in root of repository

Testing

  1. Run the unit tests
    run .\tools\manage.ps1 run pytest .\tests\client\ayon_core\pipeline\editorial\ (windows)

AY-979

@jakubjezek001
Copy link
Member

jakubjezek001 commented Sep 18, 2024

For the testing data, we also need to check the retime effects (LinearTimeWarp, FreezeFrame, TimeEffect). Since the method name relates to retiming, this will cover the full range of functions. Great job on practically developing the unittest we really need! I'm thinking about whether we should add the test to the appropriate folder. @kalisp and @antirotor could lend a hand here, especially since ayon-core doesn't have a test folder yet.

@jakubjezek001 jakubjezek001 assigned kalisp and unassigned robin-ynput Sep 18, 2024
@jakubjezek001
Copy link
Member

Before the introduction of Resolve's native OTIO, we mainly relied on two attributes:

  • The clip's source start frame as a relative range to the available media range.
  • The clip's media range as an absolute range, including the metadata timecode value.

With Resolve's interpretation of OTIO now in play, we consider:

  • The clip's source start frame refers to a trimmed absolute range that is available.
  • The media range remains absolute, including metadata.

We reference the media source to an absolute value with included metadata because of FFMPEG. Since we use FFMPEG for video trimming, it's crucial to have the timecode correctly defined.

First, figure out if the clip source range is relative or absolute to the media range. Next, find the relative source frame value and continue as before, making sure to clear up any confusing code that's mistakenly referring to me (old me 😄).

@antirotor
Copy link
Member

... especially since ayon-core doesn't have a test folder yet.

but it can - I added one for my own tests, see feature/OP-1188_better-representation-model branch.

@robin-ynput robin-ynput self-assigned this Sep 18, 2024
@robin-ynput robin-ynput self-assigned this Sep 18, 2024
@jakubjezek001
Copy link
Member

The method get_media_range_with_retimes is used in collect_timeline_instances.py on line 300 and also in extract_product_resources.py on line 413.

I originally used this method to calculate how much material needs to be prepared for on-the-fly Flame transcoding. While looking into flame_export.py, which handles ocio clip data, I noticed that from line 256, the available frame range on ImageSequenceReference is set by frame number rather than Timecode. This confirms your assumption was correct.

When I wrote the Flame integration two years ago, I faced the challenge of using the embedded TC on the image sequence. From the code in flame_export.py on line 305, I can see that I was using the embedded TC to calculate the true source in/out for the clip, and later for calculating retime speed (though this equation might be incorrect).

@jakubjezek001
Copy link
Member

I was doing some tests within Hiero editorial and noticed some differences in the resulting calculations. Here is the outcome before and after the fix. Since it's an old publisher, I could only copy and paste some resulting logs.

otio_clip_publish_data_hiero.zip

@robin-ynput
Copy link
Contributor Author

I was doing some tests within Hiero editorial and noticed some differences in the resulting calculations. Here is the outcome before and after the fix. Since it's an old publisher, I could only copy and paste some resulting logs.

otio_clip_publish_data_hiero.zip

Thanks for the additional tests in Hiero, I've investigated that a bit more.
It looks like the issues comes from AYON native OTIO exporter which introduce a mismatch between the available and source ranges.

When I use Hiero native OTIO exporter, our clips are retimed as expected.
I've made an issue to fix this in ayon-hiero: ynput/ayon-hiero#13

@ynbot
Copy link
Contributor

ynbot commented Sep 24, 2024

Task linked: OP-8066 Resolve: update to new publisher

@iLLiCiTiT
Copy link
Member

I'm not sure what is state of this PR?

@jakubjezek001 jakubjezek001 self-assigned this Sep 25, 2024
@robin-ynput
Copy link
Contributor Author

For the testing data, we also need to check the retime effects (LinearTimeWarp, FreezeFrame, TimeEffect). Since the method name relates to retiming, this will cover the full range of functions. Great job on practically developing the unittest we really need! I'm thinking about whether we should add the test to the appropriate folder. @kalisp and @antirotor could lend a hand here, especially since ayon-core doesn't have a test folder yet.

I'm not sure what is state of this PR?

I'm waiting on approval on this one.
I have addressed the feedback and added the tests, the Hiero problem that was reported will be tackled as its own in ayon-hiero (ynput/ayon-hiero#13).

jakubjezek001 and others added 3 commits September 25, 2024 16:06
- Added client path to sys.path in conftest.py
- Implemented function to run the repository from code in manage.ps1
@@ -157,6 +157,7 @@ default_help() {
echo -e " ${BWhite}ruff-check${RST} ${BCyan}Run Ruff check for the repository${RST}"
echo -e " ${BWhite}ruff-fix${RST} ${BCyan}Run Ruff fix for the repository${RST}"
echo -e " ${BWhite}codespell${RST} ${BCyan}Run codespell check for the repository${RST}"
echo -e " ${BWhite}run${RST} ${BCyan}Run the repository${RST}"
Copy link
Member

Choose a reason for hiding this comment

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

???
What "Run the repository" means?

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

Tested via pytests and also via Resolve and works perfect!

Comment on lines +181 to +182
shift; # will remove first arg ("run") from the "$@"
"$POETRY_HOME/bin/poetry" run "$@"
Copy link
Member

Choose a reason for hiding this comment

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

So, we're replacing run with run?

@jakubjezek001 jakubjezek001 removed their assignment Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S type: bug Something isn't working
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

AY-979_Collect OTIO subset resources - get_media_range_with_retimes returns negative range values
7 participants