Skip to content
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.

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

Fix missing and semi-transparent events in oneshot render #21

Closed
wants to merge 5 commits into from

Conversation

dmitrylyzo
Copy link
Collaborator

@dmitrylyzo dmitrylyzo commented Oct 8, 2021

Changes

  • Fix a lost last event
  • Fix event tag parsing for correct animation detection
    • \ starts new tag
    • ) completes the tag
    • ; is a generic character (not a delimiter)
    • ignore spaces after the tag: \move (...), \t (...)
    • ignore unclosed parentheses: \move(..., \t(... at the end of override block
      won't work for \t(...\t(...
  • Leave transition result after dropping animations
  • Fix array comparison

The parser is far from perfect, but I think we need to fix the semi-transparent subtitles issue in our fork until the upstream is ready.

a short review on parser - first sentence.
Looking at the revised “Fix event tag parsing” I can again see more parsing mistakes in the newly touched code (eg using locale-dependent functions, assuming parentheses can be nested, and more).

isalnum and isspace can be replaced with our own, but I see no reason to do this for nearly valid subtitles (which don't use non-Latin characters in override blocks).
I don't know if nested parentheses are ignored, but again, this doesn't harm for valid subtitles.

The upstream goes a different way - it now uses a simplified parser (without analyzing move).

New parser in action
// clear - dropAllAnimations disabled
// drop - dropAllAnimations enabled
// -> 1/0 - animated or not

clear: Move (with tabulator) {\move	(50,50,1230,670)} -> 1
drop:  Move (with tabulator) {                      } -> 0
clear: {\c1&H000000&\alpha&HAA}Move (with unclosed block) {\move(50,50,1230,670) -> 1
drop:  {\c1&H000000&\alpha&HAA}Move (with unclosed block) {                      -> 0
clear: {\notatag \fs50 \t     (\frz180}Transform (with space) -> 1
drop:  {\notatag \fs50         \frz180}Transform (with space) -> 0
clear: {\frz30 aa \fad(100,500) \fs120}Fad simple -> 1
drop:  {\frz30 aa               \fs120}Fad simple -> 0
clear: {\frz-30 b \fade(50,255,0,10,300,700,900) \fs121}Fade complex -> 1
drop:  {\frz-30 b                                \fs121}Fade complex -> 0
clear: いろは{\kf128}笑{\kf47}え{\kf24}ば {\kf91}空{\kf58}ま{\kf27}で -> 1
drop:  いろは{      }笑{     }え{     }ば {     }空{     }ま{     }で -> 0
clear: にほえと{\ko128}笑{\ko47}え{\ko24}ば {\ko91}空{\ko58}ま{\ko27}で -> 1
drop:  にほえと{      }笑{     }え{     }ば {     }空{     }ま{     }で -> 0
clear: ちりぬるを{\K128}笑{\k47}え{\K24}ば {\k91}空{\K58}ま{\k27}で -> 1
drop:  ちりぬるを{     }笑{    }え{    }ば {    }空{    }ま{    }で -> 0
clear: {\shad5\bord5\fnIPAMincho}週間 -> 0
drop:  {\shad5\bord5\fnIPAMincho}週間 -> 0
clear: {\an2}Not animated due to libass ext \{\move(1,1,500,500)} -> 0
drop:  {\an2}Not animated due to libass ext \{\move(1,1,500,500)} -> 0
clear: {\an8}{\bord3}Animated {\move(1,1,500,500)} with unusual{\1c&H00FF00&}ly placed {\r}tags -> 1
drop:  {\an8}{\bord3}Animated {                  } with unusual{\1c&H00FF00&}ly placed {\r}tags -> 0
clear: {\notatag \fs50 \t     (\frz180)}Transform 2 (with space) -> 1
drop:  {\notatag \fs50         \frz180)}Transform 2 (with space) -> 0
clear: {\notatag \fs50 \t     ((\frz180)}Transform 3 (with space) -> 1
drop:  {\notatag \fs50          \frz180)}Transform 3 (with space) -> 0
clear: {\notatag \fs50 \t (500, 2000, \frz180)}Transform 4 (with space) -> 1
drop:  {\notatag \fs50                \frz180)}Transform 4 (with space) -> 0
clear: {\t(0,130,\1c&HFFFFFF&\t(65,565,\fscy0)}Transform 5 -> 1
drop:  {         \1c&HFFFFFF&\t(65,565,\fscy0)}Transform 5 -> 0
clear: {\t(0,120,\1c&H00FFFF&\t\frz180)}Transform 6 -> 1
drop:  {         \1c&H00FFFF&\t\frz180)}Transform 6 -> 0

The animation for Transform 5 is incorrectly dropped (but detected correctly) because \t( starts a new transition (libass#111 (comment)), but the parser is "in brackets". We don't use dropAllAnimations and the old parser didn't detect animation for these samples.

Issues
jellyfin/jellyfin-web#2291
Due to incorrect parsing \fad and \fade are not considered animated and are displayed in a semi-transparent state.

@dmitrylyzo dmitrylyzo changed the title Fix oneshot render 2 Fix missing and semi-transparent events in oneshot render Oct 8, 2021
@dmitrylyzo dmitrylyzo marked this pull request as ready for review October 8, 2021 21:24
@dmitrylyzo dmitrylyzo marked this pull request as draft October 9, 2021 21:59
@dmitrylyzo dmitrylyzo force-pushed the fix-oneshot-render-2 branch 3 times, most recently from 27d8d0e to a9ee988 Compare October 16, 2021 23:47
@dmitrylyzo dmitrylyzo marked this pull request as ready for review October 17, 2021 00:04
If there are no more events, 'minStart' is '-1', but we need to finish the last event with a valid time.
Otherwise it will be treated as the end of events (blank).
@dmitrylyzo
Copy link
Collaborator Author

Superseded by #24, #26 for the new master branch.

@dmitrylyzo dmitrylyzo closed this May 14, 2022
@dmitrylyzo dmitrylyzo deleted the fix-oneshot-render-2 branch May 16, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant