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.

Sign up for GitHub

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

Jump to bottom

Captions regression in 3.3.0 #3850

Closed
FireMasterK opened this issue Jan 12, 2022 · 7 comments · Fixed by #4009
Closed

Captions regression in 3.3.0 #3850

FireMasterK opened this issue Jan 12, 2022 · 7 comments · Fixed by #4009
Assignees
@theodab
Labels
component: captions/subtitles The issue involves captions or subtitles 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
v4.0

Comments

@FireMasterK
Copy link

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

What version of Shaka Player are you using?
3.3.0

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

Can you reproduce the issue with the latest code from master?
I can on the nightly demo

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

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

What browser and OS are you using?
Brave, Linux

What are the manifest and license server URIs?
https://bin.snopyta.org/?b692293141083c43#7jpXJvfKgK6JMxQ8JZFpHqkmTbtUmm6gV2QkqcZRtxCU

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

What did you do?

const player = document.querySelector("video").ui.getControls().getPlayer();
player.addTextTrackAsync("https://...", "EN", "SUBTITLE", "application/ttml+xml", null, "ENGLISH");

What did you expect to happen?
When the captions have 2 lines, they shouldn't be overlapped.

What actually happened?
The captions appear overlapped.
image
@FireMasterK FireMasterK added the type: bug Something isn't working correctly label Jan 12, 2022
@shaka-bot shaka-bot added this to the v3.3 milestone Jan 12, 2022
@theodab theodab added component: captions/subtitles The issue involves captions or subtitles priority: P1 Big impact or workaround impractical; resolve before feature release labels Jan 12, 2022
@theodab theodab self-assigned this Jan 12, 2022
@theodab
Copy link
Contributor

theodab commented Jan 12, 2022

It looks like this is a result of cdeffbb

@avelad
Copy link
Member

avelad commented Jan 12, 2022

Maybe this related to #3741?

@theodab
Copy link
Contributor

theodab commented Jan 12, 2022

That might be the case, yes. Reverting the line of code that seems to be causing this also looks to fix that issue.

@avelad
Copy link
Member

avelad commented Jan 14, 2022

@theodab Did you find a solution for this?

@theodab
Copy link
Contributor

theodab commented Jan 14, 2022

Yeah, I believe so.
I think the problem is that our TTML text parser, if you assign a region to an element, also assigns that region to all of that element's descendants.
cdeffbb, on further inspection, wasn't really the source of the problem; it just exposed the problem, by removing the logic in the displayer that said to ignore regions if there weren't on root elements.
So the fix I am working on right now is to just only assign a region if it's set directly in the element, and ignore inherited regions for the purposes of positioning.

I just want to take some time first to hopefully figure out whether or not this is going to break some other use case.

@avelad
Copy link
Member

avelad commented Jan 17, 2022

Another example with the same error:

<?xml version="1.0" encoding="utf-8"?>
<tt xml:lang="" xmlns="http://www.w3.org/ns/ttml" xmlns:tts="http://www.w3.org/ns/ttml#styling">
	<head>
		<styling>
			<style tts:backgroundColor="transparent" tts:color="white" tts:displayAlign="after" tts:fontFamily="proportionalSansSerif" tts:fontSize="0.68c" tts:textOutline="black 2px" xml:id="speakerStyle"/>
		</styling>
		<layout>
			<region style="speakerStyle" tts:extent="78% 16%" tts:origin="9% 80%" tts:textAlign="center" tts:zIndex="1" xml:id="speaker_1"/>
			<region style="speakerStyle" tts:extent="80% 35%" tts:origin="10% 60%" tts:textAlign="center" tts:zIndex="1" xml:id="speaker_2"/>
		</layout>
	</head>
	<body>
		<div>
			<p begin="61765:29:27.141" end="61765:29:29.061" region="speaker_1">
				<span tts:color="white" tts:fontStyle="normal" tts:fontWeight="normal" tts:textDecoration="none">El testigo</span>
				<br/>
				<span tts:color="white" tts:fontStyle="normal" tts:fontWeight="normal" tts:textDecoration="none">ha desaparecido misteriosamente.</span>
			</p>
			<p begin="61765:29:29.061" end="61765:29:29.141" region="speaker_2"/>
		</div>
	</body>
</tt>

@avelad
Copy link
Member

avelad commented Jan 18, 2022

I have seen that this issue is also related to #3762

joeyparrish pushed a commit that referenced this issue Jan 28, 2022
@theodab @joeyparrish
fix(text): Fix caption overlap.
d6884bd 
This changes the TTML parser to not allow cue regions to be inherited
to the children of the element the region was originally assigned on,
except for the purposes of styles (colors, etc).
To allow regions on elements "above" the cues in TTML, such as the
<body> or <div> elements, this also changes the TTML parser to render
the full structure of the TTML file as a tree of cues. The end result
will be a single cue representing the <body>, with children
representing the <div> elements inside it, and those <divs> will have
children that represent the actual cues. Now that our text displayer
can intelligently update child cues as they enter or leave the display
window, this approach should be possible.

Closes #3850
Closes #3741

Backported to v3.1.x

Change-Id: Ia8d750daa06920610c04e9b26e29d2d304eaf8a9
joeyparrish pushed a commit that referenced this issue Jan 28, 2022
@theodab @joeyparrish
fix(text): Fix caption overlap.
615e300 
This changes the TTML parser to not allow cue regions to be inherited
to the children of the element the region was originally assigned on,
except for the purposes of styles (colors, etc).
To allow regions on elements "above" the cues in TTML, such as the
<body> or <div> elements, this also changes the TTML parser to render
the full structure of the TTML file as a tree of cues. The end result
will be a single cue representing the <body>, with children
representing the <div> elements inside it, and those <divs> will have
children that represent the actual cues. Now that our text displayer
can intelligently update child cues as they enter or leave the display
window, this approach should be possible.

Closes #3850
Closes #3741

Backported to v3.2.x

Change-Id: Ia8d750daa06920610c04e9b26e29d2d304eaf8a9
joeyparrish pushed a commit that referenced this issue Jan 28, 2022
@theodab @joeyparrish
fix(text): Fix caption overlap.
4980195 
This changes the TTML parser to not allow cue regions to be inherited
to the children of the element the region was originally assigned on,
except for the purposes of styles (colors, etc).
To allow regions on elements "above" the cues in TTML, such as the
<body> or <div> elements, this also changes the TTML parser to render
the full structure of the TTML file as a tree of cues. The end result
will be a single cue representing the <body>, with children
representing the <div> elements inside it, and those <divs> will have
children that represent the actual cues. Now that our text displayer
can intelligently update child cues as they enter or leave the display
window, this approach should be possible.

Closes #3850
Closes #3741

Change-Id: Ia8d750daa06920610c04e9b26e29d2d304eaf8a9
@github-actions github-actions bot mentioned this issue Feb 12, 2022
Closed
@github-actions github-actions bot mentioned this issue Mar 3, 2022
Merged
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Mar 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2022
@avelad avelad modified the milestones: v3.3, v4.0 May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@theodab theodab

Labels
component: captions/subtitles The issue involves captions or subtitles 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
Milestone
v4.0
4 participants
@avelad @shaka-bot @FireMasterK @theodab