Remove redundant frame range assignment in extract_review (YN-0888)

[jakubjezek001]

Changelog Description

Remove redundant frame range assignment in extract_review

Additional info

This was actually creating false Burnin frame range. So in case these data:

frameStart: 1001
frameEnd: 1052
handleStart: 4
handleEnd: 4

Burnins and Slater were getting double handles calculation and {frames} were formatted as: 993-1060 (68) but it really should have been 997-1056 (60)

Testing notes:

1. publish some image sequence on folder with handles
2. check the burnins frame region before and after the PR.

closes https://github.com/ynput/ayon-slater/issues/40

[BigRoy]

Doesn't extract review always run before extract burnin? Why hasn't this been reported more often? Sounds like an obvious case where this would happen rather often?

[jakubjezek001]

Doesn't extract review always run before extract burnin? Why hasn't this been reported more often? Sounds like an obvious case where this would happen rather often?

Agree that this is strange noone was reporting it yet, but the data for it was found in my latest debugging logs and it would be great if someone tested it first @moonyuet or @tadeas-hejnic?

[Copilot]

Pull request overview

Removes a side-effect in extract_review where the plugin overwrote instance.data["frameStart"]/["frameEnd"] when fill_missing_frames == "only_rendered", which could lead downstream burnin/frame formatting logic (e.g. Slater/burnins) to apply handles twice and display an incorrect frame range.

Changes:

• Stop mutating instance.data["frameStart"] and instance.data["frameEnd"] inside the "only_rendered" fill-missing-frames path.
• Keep the “only rendered frames” range confined to the extractor’s internal temp_data instead of globally affecting the instance.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jakubjezek001]

Additionally I am adding logs for investigation ExtractBurnin and ExtractSlater..

before_the_fix_dl_log.txt

after_the_fix_dl_log.txt

[moonyuet]

As far as I tested from my side, I dont think the attributes adjust the frame start and frame end in burnin.

Published successfully as expected.
Enclosed with the publish report

publish-report-260702-22-40.json

[iLLiCiTiT]

I actually have local changes with this

frame_start = min(collection.indexes)
frame_end = max(collection.indexes)
temp_data.frame_start = frame_start
temp_data.frame_end = frame_end
temp_data.first_sequence_frame = frame_start

# modify range for burnins
frame_diff = frame_end - frame_start
i_frame_start = instance.data["frameStart"]
instance.data["frameEnd"] = i_frame_start + frame_diff + 1

I guess I already did look into it in past?

It changes first_sequence_frame too and modifies only frame end on the instance.

[BigRoy]

I actually have local changes with this

frame_start = min(collection.indexes)
frame_end = max(collection.indexes)
temp_data.frame_start = frame_start
temp_data.frame_end = frame_end
temp_data.first_sequence_frame = frame_start

# modify range for burnins
frame_diff = frame_end - frame_start
i_frame_start = instance.data["frameStart"]
instance.data["frameEnd"] = i_frame_start + frame_diff + 1

I guess I already did look into it in past?

It changes first_sequence_frame too and modifies only frame end on the instance.

Logic is still wrong unfortunately.

Our handles are outside of the frame ranges.
It would need to be:

handles = instance.data.get("handleStart", 0) + instance.data.get("handleEnd", 0)
instance.data["frameEnd"] = i_frame_start + frame_diff + 1 - handles

This does have the caveat that whatever "new range" we're computing somehow is shorter than the amount of handles that things will go haywire.
Can we actually still rely on any handles if we're just somehow figuring out a possibly random new frame range?

What are we solving? Why do we need to update the frame range?

Also, what's first_sequence_frame?

[iLLiCiTiT]

Also, what's first_sequence_frame?

That's used for ffmpeg commands in rest of the logic. Is important only if frame start changes.

Can we actually still rely on any handles if we're just somehow figuring out a possibly random new frame range?

I don't know. If you drop random files to traypublisher/webpublisher, or render custom frame range of anything, you probably can't handle handles, or?

NOTE This PR is breaking all those cases now BTW, probably.
I wonder why we do handle the frame changes of instance in extract review. For some reason? Shouldn't that happen when the frames are actually defined to be custom? Are handles currently set up automatically?

We 100% can't use the frame start from the files, because those can be random, can be clip start instead of frame start, can be from frame 1 instead of 1001, so the existing logic is also wrong.

[iLLiCiTiT]

Ok, this is wrong on many levels. I do believe this is correct fix because extract review should not auto-fix frameStart data on instance, the question is what will this break.

I really tend to believe that each representation file should have information about filename frame and frame it represents during publishing (because it can be different), with that all these guesses would be gone.

@BigRoy my suggestion is to merge, wait if/what it breaks and based on that fix that in a better way?

[BigRoy]

I wonder why we do handle the frame changes of instance in extract review. For some reason? Shouldn't that happen when the frames are actually defined to be custom? Are handles currently set up automatically?

I agree here - if there's a bug without this here, I think the bug really is elsewhere and it should've been caught there instead.

I really tend to believe that each representation file should have information about filename frame and frame it represents during publishing (because it can be different), with that all these guesses would be gone.

Explicit is better than implicit. So when we can, I think this is good. Feel free to make an issue explaining how you envision this. What would define this, and where. What does it look like as data and who would use that information, etc.

[BigRoy]

my suggestion is to merge, wait if/what it breaks and based on that fix that in a better way?

Reasonable. It sounds unreasonable, but since what's there is technically wrong approach too - I think this is the way. We'll need to find the things that this was trying to catch and resolve them then, because the bug would be elsewhere.

[iLLiCiTiT]

Feel free to make an issue explaining how you envision this. What would define this, and where. What does it look like as data and who would use that information, etc.

Issue created #1950