[ANALYSIS] current_fps initialization complexity - potential refactoring needed? #860

New Issue

claunia · 2026-01-29T16:55:34Z

claunia commented

2026-01-29 16:55:34 +00:00

Originally created by @Harshdhall01 on GitHub (Dec 25, 2025).

Description:

While investigating the TODO at src/lib_ccx/ccx_common_timing.c:22, I discovered that the issue is more complex than the comment suggests.

Current TODO:

double current_fps = (double)30000.0 / 1001; /* 29.97 */ 
// TODO: Get from framerates_values[] instead

What I Found:

I traced all usages of current_fps and found it's used in 40+ locations across the codebase:

~20 uses in C code (src/lib_ccx/)
~20 uses in Rust code (src/rust/)

Actual Behavior:

current_fps is:

Initialized to 29.97 fps (hardcoded constant)
Updated dynamically during video processing from multiple locations:
- avc_functions.c:888 - from H.264/AVC timing parameters
- avc_functions.c:988 - from framerates_values[] array
- es_functions.c:442 - from framerates_values[] array
- Similar updates in Rust code

The Real Issue:

The TODO comment is misleading. The variable does get values from framerates_values[] during runtime. The actual issues are:

Should the initial value use framerates_values[4] instead of the hardcoded calculation?
Is a global mutable variable the right design pattern for FPS management?
Should this be part of a context structure instead of global state?

Why This Matters:

current_fps affects timing calculations throughout the codebase:

Subtitle synchronization timing
Frame counting and indexing
PTS (Presentation Time Stamp) calculations
GOP (Group of Pictures) timing

Incorrect FPS handling could cause subtitle sync issues.

Questions for Maintainers:

Is the current design (global variable + runtime updates) intentional and acceptable?
Should we refactor this to use a context-based approach for better encapsulation?
Is this already a known issue that's been deferred?
Would this be appropriate scope for a GSoC 2026 project focused on the 1.00 release?

Proposed Solutions:

Option A (Simple): Update the comment to accurately document current behavior
Option B (Medium): Initialize from framerates_values[4] but keep global design
Option C (Complex): Refactor to context-based FPS management (major architectural change)

What's the preferred approach?

Originally created by @Harshdhall01 on GitHub (Dec 25, 2025). **Description:** While investigating the TODO at `src/lib_ccx/ccx_common_timing.c:22`, I discovered that the issue is more complex than the comment suggests. **Current TODO:** ```c double current_fps = (double)30000.0 / 1001; /* 29.97 */ // TODO: Get from framerates_values[] instead ``` **What I Found:** I traced all usages of `current_fps` and found it's used in **40+ locations** across the codebase: - ~20 uses in C code (`src/lib_ccx/`) - ~20 uses in Rust code (`src/rust/`) **Actual Behavior:** `current_fps` is: 1. **Initialized** to 29.97 fps (hardcoded constant) 2. **Updated dynamically** during video processing from multiple locations: - `avc_functions.c:888` - from H.264/AVC timing parameters - `avc_functions.c:988` - from `framerates_values[]` array - `es_functions.c:442` - from `framerates_values[]` array - Similar updates in Rust code The Real Issue: The TODO comment is misleading. The variable **does** get values from `framerates_values[]` during runtime. The actual issues are: 1. Should the **initial value** use `framerates_values[4]` instead of the hardcoded calculation? 2. Is a global mutable variable the right design pattern for FPS management? 3. Should this be part of a context structure instead of global state? Why This Matters: `current_fps` affects timing calculations throughout the codebase: - Subtitle synchronization timing - Frame counting and indexing - PTS (Presentation Time Stamp) calculations - GOP (Group of Pictures) timing Incorrect FPS handling could cause subtitle sync issues. Questions for Maintainers: 1. Is the current design (global variable + runtime updates) intentional and acceptable? 2. Should we refactor this to use a context-based approach for better encapsulation? 3. Is this already a known issue that's been deferred? 4. Would this be appropriate scope for a GSoC 2026 project focused on the 1.00 release? Proposed Solutions: - **Option A (Simple):** Update the comment to accurately document current behavior - **Option B (Medium):** Initialize from `framerates_values[4]` but keep global design - **Option C (Complex):** Refactor to context-based FPS management (major architectural change) What's the preferred approach?

claunia closed this issue

2026-01-29 16:55:35 +00:00

claunia commented

2026-01-29 16:55:36 +00:00

@cfsmp3 commented on GitHub (Dec 26, 2025):

Damn, we can use AI too :-)

For now we want to tackle existing real issues (reported by users) and complete the transition to Rust.

@cfsmp3 commented on GitHub (Dec 26, 2025): Damn, we can use AI too :-) For now we want to tackle existing real issues (reported by users) *and* complete the transition to Rust.

claunia referenced this issue

2026-01-29 17:18:01 +00:00

[PR #860] [CLOSED] README FILE #1694

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/ccextractor#860