TheAcharya / MarkersExtractor

Extract Markers from Final Cut Pro FCPXML
MIT License
36 stars 2 forks source link

Bug in Timeline with Mixed Frame Rate Clips #81

Closed IAmVigneswaran closed 5 months ago

IAmVigneswaran commented 9 months ago

Steffan,

I notice a bug in frame / image extraction of 23.98 video file. For some reason, the marker position is not respected. There is an offset of 23 frames back.

In Timeline

Marker-Position

Extracted Image 23_98_V1_00_00_53_15

It does not happened to all markers but randomly on some markers.

orchetect commented 9 months ago

See if you can narrow down the symptoms. Are there mixed frame rates in the project? What clip type is it and what is its frame rate? What is the project frame rate?

IAmVigneswaran commented 9 months ago

Are there mixed frame rates in the project?

Yes.

What clip type is it and what is its frame rate?

The Clip Type is on Audition. The Frame Rate of the original clip is 25 FPS.

audition-clip-type

What is the project frame rate?

23.98 FPS.

The offset bug only occur on a clip that has an original 25 FPS as per my sample timeline.

offset-bug
IAmVigneswaran commented 9 months ago

Second Example - With Project Frame Rate 60 FPS. Having a mixed clips of 29.97 and 23.98.

It occurs for some markers.

60FPS-Timeline
orchetect commented 9 months ago

I'm not able to reproduce this. Can you email me the projects for both of these examples?

orchetect commented 9 months ago

Ok I've confirmed the issue with the sample projects. Will see if I can get a fix out this week.

orchetect commented 9 months ago

On analyzing the FCPXML, the numbers just don't make sense.

I'll have to shelve this and come back to it because I just can't see how FCP is coming up with the time values it's using in the XML. (I've done a lot of mixed-framerate parsing already with FCPXML -- and everything has made sense so far, but not this.)

latenitefilms commented 9 months ago

Unrelated, but also something to keep in mind... FCPX doesn't do any DTD validation on export, so FCPX can itself export invalid FCPXML:

https://github.com/CommandPost/FCPCafe/issues/314

orchetect commented 9 months ago

FCPX can itself export invalid FCPXML

Yes true. This isn't a DTD violation thankfully, it's the actual time value math that isn't adding up. And yet importing the XML back into FCPX imports everything to correct time positions. So the answer is somewhere, just needs to be found. Even accounting for frame rate scaling/conversion, no combination of operations seems to work out to the expected values in specific cases.

TL;DR: In a project where the project frame rate differs from video asset frame rate, marker start times within an audition clip seem incorrect, no matter how you figure the frame rate conversion/scaling. All other clip types are fine and I can parse as expected. I want to say there's an offset or some piece of information missing from the XML, but again, FCPX knows how to interpret the very same XML on import. Puzzling.

latenitefilms commented 9 months ago

Can you share a FCPXML with the issue?

orchetect commented 9 months ago

I think I've cracked the nut. FCPX is doing frame rate scaling in a way I didn't anticipate.

ie: In a project at 23.98 fps, an asset-clip which references media at 25fps may look like this:

<asset-clip ref="r6" name="TestVideo2" duration="646646/24000s" format="r7" tcFormat="NDF" audioRole="dialogue">
    <conform-rate srcFrameRate="25"/>
    <marker start="177/25s" duration="1001/24000s" value="Audition 1"/>
    <marker start="344/25s" duration="1001/24000s" value="Audition 2"/>
    <marker start="22s" duration="1001/24000s" value="Audition 3"/>
</asset-clip>

"r6" is a 25fps asset, "r7" is a "FFVideoFormat1080p25" format.

Time values within the asset-clip must be scaled based on source (local timeline) frame rate and parent (sequence/spine) frame rate because of the presence of an implicitly-enabled conform-rate.

<marker start="177/25s" ...

177/25s here cannot be used as a literal value (7.08 seconds) and must be scaled.

orchetect commented 9 months ago

I have made the corresponding refactors and unit tests in DAWFileKit 0.4.1.

The fix will propagate to MarkersExtractor 0.3.2.

IAmVigneswaran commented 9 months ago

Just tested. I am getting some strange results. In our Marker FPS Test library, I have tested with the 60_V1 timeline.

The timecode does not line up.

