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

improve TTML rendering #1962

Merged
merged 2 commits into from Jul 12, 2019
Merged

improve TTML rendering #1962

merged 2 commits into from Jul 12, 2019

Conversation

@valotvince
Copy link
Contributor

@valotvince valotvince commented May 24, 2019

This PR adds:

  • a notion of nested cue, that renders inside the parent cue
  • a first support of spans inside paragraphs

How to test:

TODO

  • Unit tests: text_displayer(s), ttml_text_parser
  • Rebase branch on master
  • Follow the Contributing guidelines fully
@googlebot
Copy link

@googlebot googlebot commented May 24, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Loading

@valotvince
Copy link
Contributor Author

@valotvince valotvince commented May 24, 2019

I signed it!

Loading

@googlebot
Copy link

@googlebot googlebot commented May 24, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Loading

@valotvince valotvince force-pushed the enhance-ttml-display branch 2 times, most recently from 94f4512 to a0bcd11 May 24, 2019
@googlebot
Copy link

@googlebot googlebot commented May 24, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Loading

@ismena
Copy link
Member

@ismena ismena commented May 24, 2019

Hi @valotvince, thanks for the PR!
Before we get deep into code review, could you help me better understand what this change is doing?
Is it coloring some parts of the subtitles based on a TTML attribute?
Could you give me an example of a TTML that specifies this (doesn't have to be real, made up is fine) and what you expect to happen.

Loading

@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jun 6, 2019

Hi @ismena, sorry for the wait, I am on vacation :)

Lets suggest we have the following TTML extract:

<p region="..." begin="..." end="...">
  <span style="style01">A text</span>
  <br />
  <span>Another text</span>
</p>
<p region="..." begin="..." end="...">Text</p>

As of today, Shaka won't read styles in each span of paragraphs, but will merge the spans in one and only cue: A text\nAnother text (only printed with the paragraph color)

That PR is makes Shaka read each span within paragraphs. The spans are read as independent cues with their own region/begin/end attributes (based on the closest ancestor who possesses them), so two cues will be printed out (with their own style definitions based on the text displayer in use):
A text (maybe printed with a yellow color)
Another text (maybe printed with a red color)

The counter-part of doing so, is that the paragraph style section won't be used in the span styles... But as I've seen in issues on Github, the text displayers and TTML parser would need a little refactoring to be able to meet the TTML specs.

Hope I was clear enough :)

Best,

Loading

@ismena
Copy link
Member

@ismena ismena commented Jun 10, 2019

No worries, hope you had a great vacation :)

Ah, gotcha. Let's get to it! I left a few questions on the CL, here's another one: do you have content you're testing with? I'd love to cherry-pick your change and try it.
Feel free to send it privately to shaka-player-issues@ if you'd like.

Loading

@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jun 17, 2019

Hi @ismena,

I have some private streams that I can't share but I was also testing alongside the streams found in that article: https://subtitling.irt.de

Loading

@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jun 18, 2019

Hi @ismena,

I've committed to add some features for TTML rendering (I've followed some of the ideas of #1080).

My company's video team is working to give you a public manifest with some TTML features like positioning and colours, I'll update here as soon as I have it :)

Best,

Loading

@valotvince valotvince changed the title add basic support of ttml span in p improve html rendering Jun 18, 2019
@valotvince valotvince changed the title improve html rendering improve TTML rendering Jun 18, 2019
@valotvince valotvince force-pushed the enhance-ttml-display branch from 457bf65 to 684dc25 Jun 18, 2019
@googlebot
Copy link

@googlebot googlebot commented Jun 18, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

Loading

@valotvince valotvince changed the base branch from v2.5.x to master Jun 18, 2019
@googlebot
Copy link

@googlebot googlebot commented Jun 18, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Loading

@valotvince valotvince force-pushed the enhance-ttml-display branch from d717dd1 to 1e06dc4 Jun 19, 2019
demo/main.js Outdated Show resolved Hide resolved
Loading
demo/demo.less Outdated Show resolved Hide resolved
Loading
lib/text/simple_text_displayer.js Outdated Show resolved Hide resolved
Loading

/* Set the captions at the bottom by default. */
align-items: flex-end;
align-items: center;

Copy link
Member

@ismena ismena Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Loading

Copy link
Contributor Author

@valotvince valotvince Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the flex-direction for the cues to display on a column so I had to change the flex rules on the container.

That was mainly because I've tested prior to the support of span of p, I'll revert this to see if it still works ok !

Loading

ui/text_displayer.js Show resolved Hide resolved
Loading
externs/shaka/text.js Show resolved Hide resolved
Loading
@ismena
Copy link
Member

@ismena ismena commented Jun 21, 2019

