|
| 1 | +# Implementation Plan: Reimplement PR #1618 (Issue #1499) |
| 2 | + |
| 3 | +## Current Status |
| 4 | + |
| 5 | +| Phase | Status | Branch | PR | |
| 6 | +|-------|--------|--------|-----| |
| 7 | +| Phase 1: Rust Core | ✅ COMPLETE | `fix/1499-dtvcc-persistent-state` | #1782 | |
| 8 | +| Phase 2: C Headers | ✅ COMPLETE | `fix/1499-dtvcc-persistent-state` | #1782 | |
| 9 | +| Phase 3: C Implementation | 🔲 NOT STARTED | - | - | |
| 10 | +| Phase 4: Testing | 🔲 NOT STARTED | - | - | |
| 11 | + |
| 12 | +--- |
| 13 | + |
| 14 | +## Problem Statement |
| 15 | + |
| 16 | +The Rust CEA-708 decoder has a fundamental bug: the `Dtvcc` struct is recreated on every call to `ccxr_process_cc_data()`, causing all state to be reset. This breaks stateful caption processing. |
| 17 | + |
| 18 | +**Current buggy code** (`src/rust/src/lib.rs:175-176`): |
| 19 | +```rust |
| 20 | +let dtvcc_ctx = unsafe { &mut *dec_ctx.dtvcc }; |
| 21 | +let mut dtvcc = Dtvcc::new(dtvcc_ctx); // Created fresh EVERY call! |
| 22 | +``` |
| 23 | + |
| 24 | +## Solution Overview |
| 25 | + |
| 26 | +Create a persistent Rust `Dtvcc` instance stored in `lib_cc_decode.dtvcc_rust` that is: |
| 27 | +1. Initialized once at program start via `ccxr_dtvcc_init()` |
| 28 | +2. Used throughout processing via pointer access |
| 29 | +3. Freed at program end via `ccxr_dtvcc_free()` |
| 30 | + |
| 31 | +--- |
| 32 | + |
| 33 | +## Commit-by-Commit Analysis |
| 34 | + |
| 35 | +### Commit 1: `2a1fce53` - Change `Dtvcc::new()` signature |
| 36 | +**Status: NEEDED** |
| 37 | + |
| 38 | +Changes `Dtvcc::new()` to take `ccx_decoder_dtvcc_settings` instead of `dtvcc_ctx`. |
| 39 | + |
| 40 | +**Why needed:** The current implementation wraps the C `dtvcc_ctx`, but we need a standalone Rust struct that owns its data and doesn't depend on the C context. |
| 41 | + |
| 42 | +**Files:** `src/rust/src/decoder/mod.rs` |
| 43 | + |
| 44 | +**Key changes:** |
| 45 | +- Change `Dtvcc::new(ctx: &'a mut dtvcc_ctx)` to `Dtvcc::new(opts: &ccx_decoder_dtvcc_settings)` |
| 46 | +- Change `decoders` from `Vec<&'a mut dtvcc_service_decoder>` to `[Option<Box<dtvcc_service_decoder>>; CCX_DTVCC_MAX_SERVICES]` |
| 47 | +- Change `encoder` from `&'a mut encoder_ctx` to `*mut encoder_ctx` (set later) |
| 48 | +- Add `pub const CCX_DTVCC_MAX_SERVICES: usize = 63;` |
| 49 | + |
| 50 | +--- |
| 51 | + |
| 52 | +### Commit 2: `05c6a2f7` - Create `ccxr_dtvcc_init` & `ccxr_dtvcc_free` |
| 53 | +**Status: NEEDED** |
| 54 | + |
| 55 | +FFI functions to create and destroy the Rust Dtvcc from C code. |
| 56 | + |
| 57 | +**Files:** `src/rust/src/lib.rs` |
| 58 | + |
| 59 | +**Key changes:** |
| 60 | +```rust |
| 61 | +#[no_mangle] |
| 62 | +extern "C" fn ccxr_dtvcc_init<'a>(opts_ptr: *const ccx_decoder_dtvcc_settings) -> *mut Dtvcc<'a> |
| 63 | + |
| 64 | +#[no_mangle] |
| 65 | +extern "C" fn ccxr_dtvcc_free(dtvcc_rust: *mut Dtvcc) |
| 66 | +``` |
| 67 | + |
| 68 | +--- |
| 69 | + |
| 70 | +### Commit 3: `af23301b` - Create `ccxr_dtvcc_set_encoder` |
| 71 | +**Status: NEEDED** |
| 72 | + |
| 73 | +Allows setting the encoder on the Rust Dtvcc after initialization. |
| 74 | + |
| 75 | +**Files:** `src/rust/src/lib.rs` |
| 76 | + |
| 77 | +**Key changes:** |
| 78 | +```rust |
| 79 | +#[no_mangle] |
| 80 | +extern "C" fn ccxr_dtvcc_set_encoder(dtvcc_rust: *mut Dtvcc, encoder: *mut encoder_ctx) |
| 81 | +``` |
| 82 | + |
| 83 | +--- |
| 84 | + |
| 85 | +### Commit 4: `92cc3c54` - Add `dtvcc_rust` member to `lib_cc_decode` |
| 86 | +**Status: NEEDED** |
| 87 | + |
| 88 | +Storage for the Rust Dtvcc pointer in C struct. |
| 89 | + |
| 90 | +**Files:** `src/lib_ccx/ccx_decoders_structs.h` |
| 91 | + |
| 92 | +**Key changes:** |
| 93 | +```c |
| 94 | +struct lib_cc_decode { |
| 95 | + // ... existing fields ... |
| 96 | + void *dtvcc_rust; // For storing Dtvcc coming from rust |
| 97 | + dtvcc_ctx *dtvcc; |
| 98 | + // ... |
| 99 | +}; |
| 100 | +``` |
| 101 | + |
| 102 | +--- |
| 103 | + |
| 104 | +### Commit 5: `b9215f59` - Declare extern functions in C header |
| 105 | +**Status: NEEDED** |
| 106 | + |
| 107 | +C header declarations for the Rust FFI functions. |
| 108 | + |
| 109 | +**Files:** `src/lib_ccx/ccx_dtvcc.h` |
| 110 | + |
| 111 | +**Key changes:** |
| 112 | +```c |
| 113 | +#ifndef DISABLE_RUST |
| 114 | +extern void *ccxr_dtvcc_init(struct ccx_decoder_dtvcc_settings *settings_dtvcc); |
| 115 | +extern void ccxr_dtvcc_free(void *dtvcc_rust); |
| 116 | +extern void ccxr_dtvcc_process_data(void *dtvcc_rust, const unsigned char cc_valid, |
| 117 | + const unsigned char cc_type, const unsigned char data1, const unsigned char data2); |
| 118 | +#endif |
| 119 | +``` |
| 120 | + |
| 121 | +--- |
| 122 | + |
| 123 | +### Commit 6: `fce9fcfc` - Declare `ccxr_dtvcc_set_encoder` in C |
| 124 | +**Status: NEEDED** |
| 125 | + |
| 126 | +**Files:** `src/lib_ccx/lib_ccx.h` |
| 127 | + |
| 128 | +**Key changes:** |
| 129 | +```c |
| 130 | +#ifndef DISABLE_RUST |
| 131 | +void ccxr_dtvcc_set_encoder(void *dtvcc_rust, struct encoder_ctx* encoder); |
| 132 | +#endif |
| 133 | +``` |
| 134 | + |
| 135 | +--- |
| 136 | + |
| 137 | +### Commit 7: `e8a26608` - Use Rust init/free in C code |
| 138 | +**Status: NEEDED** |
| 139 | + |
| 140 | +Replace C dtvcc initialization with Rust version. |
| 141 | + |
| 142 | +**Files:** `src/lib_ccx/ccx_decoders_common.c` |
| 143 | + |
| 144 | +**Key changes:** |
| 145 | +- In `init_cc_decode()`: Call `ccxr_dtvcc_init()` instead of `dtvcc_init()` |
| 146 | +- In `dinit_cc_decode()`: Call `ccxr_dtvcc_free()` instead of `dtvcc_free()` |
| 147 | +- Use `#ifndef DISABLE_RUST` guards |
| 148 | + |
| 149 | +--- |
| 150 | + |
| 151 | +### Commit 8: `1ce5ad42` - Fix rust build |
| 152 | +**Status: LIKELY OBSOLETE** - Will need fresh fixes for current codebase |
| 153 | + |
| 154 | +--- |
| 155 | + |
| 156 | +### Commit 9: `cf2a335c` - Fix `is_active` value |
| 157 | +**Status: NEEDED** (part of commit 1 implementation) |
| 158 | + |
| 159 | +Ensures `dtvcc_rust.is_active` is set correctly from settings. |
| 160 | + |
| 161 | +--- |
| 162 | + |
| 163 | +### Commit 10: `6cb6f6e3` - Change `ccxr_flush_decoder` to use `dtvcc_rust` |
| 164 | +**Status: NEEDED** |
| 165 | + |
| 166 | +**Files:** |
| 167 | +- `src/rust/src/decoder/service_decoder.rs` |
| 168 | +- `src/rust/src/decoder/mod.rs` |
| 169 | +- `src/lib_ccx/ccx_decoders_common.h` |
| 170 | + |
| 171 | +**Key changes:** |
| 172 | +- Change `ccxr_flush_decoder` signature from raw pointers to Rust types |
| 173 | +- Add `ccxr_flush_active_decoders` extern C function |
| 174 | +- Remove old `ccxr_flush_decoder` extern declaration from C |
| 175 | + |
| 176 | +--- |
| 177 | + |
| 178 | +### Commit 11: `e8cf6ae2` - Fix double free issue |
| 179 | +**Status: NEEDED** (incorporated into memory management design) |
| 180 | + |
| 181 | +--- |
| 182 | + |
| 183 | +### Commit 12-13: `3f2d2f2e`, `e1ca3a66` - Convert code portions to rust |
| 184 | +**Status: NEEDED** |
| 185 | + |
| 186 | +Changes `do_cb` to get `dtvcc` from `ctx.dtvcc_rust` instead of parameter. |
| 187 | + |
| 188 | +**Files:** `src/rust/src/lib.rs` |
| 189 | + |
| 190 | +**Key changes:** |
| 191 | +```rust |
| 192 | +pub fn do_cb(ctx: &mut lib_cc_decode, cc_block: &[u8]) -> bool { |
| 193 | + let dtvcc = unsafe { &mut *(ctx.dtvcc_rust as *mut Dtvcc) }; |
| 194 | + // ... |
| 195 | +} |
| 196 | +``` |
| 197 | + |
| 198 | +Also modifies `ccxr_process_cc_data` to not create new Dtvcc. |
| 199 | + |
| 200 | +--- |
| 201 | + |
| 202 | +### Commit 14: `f8160d76` - Revert formatting issues |
| 203 | +**Status: OBSOLETE** - Was cleanup, not functionality |
| 204 | + |
| 205 | +--- |
| 206 | + |
| 207 | +### Commit 15: `4c50f436` - Clippy fixes |
| 208 | +**Status: OBSOLETE** - Will need fresh clippy fixes |
| 209 | + |
| 210 | +--- |
| 211 | + |
| 212 | +### Commit 16: `7a7471c3` - Use Rust functions in mp4.c |
| 213 | +**Status: NEEDED** |
| 214 | + |
| 215 | +**Files:** `src/lib_ccx/mp4.c` |
| 216 | + |
| 217 | +**Key changes:** |
| 218 | +```c |
| 219 | +#ifndef DISABLE_RUST |
| 220 | + ccxr_dtvcc_set_encoder(dec_ctx->dtvcc_rust, enc_ctx); |
| 221 | + ccxr_dtvcc_process_data(dec_ctx->dtvcc_rust, temp[0], temp[1], temp[2], temp[3]); |
| 222 | +#else |
| 223 | + dec_ctx->dtvcc->encoder = (void *)enc_ctx; |
| 224 | + dtvcc_process_data(dec_ctx->dtvcc, (unsigned char *)temp); |
| 225 | +#endif |
| 226 | +``` |
| 227 | + |
| 228 | +--- |
| 229 | + |
| 230 | +### Commit 17: `609c4d03` - Fix mp4 arguments & clippy |
| 231 | +**Status: OBSOLETE** - Bug fixes for commit 16, will be handled correctly |
| 232 | + |
| 233 | +--- |
| 234 | + |
| 235 | +### Commit 18: `991de144` - Fix memory leaks |
| 236 | +**Status: NEEDED** (incorporated into memory management design) |
| 237 | + |
| 238 | +--- |
| 239 | + |
| 240 | +### Commit 19: `294b9d91` - Fix unit test errors |
| 241 | +**Status: NEEDED** - Tests need to use new initialization |
| 242 | + |
| 243 | +--- |
| 244 | + |
| 245 | +### Commit 20: `1ccaf033` - Merge master |
| 246 | +**Status: OBSOLETE** - Was just a merge commit |
| 247 | + |
| 248 | +--- |
| 249 | + |
| 250 | +## Implementation Steps |
| 251 | + |
| 252 | +### Phase 1: Rust Core Changes ✅ COMPLETE |
| 253 | + |
| 254 | +> **Implementation Note:** Instead of modifying the existing `Dtvcc` struct (which would break |
| 255 | +> backward compatibility), we created a new `DtvccRust` struct. This allows the existing code |
| 256 | +> to continue working while the new persistent context is available for Phase 2-3. |
| 257 | +
|
| 258 | +**Step 1.1: Add new `DtvccRust` struct** ✅ |
| 259 | +- File: `src/rust/src/decoder/mod.rs` |
| 260 | +- Added `CCX_DTVCC_MAX_SERVICES` constant (63) |
| 261 | +- Created new `DtvccRust` struct with: |
| 262 | + - `decoders: [Option<Box<dtvcc_service_decoder>>; CCX_DTVCC_MAX_SERVICES]` (owned) |
| 263 | + - `encoder: *mut encoder_ctx` (set later via `set_encoder()`) |
| 264 | + - Raw pointers for `report` and `timing` (owned by C) |
| 265 | +- Implemented `DtvccRust::new(opts: &ccx_decoder_dtvcc_settings)` |
| 266 | +- Used heap allocation (`alloc_zeroed`) to avoid stack overflow with large structs |
| 267 | +- Added `set_encoder()`, `process_cc_data()`, `flush_active_decoders()` methods |
| 268 | + |
| 269 | +**Step 1.2: Add FFI functions in lib.rs** ✅ |
| 270 | +- File: `src/rust/src/lib.rs` |
| 271 | +- Added `ccxr_dtvcc_init()` - creates boxed DtvccRust, returns opaque `void*` |
| 272 | +- Added `ccxr_dtvcc_free()` - properly frees all owned memory (decoders, tv_screens, rows) |
| 273 | +- Added `ccxr_dtvcc_set_encoder()` - sets encoder pointer |
| 274 | +- Added `ccxr_dtvcc_process_data()` - processes CC data on existing DtvccRust |
| 275 | +- Added `ccxr_flush_active_decoders()` - flushes all active service decoders |
| 276 | +- Added `ccxr_dtvcc_is_active()` - helper to check if context is active |
| 277 | + |
| 278 | +**Step 1.3: Existing code unchanged** ✅ |
| 279 | +- The existing `Dtvcc` struct and `ccxr_process_cc_data()` remain unchanged |
| 280 | +- This ensures backward compatibility during the transition |
| 281 | +- Phase 2-3 will switch to using the new functions |
| 282 | + |
| 283 | +**Step 1.4: Add unit tests** ✅ |
| 284 | +- Added 5 new tests for `DtvccRust`: |
| 285 | + - `test_dtvcc_rust_new` - verifies initialization |
| 286 | + - `test_dtvcc_rust_set_encoder` - verifies encoder setting |
| 287 | + - `test_dtvcc_rust_process_cc_data` - verifies CC data processing |
| 288 | + - `test_dtvcc_rust_clear_packet` - verifies packet clearing |
| 289 | + - `test_dtvcc_rust_state_persistence` - **key test** verifying state persists across calls |
| 290 | +- All 176 tests pass (171 existing + 5 new) |
| 291 | + |
| 292 | +### Phase 2: C Header Changes ✅ COMPLETE |
| 293 | + |
| 294 | +**Step 2.1: Add `dtvcc_rust` to struct** ✅ |
| 295 | +- File: `src/lib_ccx/ccx_decoders_structs.h` |
| 296 | +- Added `void *dtvcc_rust;` field to `lib_cc_decode` struct |
| 297 | + |
| 298 | +**Step 2.2: Declare Rust FFI functions** ✅ |
| 299 | +- File: `src/lib_ccx/ccx_dtvcc.h` |
| 300 | +- Added extern declarations for `ccxr_dtvcc_init`, `ccxr_dtvcc_free`, `ccxr_dtvcc_process_data` |
| 301 | +- File: `src/lib_ccx/lib_ccx.h` |
| 302 | +- Added extern declaration for `ccxr_dtvcc_set_encoder` |
| 303 | +- File: `src/lib_ccx/ccx_decoders_common.h` |
| 304 | +- Added extern declaration for `ccxr_flush_active_decoders` |
| 305 | + |
| 306 | +### Phase 3: C Implementation Changes |
| 307 | + |
| 308 | +**Step 3.1: Update decoder initialization/destruction** |
| 309 | +- File: `src/lib_ccx/ccx_decoders_common.c` |
| 310 | +- In `init_cc_decode()`: Use `ccxr_dtvcc_init()` when Rust enabled |
| 311 | +- In `dinit_cc_decode()`: Use `ccxr_dtvcc_free()` when Rust enabled |
| 312 | +- In `flush_cc_decode()`: Use `ccxr_flush_active_decoders()` when Rust enabled |
| 313 | +- Remove old `ccxr_flush_decoder` extern declaration |
| 314 | + |
| 315 | +**Step 3.2: Update encoder assignment points** |
| 316 | +- File: `src/lib_ccx/general_loop.c` |
| 317 | +- Three locations need `ccxr_dtvcc_set_encoder()` calls |
| 318 | +- File: `src/lib_ccx/mp4.c` |
| 319 | +- Use `ccxr_dtvcc_set_encoder()` and `ccxr_dtvcc_process_data()` |
| 320 | + |
| 321 | +### Phase 4: Testing |
| 322 | + |
| 323 | +**Step 4.1: Update Rust unit tests** |
| 324 | +- File: `src/rust/src/lib.rs` (test module) |
| 325 | +- File: `src/rust/src/decoder/mod.rs` (test module) |
| 326 | +- Use new initialization pattern with `ccx_decoder_dtvcc_settings` |
| 327 | + |
| 328 | +**Step 4.2: Build verification** |
| 329 | +- Verify build with Rust enabled |
| 330 | +- Verify build with `DISABLE_RUST` |
| 331 | +- Run existing test suite |
| 332 | + |
| 333 | +**Step 4.3: Functional testing** |
| 334 | +- Test CEA-708 decoding with MP4 files |
| 335 | +- Test CEA-708 decoding with MPEG-TS files |
| 336 | +- Verify state persistence across multiple CC data blocks |
| 337 | + |
| 338 | +--- |
| 339 | + |
| 340 | +## Risk Areas |
| 341 | + |
| 342 | +1. **Memory management**: The Rust `Dtvcc` owns `Box`ed decoders and TV screens that must be properly freed |
| 343 | +2. **Lifetime issues**: The `'a` lifetime on `Dtvcc` references `timing` and `report` - ensure these outlive the Dtvcc |
| 344 | +3. **Thread safety**: Raw pointers are used; ensure single-threaded access |
| 345 | +4. **Bindgen compatibility**: Ensure `lib_cc_decode` bindings include the new `dtvcc_rust` field |
| 346 | + |
| 347 | +--- |
| 348 | + |
| 349 | +## Files Modified Summary |
| 350 | + |
| 351 | +### Phase 1 (Complete) |
| 352 | + |
| 353 | +| File | Type | Status | Changes | |
| 354 | +|------|------|--------|---------| |
| 355 | +| `src/rust/src/decoder/mod.rs` | Rust | ✅ | Added `DtvccRust` struct (+260 lines), tests (+120 lines) | |
| 356 | +| `src/rust/src/lib.rs` | Rust | ✅ | Added 6 FFI functions (+130 lines) | |
| 357 | + |
| 358 | +### Phase 2 (Complete) |
| 359 | + |
| 360 | +| File | Type | Status | Changes | |
| 361 | +|------|------|--------|---------| |
| 362 | +| `src/lib_ccx/ccx_decoders_structs.h` | C Header | ✅ | Added `dtvcc_rust` field | |
| 363 | +| `src/lib_ccx/ccx_dtvcc.h` | C Header | ✅ | Added extern declarations for init/free/process_data | |
| 364 | +| `src/lib_ccx/lib_ccx.h` | C Header | ✅ | Added `ccxr_dtvcc_set_encoder` declaration | |
| 365 | +| `src/lib_ccx/ccx_decoders_common.h` | C Header | ✅ | Added `ccxr_flush_active_decoders` declaration | |
| 366 | + |
| 367 | +### Phase 3 (Pending) |
| 368 | + |
| 369 | +| File | Type | Status | Changes | |
| 370 | +|------|------|--------|---------| |
| 371 | +| `src/lib_ccx/ccx_decoders_common.c` | C | 🔲 | Use Rust init/free/flush | |
| 372 | +| `src/lib_ccx/general_loop.c` | C | 🔲 | Set encoder via Rust function | |
| 373 | +| `src/lib_ccx/mp4.c` | C | 🔲 | Use Rust processing for MP4 | |
| 374 | + |
| 375 | +--- |
| 376 | + |
| 377 | +## Estimated Complexity |
| 378 | + |
| 379 | +- **Phase 1 (Rust)**: ✅ COMPLETE - Added new struct alongside existing code |
| 380 | +- **Phase 2 (C Headers)**: ✅ COMPLETE - Added field and extern declarations |
| 381 | +- **Phase 3 (C Implementation)**: Low-Medium - conditional compilation blocks |
| 382 | +- **Phase 4 (Testing)**: Medium - need to verify state persistence works correctly |
| 383 | + |
| 384 | +**Total estimate**: This is a significant change touching core decoder logic. Recommend implementing in small, testable increments. |
| 385 | + |
| 386 | +--- |
| 387 | + |
| 388 | +## Next Steps |
| 389 | + |
| 390 | +1. Create PR for Phase 1 (pending CI validation) |
| 391 | +2. After Phase 1 PR is merged, implement Phase 2 (C Headers) |
| 392 | +3. After Phase 2 PR is merged, implement Phase 3 (C Implementation) |
| 393 | +4. Full integration testing in Phase 4 |
0 commit comments