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

Bad subtitle rendering #2524

Closed
avelad opened this issue Apr 23, 2020 · 33 comments
Closed

Bad subtitle rendering #2524

avelad opened this issue Apr 23, 2020 · 33 comments
Assignees
Labels
component: captions/subtitles The issue involves captions or subtitles component: TTML The issue involves TTML subtitles specifically component: UI The issue involves the Shaka Player UI status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@avelad
Copy link
Member

avelad commented Apr 23, 2020

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
Copy link
Contributor

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
Copy link
Member Author

avelad commented Apr 23, 2020

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

@joeyparrish
Copy link
Member

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

@theodab theodab self-assigned this Apr 23, 2020
@theodab theodab added type: bug Something isn't working correctly component: UI The issue involves the Shaka Player UI and removed needs triage labels Apr 23, 2020
@theodab
Copy link
Contributor

theodab commented Apr 23, 2020

Reproduced. I'll look into it.

@shaka-bot shaka-bot added this to the v2.6 milestone Apr 23, 2020
@theodab
Copy link
Contributor

theodab commented Apr 24, 2020

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
Copy link
Member

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
Copy link
Contributor

theodab commented Apr 25, 2020

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.

joeyparrish pushed a commit that referenced this issue May 1, 2020
Previously, we were positioning subtitles relative to the video,
but this lead to problems with subtitles that were manually positioned
(via the line attribute) to be low (85% or lower); they would overlap
with the controls bar, and thus be hidden.

This changes cues to be positioned relative to the text-usable area of
the screen (e.g. the space above the controls bar). In addition, the
less values have been changed to not trim cues that extend out of the
container, so that if they do overlap with the controls, they aren't
clipped in an ugly way.
Examining the Chrome native controls, this appears to be closer to how
they render captions.

Fixes #2524

Backported to v2.5.x

Change-Id: I4500b8f637f3f43f48d019d14ef527e75d960fec
@avelad
Copy link
Member Author

avelad commented May 6, 2020

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
Copy link
Member

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

@joeyparrish joeyparrish reopened this May 7, 2020
@theodab
Copy link
Contributor

theodab commented May 7, 2020

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
Copy link
Member Author

avelad commented May 8, 2020

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 joeyparrish self-assigned this May 11, 2020
@joeyparrish
Copy link
Member

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
Copy link
Member Author

avelad commented May 11, 2020

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
Copy link
Member

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

@joeyparrish
Copy link
Member

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
Copy link
Member Author

avelad commented May 12, 2020

@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
Copy link
Member Author

avelad commented May 12, 2020

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

error2

@joeyparrish
Copy link
Member

@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
Copy link
Member

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

nightly-ttml-subs

@joeyparrish
Copy link
Member

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

@avelad
Copy link
Member Author

avelad commented May 12, 2020

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

error3

Note: I shared the stream privately

@joeyparrish joeyparrish added the component: TTML The issue involves TTML subtitles specifically label May 12, 2020
@joeyparrish
Copy link
Member

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 joeyparrish reopened this May 12, 2020
@joeyparrish joeyparrish changed the title Bad subtitle rendering with wvtt format Bad subtitle rendering May 12, 2020
joeyparrish added a commit that referenced this issue May 13, 2020
When no specific size has been set on a Cue object, the default will
now be 0, which means "auto" size to the text displayer.  If a cue has
no specific width, the background will fit to the text.  If a cue has
an explicit width of 100%, the background will fill the entire
horizontal space of the text container.

This fixes the 100%-width background introduced in Change-Id
I4500b8f637f3f43f48d019d14ef527e75d960fec in #2524.

Fixes #2524

Backported to v2.5.x

Change-Id: I98caeb89d3da640a8175560810c8ccf2de27a2bb
@joeyparrish
Copy link
Member

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

@joeyparrish
Copy link
Member

The nightly has now been updated.

@avelad
Copy link
Member Author

avelad commented May 14, 2020

Now it works as expected. Thanks!

@avelad
Copy link
Member Author

avelad commented May 15, 2020

@joeyparrish joeyparrish reopened this May 15, 2020
@joeyparrish
Copy link
Member

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 joeyparrish added the component: captions/subtitles The issue involves captions or subtitles label May 19, 2020
@joeyparrish
Copy link
Member

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
Copy link
Member Author

avelad commented May 22, 2020

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

@joeyparrish
Copy link
Member

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 added a commit that referenced this issue May 23, 2020
The position: 'absolute' style should only be set when other
positioning attributes (left, right, top, bottom) are being set.
Otherwise, the natural layout flow is disturbed, without anything to
replace it.

Closes #2524

Backported to v2.5.x

Change-Id: I4062c68253218a0f4ace6560a8847381d80c48d5
joeyparrish added a commit that referenced this issue May 23, 2020
The margin: auto style centers the element both horizontally and
vertically.  We don't want vertical centering, and we already get
horizontal centering when appropriate through other styles.  So
removing margin: auto is the best fix.

Closes #2524

Change-Id: Icff74e49b3917b50141d1205232ebb0856fbcf16
@joeyparrish
Copy link
Member

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

@avelad
Copy link
Member Author

avelad commented May 28, 2020

@joeyparrish do you have any ETA for 2.5.12?

@joeyparrish
Copy link
Member

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.

@shaka-project shaka-project locked and limited conversation to collaborators Jul 20, 2020
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 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 component: UI The issue involves the Shaka Player UI 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

5 participants