60FPS-Error

But when testing with the 23.98_V1 timeline; the timecode is correct and they line-up.

23-98-FPS-Data
orchetect commented 9 months ago

Ok I will pull open that project and see what's going on.

orchetect commented 9 months ago

After spending all evening hacking at this, it appears the frame rate scaling is way more nuanced than it first seemed.

The scaling is not a simple formula between two rates, but varies depending on the nature of the individual rates and how they relate to each other.

I've refactored DAWFileKit to resolve the issues with both sample projects above (23.98_V1 and 60_V1) but they only involve a handful of rates. It's a step forward but not a universal fix just yet.

The chart found here is not quite aligning with real-world results that I'm seeing in practice. (For example, the chart indicates that no scaling happens between 23.98 and 60 but manual verification shows that scaling is in fact being performed and is required.) It would be more useful if they could show pull-up/pull-down rates instead.

For that reason I will need to manually verify and unit test as many rate combinations as possible and see if any patterns emerge. There are rate groupings that will likely share similar scaling factors.

orchetect commented 9 months ago

Thus far I've been able to implement correct scaling and unit tests for the following:

Project rate Containing media at rate(s)
23.98 23.98 and 29.97
24 23.98 and 29.97
29.97 23.98 and 29.97
29.97 drop 23.98 and 29.97
30 23.98 and 29.97
59.94 23.98 and 29.97
60 23.98 and 29.97

In theory, the reciprocals should also be functional (ie: for any given test above, swap project rate with any of its containing media rates) Which means all of these scenarios should also be working, but have not been explicitly unit tested:

Project rate Containing media at rate(s)
23.98 23.98, 24, 29.97, 29.97-drop, 30, 59.94, and 60
29.97 23.98, 24, 29.97, 29.97-drop, 30, 59.94, and 60
IAmVigneswaran commented 9 months ago

Seems to be working with our FPS testing timelines. I will close this issue for now.

We can revisit or open this in future if any users reports on this issue.

Thank you Steffan!

latenitefilms commented 7 months ago

Did you try putting 25.0fps clips in a 24.0fps timeline?

IAmVigneswaran commented 7 months ago

@latenitefilms Thanks for pointing out.

@orchetect Looks like 25FPS clips on 24FPS Timeline is not supported yet.

25 Clip on 24 Timeline - 01 25 Clip on 24 Timeline- 02
orchetect commented 7 months ago

Not all rate combinations have been tested for scaling yet so some may be incorrect.

latenitefilms commented 7 months ago

I've tried to get my head around a 25fps clip on a 24fps timeline, but even with this simple example (at the bottom of the page), I'm left very tired and confused:

https://fcp.cafe/developers/fcpxml/#getting-source-timecode

orchetect commented 7 months ago

In DAWFileKit it's just a scaling factor I need to add. Doesn't take long to work it out but I need to create a test project first and add it to the unit tests which takes a while.

latenitefilms commented 7 months ago

Yeah, the only reason I stumbled across this is because I was looking at your _fcpConformRateScalingFactor code.

You did have this:

case .whole: // 24, 25, 30, 60
            switch timelineFrameRate.compatibleGroup {
            case .ntscColor: 
                return t / m // works for 23.98 timeline -> 25 media

...but t / m doesn't seem to work for 24.0 timeline -> 25.0 media.

orchetect commented 7 months ago

24 → 25 would be .whole to .whole which currently returns a nil scaling factor because it's one I haven't reverse-engineered yet. That's where I need to add special cases.

orchetect commented 7 months ago

t / m doesn't seem to work for 24.0 timeline -> 25.0 media

Just added unit test. Incidentally t / m is the correct solution on my end. But it may be that your methods aren't taking all calculations into account in the chain. Lots of aggregate variables.

I'll push a fix soon that adds 24 t/l with 25 media soon.

In theory this should also add support for 25 t/l with 24 media, but not explicitly tested yet.

I've added a supported scaling rate pairs chart to this DAWFileKit issue thread for reference.

latenitefilms commented 7 months ago

Ah, interesting! I actually tried with DAWFileKit and forced the calculation t/m, but must have made a mistake somewhere.

I think maybe the issue with the FCP Cafe post is that I'm basing things off the seconds value, but maybe it needs to rounded up to the nearest actual video frame?