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

TTML displayAlign broken in 3.1 #3379

Closed
avelad opened this issue Apr 30, 2021 · 13 comments
Closed

TTML displayAlign broken in 3.1 #3379

avelad opened this issue Apr 30, 2021 · 13 comments
Assignees
Labels
component: captions/subtitles The issue involves captions or subtitles component: TTML The issue involves TTML subtitles specifically priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@avelad
Copy link
Collaborator

avelad commented Apr 30, 2021

Have you read the FAQ and checked for duplicate open issues? Yes

What version of Shaka Player are you using? 3.1.0

Can you reproduce the issue with our latest release version? Yes

Can you reproduce the issue with the latest code from master? Yes

Are you using the demo app or your own custom app? Both

If custom app, can you reproduce the issue using our demo app? Yes

What browser and OS are you using?
Chrome 90 macOS 10.15.7

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?

Subtitle example:
subtitle.ttml.zip

What configuration are you using? What is the output of player.getConfiguration()?

N/A

What did you do?

Load any stream with the previous TTML

What did you expect to happen?
The subtitle is rendered correctly

What actually happened?

tts:displayAlign="after" is ignored (verticalAlign style in UITextDisplayer doesn't have effect)

image

http://sandflow.com/imsc1_1/
image

@avelad
Copy link
Collaborator Author

avelad commented Apr 30, 2021

Note: it is a regression of 3.1 in 3.0.x it works fine.

@TheModMaker TheModMaker added type: bug Something isn't working correctly component: captions/subtitles The issue involves captions or subtitles priority: P1 Big impact or workaround impractical; resolve before feature release component: TTML The issue involves TTML subtitles specifically and removed needs triage labels Apr 30, 2021
@TheModMaker TheModMaker added this to the v3.2 milestone Apr 30, 2021
@avelad
Copy link
Collaborator Author

avelad commented May 5, 2021

@ismena , Since issues are now labeled with priorities, is there an estimated time for issues with different priorities to be resolved?

After analyzing the release 3.1 this is the only problem to be able to migrate to it :(

@joeyparrish
Copy link
Member

P0 (top priority) will generally mean that someone needs to drop everything else until it's done. These are bugs that break something major for everyone.
P1 should be resolved before the next feature release. It impacts lots of users, and workarounds are generally not practical.
P2 should be resolved soon. It impacts some users, or there is an easy workaround.
P3 is useful but not urgent.
P4 is nice to have, or maybe just wishful thinking.

@avelad
Copy link
Collaborator Author

avelad commented Jun 24, 2021

Bug introduced with 9c2315e

@avelad
Copy link
Collaborator Author

avelad commented Jun 24, 2021

@joeyparrish is there any way to prioritize this? For me this is blocking the integration of release 3.1. I've tried to fix the problem and do a PR, but I don't see any way to fix it without using FLEX, which is just what you removed in 9c2315e.

@joeyparrish
Copy link
Member

I'll work on it now.

@joeyparrish joeyparrish self-assigned this Jun 24, 2021
@joeyparrish
Copy link
Member

Repro:

  1. Download TTML subtitles from OP into 3379.ttml, in the shaka source root
  2. Modify the timestamps, reducing by 1:21:56 so that the subs show quickly.
  3. Open BBB in local Shaka Demo
  4. Run in the debugger:
video.muted = true;
player = video.ui.getControls().getLocalPlayer();
await player.addTextTrackAsync('../3379.ttml', 'es', 'subtitle', 'application/ttml+xml');
player.selectTextLanguage('es');
player.setTextTrackVisibility(true);

The div is placed correctly and has the correct size, but vertical-align: bottom; has no effect on the placement of the span within it.

@joeyparrish
Copy link
Member

You're right, it's easily-solved with flexbox layout. I haven't come up with an alternative yet. I will see what breaks in our layout tests with flexbox and work backward. Perhaps I can make the existing tests pass and avoid recreating bug #3013.

@joeyparrish
Copy link
Member

joeyparrish commented Jun 24, 2021

Issues discovered while playing with display: flex on the div:

  1. <br> elements directly inside the div take up as much vertical space as any other element. Should be 0. Workaround: wrap all children in another span, with background set to none.
  2. Multiple span elements inside the div are layed out vertically instead of horizontally. The same workaround (extra span wrapper) seems to fix this.
  3. Spaces between sibling spans in TTML seem to get lost. May be a TTML parser issue and not layout related.

Side-note: the line-height style on the spans appears to be too large. Setting to 1.2 brings the Chrome HTML rendering in line with the IMSC1 renderer.

@avelad
Copy link
Collaborator Author

avelad commented Jun 28, 2021

@joeyparrish , have you found any solutions for the problems you have encountered?

@joeyparrish
Copy link
Member

@joeyparrish , have you found any solutions for the problems you have encountered?

Potentially. See my earlier workaround comment, which I'm trying to turn into a concrete solution:

Workaround: wrap all children in another span, with background set to none.

@joeyparrish joeyparrish modified the milestones: v3.2, v3.3 Jul 7, 2021
@joeyparrish
Copy link
Member

I'm resuming work on this today. Sorry for the delays.

@joeyparrish
Copy link
Member

I have it working now, plus regression tests, and the change is in review. Thanks for your patience!

joeyparrish added a commit that referenced this issue Jul 13, 2021
This uses flexbox once again to get proper positioning of cues.  To
compensate for the issues that originally made us remove flexbox, this
adds a wrapper span inside the flexbox element.

Summary of screenshot changes:
 - slight change to background sizing
   - ui-basic-cue
   - ui-cue-with-controls
   - ui-duplicate-cues
   - ui-end-time-edge-case
   - ui-flat-cue-bg
   - ui-two-basic-cues
 - background fills block with literal newline in text
   - ui-cue-with-newline
 - region anchored without padding
   - ui-region-position
 - new screenshots
   - *-nested-cues-with-linebreak
   - *-region-with-display-alignment  (regression test for this issue)

Closes #3379

Change-Id: I8c678721d96662e0f8940cda12df4f5b5e5baf1e
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Sep 7, 2021
@shaka-project shaka-project locked and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: captions/subtitles The issue involves captions or subtitles component: TTML The issue involves TTML subtitles specifically priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

4 participants