Skip to content

Fix cue render order for ass/ssa. #2137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
May 9, 2025
Merged

Fix cue render order for ass/ssa. #2137

merged 14 commits into from
May 9, 2025

Conversation

peerless2012
Copy link
Contributor

This should fix #2124

  • Add layer in cue and output.
  • Add layer support for ssa parser.

@tonihei
Copy link
Collaborator

tonihei commented Feb 21, 2025

@icbaker Could you take a look? Thanks!

icbaker
icbaker reviewed Feb 25, 2025
View reviewed changes
@icbaker icbaker linked an issue Feb 25, 2025 that may be closed by this pull request
ASS render in a wrong order. #2124
Closed
1 task
@peerless2012 peerless2012 requested a review from icbaker February 26, 2025 16:38
@peerless2012
Copy link
Contributor Author

@icbaker hello, please make a review for the new commit, and hope this pr can merge to 1.6.0 final release.

@peerless2012 peerless2012 marked this pull request as draft March 25, 2025 02:06
@peerless2012 peerless2012 marked this pull request as ready for review March 25, 2025 02:07
@icbaker
Copy link
Collaborator

icbaker commented Apr 29, 2025

Please can you add some tests for this?

I suggest:

  1. Some parsing tests in SsaParserTest (including negative & unparseable layer values) - assert that the right value ends up in Cue.zIndex
  2. An end-to-end test in SubtitlePlaybackTest? This will cover both your parsing code and the re-ordering in the CueGroup constructor.

For (2) I suggest you craft an SSA test file that has cues overlapping in time with different layer values, to demonstrate why the re-ordering has to be done 'outside' SsaParser.

@peerless2012 peerless2012 mentioned this pull request May 6, 2025
Closed
@peerless2012
Copy link
Contributor Author

I add parseLayer and parseInvalidLayer in SsaParserTest to cover the layer value in ssa.

How can I add a test in SubtitlePlaybackTest? I have no idea.

@icbaker
Copy link
Collaborator

icbaker commented May 8, 2025

Thanks for adding the tests to SsaParserTest, I've added SsaPlaybackTest to cover the layer-based re-ordering inside CueGroup.

@icbaker
Copy link
Collaborator

icbaker commented May 8, 2025

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@peerless2012
Copy link
Contributor Author

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

Thanks a lot for your help. 🫶

@copybara-service copybara-service bot merged commit 909b560 into androidx:main May 9, 2025
1 check passed
@peerless2012 peerless2012 deleted the fix_cue_order branch May 9, 2025 14:51
@icbaker icbaker mentioned this pull request May 9, 2025
Closed
1 task
@peerless2012 peerless2012 mentioned this pull request May 20, 2025
Merged
@androidx androidx locked and limited conversation to collaborators Jul 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@icbaker icbaker Awaiting requested review from icbaker

Assignees

@icbaker icbaker

Labels
should merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ASS render in a wrong order.
3 participants
@peerless2012 @tonihei @icbaker