[FEATURE] Add guarded ASS/SSA \pos positioning for CEA-608 captions #1885
Conversation
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for this feature! The implementation is well-designed with good safeguards. Testing confirms it works correctly.
✅ What's Good
- Conservative guard: only applies positioning for 1-2 row captions
- Falls back gracefully to legacy behavior for complex layouts (>2 rows)
- Uses standard SSA resolution (384×288) with explicit header declaration
- Positioning math is correct: Row 0 → y=172 (60%), Row 14 → y=273 (95%)
- Doesn't affect SRT or other output formats
- All CI checks pass, all 248 Rust tests pass
Testing Results
Tested with sample 56c9f345... from Sample Platform:
- 118 dialogues correctly received
{\an2\pos(x,y)}tags - 1 dialogue with >2 rows correctly fell back to legacy behavior
- Compared with master: changes are purely additive
📋 One Minor Fix Needed
CHANGES.TXT entry is truncated:
- New: Added ASS/SSA \pos-based positioning for CEA-608 captions when layout
Please complete the sentence, e.g.:
- New: Added ASS/SSA \pos-based positioning for CEA-608 captions when layout is simple (1-2 rows) (#1726)
Once fixed, this is ready to merge! 🎉
|
Note: The approval above is conditional on the Sample Platform CI tests passing. The tests were still queued at the time of review:
Please wait for these to complete before merging. If any tests fail, the issues should be addressed first. |
|
Thanks for the review. I’ve fixed the truncated changelog entry and updated the loops to use scoped variables, and pushed the updated commit. |
x15sr71
force-pushed
the
feat/ssa-ass-precise-positioning
branch
from
34cd891 to
9ad9155
Compare
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
The implementation looks good! The positioning is working correctly and the guard for 1-2 row captions is a sensible approach to avoid regressions.
The Windows CI failure is expected since the ASS output format changed - we'll need to update the test baselines.
One small fix needed: the CHANGES.TXT entry appears incomplete:
- New: Added ASS/SSA \pos-based positioning for CEA-608 captions when layout
Please complete this sentence (e.g., "...when layout information is available" or similar).
Once that's fixed, we can merge and update the test baselines.
x15sr71
force-pushed
the
feat/ssa-ass-precise-positioning
branch
from
50a8540 to
5dd1961
Compare
|
@cfsmp3, I've completed the |
|
Hi @x15sr71, the CHANGES.TXT entry appears to be cut off mid-sentence. Could you complete it? |
|
Hi @cfsmp3, thanks for checking again.
Please let me know if you’d prefer any wording changes, as I might be misunderstanding something. |
|
Hi @cfsmp3, quick ping in case this got buried, no rush on my side. Happy to help with any adjustments or test updates if that helps move this forward. |
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Deep Review of PR #1885
I've thoroughly analyzed the math, built the PR, and tested with multiple CEA-608 samples.
What Works
- PlayResX/PlayResY header: Correctly added (384×288, SSA default)
- Single-line captions: Positioning is correct
- Guard for 3+ rows: Falls back to legacy behavior appropriately
Issue 1: Math Error for 2-Line Captions
The code uses first_row with \an2 (bottom-center alignment), which is incorrect for multi-row captions.
The problem:
\an2places the bottom of the text block at the specified Y coordinate- For a 2-line caption at rows 13-14:
first_row = 13,y = 265- The bottom line (row 14's text) ends at y=265
- But row 14 should be at y=273
- Error: 8 pixels (2.8% of screen)
Evidence from test output:
Dialogue: 0,00:00:21.98,00:00:24.23,Default,,0000,0000,0000,,{\an2\pos(192,265)}(both)\NWOW!
With \an2, "WOW!" (the bottom line, row 14) has its bottom at y=265, but row 14's calculated position is y=273.
Fix: Use the last used row instead of the first:
// Current (lines 254-260):
for (i = 0; i < 15; i++)
if (data->row_used[i]) { first_row = i; break; }
// Should be:
int last_row = -1;
for (i = 14; i >= 0; i--)
if (data->row_used[i]) { last_row = i; break; }Issue 2: Non-Adjacent Rows Would Be Catastrophically Wrong
The code assumes used_row_count <= 2 means a "single logical caption region" but doesn't verify the rows are adjacent.
Worst case: Rows 3 and 14 both used (rare but possible):
first_row = 3,y = 193- BOTH lines positioned with bottom at y=193
- Row 14 text would be misplaced by 80 pixels (27.8% of screen)
Suggested fix: Either:
- Check that the two rows are adjacent before applying positioning, OR
- Only apply positioning when
used_row_count == 1
Issue 3: Changelog Entry in Wrong Version
The changelog entry is added under 0.96.2 (2025-12-26) which is a past release. It should be moved to the current development version section (0.96.5 or whatever is current at merge time).
Row-to-Y Mapping Reference
For reviewers, here's the complete mapping with PlayResY=288:
row 0 → y=172 (59.7%) row 8 → y=229 (79.5%)
row 1 → y=179 (62.2%) row 9 → y=236 (81.9%)
row 2 → y=186 (64.6%) row 10 → y=244 (84.7%)
row 3 → y=193 (67.0%) row 11 → y=251 (87.2%)
row 4 → y=200 (69.4%) row 12 → y=258 (89.6%)
row 5 → y=208 (72.2%) row 13 → y=265 (92.0%)
row 6 → y=215 (74.7%) row 14 → y=273 (94.8%)
row 7 → y=222 (77.1%)
Summary
The feature concept is good and addresses issue #1726, but the positioning math needs correction before merging. The error is small for adjacent rows (~3%) but could be severe for edge cases.
… clean up variable placement
|
Thanks for the detailed review and for the helpful fix suggestions. I’ve applied the suggested fixes to the ASS positioning logic (anchor correction + row adjacency guard) and did a small cleanup around variable initialization. I validated the changes using this sample from the Sample Platform. I’ve attached the newly generated .ass file here for review and moved the changelog entry to the correct section. Let me know if anything else needs adjusting! |
x15sr71
force-pushed
the
feat/ssa-ass-precise-positioning
branch
from
faa8444 to
ac22206
Compare
x15sr71
force-pushed
the
feat/ssa-ass-precise-positioning
branch
from
e20497d to
4f6a832
Compare
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 2028754...:
Your PR breaks these cases:
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. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 2028754...:
Your PR breaks these cases:
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. |
3 participants
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
[FEATURE] - Add ASS/SSA \pos positioning for CEA-608 captions (feature request #1726)
Summary
Implements precise positioning for ASS/SSA subtitle export using
\pos(x,y)tags, replacing whitespace-based layout where sufficient positioning information is available.Fixes #1726
Changes
ass_position_from_row()function to map CEA-608 rows to ASS coordinates{\an2\pos(x,y)}positioning tags for single-region captions (1-2 rows)Design Decisions
Positioning is intentionally conservative:
avoid unsafe display areas
Resolution choice:
This improves visual consistency without changing caption semantics or risking regressions on complex roll-up layouts.
Testing
Future Enhancements
Fuller grid-based positioning and horizontal placement derived from CEA-608 semantics; however, this involves additional
interpretation of PAC data and roll-up behavior and is intentionally out of scope for this change.