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 animation detection #26

Merged
merged 1 commit into from
May 13, 2022

Conversation

dmitrylyzo
Copy link
Collaborator

@dmitrylyzo dmitrylyzo commented May 5, 2022

This is a version of the upstream maintainer - a simplified parser (without analyzing no-op move).
libass#111 (comment)
libass#111 (comment)

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) -> 0
drop:  {\c1&H000000&\alpha&HAA}Move (with unclosed block) {\move(50,50,1230,670) -> 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&          \fscy0)}Transform 5 -> 0
clear: {\t(0,120,\1c&H00FFFF&\t\frz180)}Transform 6 -> 1
drop:  {         \1c&H00FFFF&\t\frz180)}Transform 6 -> 0
clear: {\move	(50,50,1230,670\t(\1c&H00FFFF&\frz180)}Transform 7 -> 1
drop:  {                        \1c&H00FFFF&\frz180)}Transform 7 -> 0
clear: {\t(\1c&H00FFFF&\t(\frz180)}Transform 8 -> 1
drop:  {   \1c&H00FFFF&   \frz180)}Transform 8 -> 0

In my version I kept the in-depth analysis of the move tag (to skip no-op \move(1,2,1,2)), but my parser has parsing mistakes.

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).

Therefore, simpler is more reliable.

Changes

  • Simplified event parser.
  • No in-depth analysis of the move tag.

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 marked this pull request as ready for review May 6, 2022 18:24
@dmitrylyzo dmitrylyzo force-pushed the fix-animated-events branch from ae33183 to b619f0b Compare May 7, 2022 16:19
@dmitrylyzo dmitrylyzo marked this pull request as draft May 7, 2022 19:20
@dmitrylyzo dmitrylyzo marked this pull request as ready for review May 7, 2022 19:30
@dmitrylyzo dmitrylyzo added the bug Something isn't working label May 8, 2022
Fixes the animation detection (and removal) initially introduced
in the misleadingly named "Less zealous logging" commit
jellyfin@6980deb

The old version was not detecting valid ASS tags like '\t  (\frz50'
and did some other checks not backed by ASS' behaviour. Rewrite it from
scratch to not miss any animated tags.
The previous version made an attempt to also detect and not report noop
animation tags like '\move(1,1,1,1)'; the new version does not as
correctly parsing the tag and its values is simply more effort than its
worth and likely to result in false negatives. Noop animation tags
should be sufficiently rare anyway.
@dmitrylyzo dmitrylyzo force-pushed the fix-animated-events branch from b619f0b to 2da990e Compare May 9, 2022 14:22
@dmitrylyzo dmitrylyzo merged commit 9892dbe into jellyfin:new-master May 13, 2022
@dmitrylyzo dmitrylyzo deleted the fix-animated-events branch May 13, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants