shaka-project / shaka-player

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

WebVTT position setting not working #4486

Closed erankor closed 1 month ago

erankor commented 1 year ago

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

What version of Shaka Player are you using? v4.2.1 (from demo page)

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

Can you reproduce the issue with the latest code from main? Didn't test

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

If custom app, can you reproduce the issue using our demo app? N/A

What browser and OS are you using? Chrome 105.0.5195.102 on Windows 10

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

What are the manifest and license server URIs?

https://www.kaltura.com/p/0/playManifest/entryId/0_z79l0t8f/format/mpegdash/protocol/https/x.mpd (Choose the "Spanish" subtitles)

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

Using the demo page default

What did you do?

Play the stream & choose the "Spanish" subtitles

What did you expect to happen? The cue "Left alignment" is expected to appear on the left, it has these WebVTT settings:

00:00:25.000 --> 00:00:30.000 position:10%,line-left align:left size:35%
Left alignment

Same for "Right alignment":

00:00:30.000 --> 00:00:35.000 position:90% align:right size:35%
Right alignment

What actually happened?

All cues seem to be centered horizontally.

I think that we're missing + '%' here - https://github.com/shaka-project/shaka-player/blob/main/lib/text/ui_text_displayer.js#L620

Since position is only the number - https://github.com/shaka-project/shaka-player/blob/main/lib/text/vtt_text_parser.js#L666

However, even after manually adding the % in Chrome dev tools, the line did not show in the correct position.

Thank you!

Eran

Robloche commented 1 year ago

I encountered a related positioning bug with my subtitles. Adding % where you suggest is only part of the solution since the specs are incorrectly implemented.

For instance, if you read the specs here, you'll see that if no alignment is given for position (i.e. same as auto), it should follow the text alignment.

So if you have 00:04:07.360 --> 00:04:10.080 align:end position:90%, it means that:

But have a look at the following code:

  static setPositionAlign_(cue, align) {
    const Cue = shaka.text.Cue;
    if (align == 'line-left' || align == 'start') {
      cue.positionAlign = Cue.positionAlign.LEFT;
    } else if (align == 'line-right' || align == 'end') {
      cue.positionAlign = Cue.positionAlign.RIGHT;
    } else {
      cue.positionAlign = Cue.positionAlign.CENTER;
    }
  }

It says that if no position alignment is given, center is used.

I'll try to fix that and make a PR.

Robloche commented 1 year ago

Well... After reading the specs for the 1000th time, some things are not as clear as I thought they were. For instance, it says here:

the default value of the WebVTT cue position alignment is center

But it says here:

By default, the position alignment is set to auto.

I started a PR but I'm gonna investigate a little bit more, and commit fixes if needed.

Robloche commented 1 year ago

I commited changes to my PR and I think it's ready.

erankor commented 1 year ago

Awesome, thanks @Robloche!

Robloche commented 1 year ago

I hope it will solve your issue (and that I didn't cause any regression).