shaka-project / shaka-player

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

Bad subtitle rendering #2524

Closed avelad closed 4 years ago

avelad commented 4 years ago

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

What version of Shaka Player are you using? 2.5.10

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 81 macOS 10.15.4

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

What are the manifest and license server URIs?

I can send a link if it's necessary, I attached the subtitle segment

What did you do?

Load the video, and select a subtitle

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

What actually happened?

The subtitle is displayed misplaced and offset of the video player.

Screenshot from DASH.js Captura de pantalla 2020-04-23 a las 10 42 34

Screenshot from ShakaPlayer Captura de pantalla 2020-04-23 a las 10 42 51

Screenshot from ShakaPlayer but I commented the problematic css property (top) Captura de pantalla 2020-04-23 a las 10 42 58

WVTT segment SUB5_00000117.m4s.zip

TheModMaker commented 4 years ago

Could you please send the manifest URL? It will be difficult to test with just the text segment since it's likely a problem with several components working together.

avelad commented 4 years ago

The content is geoblocked, and is not available in the USA. So I have shared the segment. :(

joeyparrish commented 4 years ago

We should be able to add the VTT text segment to some other content with player.addTextTrack().

theodab commented 4 years ago

Reproduced. I'll look into it.

theodab commented 4 years ago

This is a result of how we place our controls, I believe. The DASH.js reference player's controls are beneath the video element; meanwhile, our controls are on top of the video element. We position the text box such that it stops above the controls bar, which takes up the bottom 15% of the video element. Your captions there are configured so that the top of the text box is 85% down the video element. That means that, as configured, it wants to overlap with the controls bar. With the two constraints at once, top:85% and bottom:15%, the result is that the text element has a height of 0 pixels.

I'm not sure what the ideal way to solve this would be. I can see a few possibilities, but none of them feel like great options to me.

  1. Automatically change the position of cues that would overlap with the controls, to shift them up a bit. This behavior might look inconsistent, though, I'm not sure.

  2. Get rid of the bottom-restraints on cues when the controls are invisible (but keep the cues hidden when the controls are visible). This could work, but also I am pretty sure it would be an accessibility issue.

  3. Make all percent-values scale based on the usable part of the video element, not the total video element. This would change almost everyone's cue positioning, though, even if they don't have cues that overlap with the controls.

@michellezhuogg, the UI text displayer was your project. What's your opinion on this?

joeyparrish commented 4 years ago

I'm not certain, but I think Chrome's native controls and native text display might adjust position automatically. So it might be good to follow suit. It could probably be done just with CSS and an attribute.

theodab commented 4 years ago

Hm. I think you're right. Looking at this sample with our UI disabled, it definitely looks like the box is higher than it is in DASH.js. Interestingly, it still looks like that even if you turn off the video element's controls. So I think they might actually be using what I listed as option 3. I'll give that a try, see how it looks.

avelad commented 4 years ago

This change produces a side effect, now all the subtitles occupy the entire width of the video instead of just the width they need.

image

Url to test: https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd

joeyparrish commented 4 years ago

You're right! We'll take another look. Thanks!

theodab commented 4 years ago

Is this actually a bug? The captions file for that asset does not specify a size, and according to the webVTT spec for the size attribute:

By default, the WebVTT cue size is set to 100%.

So it seems like this is actually the expected behavior, according to the spec at least. I experimentally tried feeding in cues with a size of something besides 100, and the text displayer honors that as expected.

avelad commented 4 years ago

Stream: https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd

https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd;panel=HOME;build=uncompiled image

https://v2-5-10-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd;panel=HOME;build=uncompiled image

Stream: https://storage.googleapis.com/shaka-demo-assets/sintel/dash.mpd

https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/sintel/dash.mpd;panel=ALL_CONTENT;build=uncompiled image

https://v2-5-10-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/sintel/dash.mpd;panel=ALL%20CONTENT;build=uncompiled image

Stream: https://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd

https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd;panel=HOME;build=uncompiled image

https://v2-5-10-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd;panel=ALL%20CONTENT;build=uncompiled image

Note: Safari native HLS vs Chrome https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/angel-one-hls/hls.m3u8;panel=ALL_CONTENT;build=uncompiled

Safari:

Captura de pantalla 2020-05-08 a las 8 42 21

Chrome: image

The behavior is not the same as version 2.5.10.

joeyparrish commented 4 years ago

The point @theodab was making was that this change seems to be in line with the VTT spec, which states that the default width for a cue is 100%.

That said, we can see your point, @avelad, that this is unexpected and different from previous releases. We can make the default width effectively "auto" in CSS terms, to fit the size of the text.

avelad commented 4 years ago

In the case of https://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd

There is another additional bug due to the fact that text overlaps when in the previous version did not occur.

@joeyparrish , I understand that the spec says that it must be 100%, but Safari does not apply it like this and also this supposes a groundbreaking change with respect to what was there before, can I suggest making this change configurable and setting the old value in 2.5.x by default and the spec in 2.6, but allowing to choose behavior?

joeyparrish commented 4 years ago

I think we can revert to the old behavior without undoing the placement fixes made by @theodab.

joeyparrish commented 4 years ago

If a cue specifies 100%, the background will extend from left edge to right edge. If there is no specific width, the background will fit to the text as it did before. This would apply to v2.5 and v2.6, and would first appear in v2.5.12. Sorry we didn't notice the difference before v2.5.11 came out. Since the "Angel One" clip starts with a mostly black frame, it was an easy mistake to overlook on that content.

avelad commented 4 years ago

@joeyparrish I think that there are another error... (sorry about that) I'm pretty sure the regions in TTML have been broken too.

https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-419;textlang=es-419;uilang=es-419;asset=https://livesim.dashif.org/dash/vod/testpic_2s/multi_subs.mpd;panel=ALL_CONTENT;panelData=SUBTITLES;build=uncompiled

error
<tt xmlns:ttp="http://www.w3.org/ns/ttml#parameter" xmlns="http://www.w3.org/ns/ttml"
    xmlns:tts="http://www.w3.org/ns/ttml#styling" xmlns:ttm="http://www.w3.org/ns/ttml#metadata"
    xmlns:ebuttm="urn:ebu:metadata" xmlns:ebutts="urn:ebu:style"
    xml:lang="eng" xml:space="default"
    ttp:timeBase="media"
    ttp:cellResolution="32 15">
  <head>
    <metadata>
      <ttm:title>DASH-IF Live Simulator</ttm:title>
      <ebuttm:documentMetadata>
        <ebuttm:conformsToStandard>urn:ebu:distribution:2014-01</ebuttm:conformsToStandard>
        <ebuttm:authoredFrameRate>30</ebuttm:authoredFrameRate>
      </ebuttm:documentMetadata>
    </metadata>
    <styling>
      <style xml:id="s0" tts:fontStyle="normal" tts:fontFamily="sansSerif" tts:fontSize="100%" tts:lineHeight="normal"
      tts:color="#FFFFFF" tts:wrapOption="noWrap" tts:textAlign="center"/>
      <style xml:id="s1" tts:color="#00FF00" tts:backgroundColor="#000000" ebutts:linePadding="0.5c"/>
      <style xml:id="s2" tts:color="#ff0000" tts:backgroundColor="#000000" ebutts:linePadding="0.5c"/>
    </styling>
    <layout>
      <region xml:id="r0" tts:origin="15% 80%" tts:extent="70% 20%" tts:overflow="visible" tts:displayAlign="before"/>
      <region xml:id="r1" tts:origin="15% 20%" tts:extent="70% 20%" tts:overflow="visible" tts:displayAlign="before"/>
    </layout>
  </head>
  <body style="s0">
    <div region="r0">

      <p xml:id="sub2768000" begin="00:46:08.000" end="00:46:09.000" >
        <span style="s1">eng [caption]: 00:46:08.000</span>
      </p>

      <p xml:id="sub2769000" begin="00:46:09.000" end="00:46:10.000" >
        <span style="s1">eng [caption]: 00:46:09.000</span>
      </p>

    </div>
  </body>
</tt>
avelad commented 4 years ago

Arg ... There is also a subtitle overlay problem.

error2
joeyparrish commented 4 years ago

@avelad, the nightly build has not been updated since the fix. It normally updates around 3 am US/Pacific time. I'll update it now.

joeyparrish commented 4 years ago

The nightly build is now up-to-date with today's changes. Here is what I see:

nightly-ttml-subs

joeyparrish commented 4 years ago

@avelad, please try again and let us know if there are any further issues.

avelad commented 4 years ago

@joeyparrish , The position now it's correct, but the subtitle overlap persist

error3

Note: I shared the stream privately

joeyparrish commented 4 years ago

We haven't received the manifest URL yet, but I'll reopen the issue now. I tried various clips in our demo content with subtitles, but I didn't see this effect with any of them. @theodab, when we get a manifest URL, can you please take a look at the overlapping subtitles in @avelad's latest report?

joeyparrish commented 4 years ago

We have fixed the overlap, and I'm updating the nightly build now to reflect this change.

joeyparrish commented 4 years ago

The nightly has now been updated.

avelad commented 4 years ago

Now it works as expected. Thanks!

avelad commented 4 years ago

:(

@joeyparrish , I found another regression with the last change..

https://nightly-dot-shaka-player-demo.appspot.com/demo/#audiolang=es-ES;textlang=es-ES;uilang=es-ES;asset=https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd;panel=ALL_CONTENT;build=uncompiled

image

The subtitle appears on the middle of the video

joeyparrish commented 4 years ago

I think I see an easy fix, but we've broken this too many times for me to keep making one-off fixes without test automation. We need tests that can check the layout of many various types of subtitles in the UI and compare them to known-good screenshots.

joeyparrish commented 4 years ago

This has been fixed and the updates have been pushed to the nightly build. Furthermore, I will have screenshot-based layout tests in internal code review soon to help prevent mistakes of this kind in the future.

avelad commented 4 years ago

@joeyparrish Is it possible to migrate the last change ( 754f16ab80494457e55c718e6734d7a8752f20d0 )to version 2.5?

joeyparrish commented 4 years ago

Good catch! I thought I had cherry-picked it already. I'll double-check and make sure that all the text fixes are in v2.5.12.

joeyparrish commented 4 years ago

We have just landed layout tests to help avoid repeating these mistakes. See b839340220152c4ab7d9e72439af04dc7f26ef25 for details.

avelad commented 4 years ago

@joeyparrish do you have any ETA for 2.5.12?

joeyparrish commented 4 years ago

I need to do another round of cherry-picks and check on the status of the newly-filed #2594, but otherwise v2.5.12 is ready to go. I had thought to release it earlier this week, but fixing #2593 (just landed today) took longer than expected.