[FEAT] Port SRT encoder to Rust#2227
Open
DhanushVarma-2 wants to merge 1 commit intoCCExtractor:masterfrom
Open
[FEAT] Port SRT encoder to Rust#2227DhanushVarma-2 wants to merge 1 commit intoCCExtractor:masterfrom
DhanushVarma-2 wants to merge 1 commit intoCCExtractor:masterfrom
Conversation
DhanushVarma-2
force-pushed
the
feat/srt-encoder-rust
branch
7 times, most recently
from
d9b3a3a to
c6523ce
Compare
Contributor
Author
Output
|
DhanushVarma-2
force-pushed
the
feat/srt-encoder-rust
branch
5 times, most recently
from
af58374 to
a1dc78a
Compare
Contributor
Author
|
The bitmap function can't be wired right now. CMake sets ENABLE_HARDSUBX for C code but never calls corrosion_set_features to pass hardsubx_ocr to the Rust crate. So the Rust function doesn't get compiled in hardsubx builds, and wiring it from C causes a linker error....that's what happened when the OCR Docker CI failed earlier. The Rust code is there and ready. To actually wire it, someone needs to add corrosion_set_features(ccx_rust hardsubx_ocr) inside the WITH_HARDSUBX block in CMakeLists.txt. I'll do that as a follow-up PR to keep this one clean. And yes get_teletext_output and get_teletext_srt_counter both exist in ccx_encoders_common.c (lines 1356 and 1460) |
Member
steel-bucket
left a comment
steel-bucket
left a comment
There was a problem hiding this comment.
Compare Master Branch SP test results and yours
Comment on lines
+9
to
+11
| #ifndef DISABLE_RUST | ||
| extern int ccxr_write_stringz_as_srt(const char *string, struct encoder_ctx *context, LLONG ms_start, LLONG ms_end); | ||
| extern int ccxr_write_cc_buffer_as_srt(struct eia608_screen *data, struct encoder_ctx *context); |
Member
There was a problem hiding this comment.
DISABLE_RUST is deprecated
Comment on lines
+10
to
+18
| extern "C" { | ||
| fn get_decoder_line_encoded( | ||
| ctx: *mut encoder_ctx, | ||
| buffer: *mut c_uchar, | ||
| line_num: c_int, | ||
| data: *const eia608_screen, | ||
| ) -> c_uint; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Import Externs from the way we do in src/rust/src/lib.rs, not this way
DhanushVarma-2
force-pushed
the
feat/srt-encoder-rust
branch
2 times, most recently
from
43ef45f to
043901b
Compare
Implement ccxr_write_stringz_as_srt and ccxr_write_cc_buffer_as_srt in src/rust/src/encoder/srt.rs. Covers: - Subtitle counter and timestamp formatting with -1ms overlap prevention - \n unescape handling for multi-line subtitles - Encoding conversion (UTF-8, Latin1, UCS-2) - Autodash detection for CEA-608 screen buffers - Speaker name detection (colon-based) Uses existing Rust encoder infrastructure (encode_line, write_wrapped) and calls C get_decoder_line_encoded for CEA-608 line encoding until that function is also ported. Exported as #[no_mangle] extern C functions ready to replace the C versions in ccx_encoders_srt.c.
DhanushVarma-2
force-pushed
the
feat/srt-encoder-rust
branch
from
043901b to
c591f90
Compare
Contributor
Author
|
@steel-bucket can you review it again |
Collaborator
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit d56a6be...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
Collaborator
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit d56a6be...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
Comment on lines
+1
to
+2
| use crate::bindings::{ccx_encoding_type_CCX_ENC_UNICODE, eia608_screen, encoder_ctx}; | ||
| use crate::encoder::common::encode_line; |
Member
There was a problem hiding this comment.
don't import bindings outside of libccxr_exports
Comment on lines
+27
to
+53
| /// Helper: copy CRLF bytes from context so we don't hold a borrow. | ||
| unsafe fn copy_crlf(ctx: &encoder_ctx) -> Vec<u8> { | ||
| let len = ctx.encoded_crlf_length as usize; | ||
| let slice = std::slice::from_raw_parts(ctx.encoded_crlf.as_ptr(), len); | ||
| slice.to_vec() | ||
| } | ||
|
|
||
| /// Helper: encode into a fresh Vec, avoiding borrow conflicts with ctx.buffer. | ||
| unsafe fn encode_to_vec(ctx: &mut encoder_ctx, text: &[u8]) -> Vec<u8> { | ||
| let cap = ctx.capacity as usize; | ||
| let buf = std::slice::from_raw_parts_mut(ctx.buffer, cap); | ||
| let used = encode_line(ctx, buf, text) as usize; | ||
| buf[..used].to_vec() | ||
| } | ||
|
|
||
| /// Core SRT writer — writes a single subtitle entry to a specific output. | ||
| /// | ||
| /// # Safety | ||
| /// Accesses raw pointers from encoder context. | ||
| pub unsafe fn write_stringz_as_srt_to_output( | ||
| string: *const i8, | ||
| context: &mut encoder_ctx, | ||
| ms_start: i64, | ||
| ms_end: i64, | ||
| out_fh: c_int, | ||
| srt_counter: *mut u32, | ||
| ) -> c_int { |
Member
There was a problem hiding this comment.
All of this is C-like, The underlying thought while doing these migrations, is that when we finally move to Rust completely we can't still pass raw pointers around
Comment on lines
+495
to
+506
| struct CcBitmap { | ||
| x: c_int, | ||
| y: c_int, | ||
| w: c_int, | ||
| h: c_int, | ||
| nb_colors: c_int, | ||
| data0: *mut std::os::raw::c_uchar, | ||
| data1: *mut std::os::raw::c_uchar, | ||
| linesize0: c_int, | ||
| linesize1: c_int, | ||
| } | ||
| let data_ptr = s.data as *mut CcBitmap; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ported all 5 functions from ccx_encoders_srt.c to Rust:
Rust is called by default. C fallback behind #ifdef DISABLE_RUST.
Tested on Matroska test files (Elephant Dreams) with 8 subtitle tracks
including English and Hungarian with UTF-8 accented characters.
All extract correctly.