Thanks for replying to my comments! I'll try your changes on the content you provided and will get back to you early next week.
Please ping me on this mid next week if I get swamped with other stuff and forget (I do that sometimes :( )

Loading

Copy link
Member

@ismena ismena left a comment

I tried the changes out - looks good! The (hopefully) last round of nitpicks, mostly style.

Loading

lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
lib/text/ttml_text_parser.js Outdated Show resolved Hide resolved
Loading
@valotvince valotvince marked this pull request as ready for review Jun 26, 2019
@valotvince valotvince force-pushed the enhance-ttml-display branch from 1e06dc4 to c353f87 Jun 26, 2019
@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jul 1, 2019

@ismena Hi :) Just a ping to let you know I've taken account of you review in the last commit :)

Loading

@ismena
Copy link
Member

@ismena ismena commented Jul 2, 2019

@valotvince Awesome and thank for pinging.
Please rebase the commit on top of the latest master and I will run the tests and approve the commit if the tests are ok.

Loading

@valotvince valotvince force-pushed the enhance-ttml-display branch from c353f87 to f979d10 Jul 3, 2019
@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jul 3, 2019

@ismena Done :)

Loading

@shaka-bot
Copy link
Collaborator

@shaka-bot shaka-bot commented Jul 3, 2019

Test Failure:

Generating Closure dependencies...
Linting JavaScript...

/var/lib/jenkins/workspace/Manual PR Test (local-tests)/test/text/ttml_text_parser_unit.js
853:1   error  Line 853 exceeds the maximum line length of 80       max-len
853:45  error  Expected parentheses around arrow function argument  arrow-parens

✖ 2 problems (2 errors, 0 warnings)
1 error and 0 warnings potentially fixable with the `--fix` option.

END-BUILD: FAILURE
Build step 'Execute shell' marked build as failure

Loading

@ismena
Copy link
Member

@ismena ismena commented Jul 3, 2019

Looks like there are some linter problems. Please run build/all.py locally to see what needs resolving.

Loading

@valotvince valotvince force-pushed the enhance-ttml-display branch from f979d10 to a8e8e2a Jul 5, 2019
@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jul 5, 2019

@ismena Sorry, forgot to run it after the rebase 💯 Should be okay now !

Loading

@valotvince valotvince force-pushed the enhance-ttml-display branch from a8e8e2a to 2d9161a Jul 12, 2019
@shaka-bot
Copy link
Collaborator

@shaka-bot shaka-bot commented Jul 12, 2019

All tests passed!

Loading

@ismena ismena merged commit c670b55 into google:master Jul 12, 2019
1 check passed
Loading
@ismena
Copy link
Member

@ismena ismena commented Jul 12, 2019

@valotvince Cool! Thanks for contributing :)

Loading

@avelad
Copy link
Contributor

@avelad avelad commented Jul 15, 2019

@ismena after this PR, CE608 and SMPTE-TT subtitles are broken. Please review it! (Asset: Live sim TTML Image Subtitles embedded (VoD) )

Loading

@avelad
Copy link
Contributor

@avelad avelad commented Jul 15, 2019

The issue is related with the next if: c670b55#diff-226b4f50d0392c977a71a42816ecdad1R246

Loading

@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Jul 15, 2019

Hello @avelad, could you provide an example source so I could work on a fix ? Got it ;)
Sorry for that !

Loading

@valotvince valotvince deleted the enhance-ttml-display branch Jul 15, 2019
@ismena
Copy link
Member

@ismena ismena commented Jul 15, 2019

Ah, damn it! We should look into adding unit tests for this so it doesn't happen again. Thanks for spotting, @avelad and @valotvince for the fix. I'll get right on to reviewing it.

Loading

AnteWall added a commit to AnteWall/shaka-player that referenced this issue Jul 17, 2019
@valotvince
Copy link
Contributor Author

@valotvince valotvince commented Aug 26, 2019

Hi there Any idea when that diff will be released ? Thanks !

Loading

@ismena
Copy link
Member

@ismena ismena commented Aug 26, 2019

We should've released this in 2.5.5 that just went out :( @joeyparrish @TheModMaker can we include this in the next minor release?

Loading

@joeyparrish
Copy link
Member

@joeyparrish joeyparrish commented Aug 26, 2019

I'll delegate this to @TheModMaker. If it makes sense to cherry-pick it, please cherry-pick to v2.5.x now, and it will show up in the next release, v2.5.6.

Loading

TheModMaker added a commit that referenced this issue Aug 27, 2019
Change-Id: Ifed3539e90649d259707a91fbd644fc58ba79b94
@TheModMaker
Copy link
Member

@TheModMaker TheModMaker commented Aug 27, 2019

I've cherry-picked these commits to v2.5.x, and they'll appear in the v2.5.6 release.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants