shaka-project / shaka-player

JavaScript player library / DASH & HLS client / MSE-EME player
Apache License 2.0
7.19k stars 1.34k forks source link

TTML displayAlign broken in 3.1 #3379

Closed avelad closed 3 years ago

avelad commented 3 years ago

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 commented 3 years ago

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

avelad commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

Bug introduced with https://github.com/google/shaka-player/commit/9c2315e69db3f855349db0e0e5e32a1aa5b9647c

avelad commented 3 years ago

@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 https://github.com/google/shaka-player/commit/9c2315e69db3f855349db0e0e5e32a1aa5b9647c.

joeyparrish commented 3 years ago

I'll work on it now.

joeyparrish commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

joeyparrish commented 3 years ago

@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 commented 3 years ago

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

joeyparrish commented 3 years ago

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