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

shaka.text.Cue.textAlign is getting mangled #987

Closed
chrisfillmore opened this issue Aug 24, 2017 · 8 comments
Closed

shaka.text.Cue.textAlign is getting mangled #987

chrisfillmore opened this issue Aug 24, 2017 · 8 comments
Assignees
Labels
flag: Why didn't we catch this sooner This issue is embarassing; we may still need an automated test that could have prevented this issue status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@chrisfillmore
Copy link
Contributor

chrisfillmore commented Aug 24, 2017

Have you read the FAQ and checked for duplicate issues:
Yes

What version of Shaka Player are you using:
2.2.0

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

Can you reproduce the issue with the latest code from master:
Unknown

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

If custom app, can you reproduce the issue using our demo app:
No. Is there demo content that includes external vtt files (not embedded)? I'm not sure if this makes a difference. Yes

What browser and OS are you using:
MacOS 10.12.6
Chrome 60.0.3112.101

What are the manifest and license server URIs:
I can send a demo application to reproduce, if necessary. Sent manifest and license url to shaka-player-issues

What did you do?
Watched a stream with external vtt tracks (edit: left-aligned)

What did you expect to happen?
Stream to play normally and subtitles to appear left-aligned ('left' is the alignment value provided by the vtt tracks).

What actually happened?
Chrome produces warnings saying The provided value 'undefined' is not a valid enum value of type AlignSetting. The subtitles themselves also appear to be center-aligned (although they're oddly cropped... but this may be an issue in our demo application). I took some screenshots of the subtitles but I think they're a bit besides the point. I think this is the problem:

The warning comes from shaka.text.SimpleTextDisplayer.prototype.convertToTextTrackCue_, here:

vttCue.align = shakaCue.textAlign;

The value of shakaCue.textAlign is undefined.

This looks like a mangling / minification bug, because I don't get it when I run Shaka uncompiled.

When I set a breakpoint and inspect in compiled mode, I see that shaka.text.Cue.textAlign looks like:
screen shot 2017-08-24 at 11 14 23 am

So, when textAlign gets set on the cue point in shaka.text.VttTextParser.setTextAlign_, here:

cue.textAlign = Cue.textAlign[align.toUpperCase()];

... the value is undefined. I don't see this problem in Shaka v2.1.7.

@joeyparrish joeyparrish added this to the v2.3.0 milestone Aug 24, 2017
@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Aug 24, 2017
@chrisfillmore chrisfillmore changed the title Chrome warning: The provided value 'undefined' is not a valid enum value of type AlignSetting. shaka.text.Cue.textAlign is getting minified Aug 24, 2017
@chrisfillmore chrisfillmore changed the title shaka.text.Cue.textAlign is getting minified shaka.text.Cue.textAlign is getting mangled Aug 24, 2017
@chrisfillmore
Copy link
Contributor Author

Would you like me to send a demo app to reproduce?

@joeyparrish
Copy link
Member

I haven't tried yet, but it sounds easy enough to reproduce.

It shouldn't be undefined, because the regex looks like this:

  if ((results = /^align:(start|middle|center|end|left|right)$/.exec(word))) {
    VttTextParser.setTextAlign_(cue, results[1]);

So results[1] should only be one of those strings in the regex alternation. Later, when we do this:

    cue.textAlign = Cue.textAlign[align.toUpperCase()];

The value of align should definitely be one of the enumerated values.

We'll dig into this. Thanks for the report!

@joeyparrish joeyparrish added needs triage and removed type: bug Something isn't working correctly labels Aug 24, 2017
@joeyparrish
Copy link
Member

Okay, I thought it would be easy to repro, but I can't so far. I have tried both "Angel One" (WebVTT subs) and "Sintel" (w/ MP4-embedded VTT text). Can you please send a demo asset or app that reproduces the problem?

@chrisfillmore
Copy link
Contributor Author

chrisfillmore commented Aug 24, 2017

It's not the value of results[1] that is problematic. The value of shaka.text.Cue.textAlign in compiled Shaka is:

{
  Hc: "end",
  Ic: "left",
  Mc: "right",
  Nc: "start",
  tb: "center"
}

So when we ask for shaka.text.Cue.textAlign['LEFT'], the value is undefined.

I can send a demo over in just a bit. [edit: demo app sent]

@chrisfillmore
Copy link
Contributor Author

I sent another email just now with repro steps for the Shaka demo app.

@ismena ismena self-assigned this Aug 25, 2017
@ismena
Copy link
Contributor

ismena commented Aug 25, 2017

Received, thanks!

@ismena ismena added type: bug Something isn't working correctly and removed needs triage labels Aug 25, 2017
@joeyparrish
Copy link
Member

It seems that we didn't catch this because we aren't testing with text streams that use these attributes.

@joeyparrish joeyparrish added the flag: Why didn't we catch this sooner This issue is embarassing; we may still need an automated test that could have prevented this issue label Aug 25, 2017
joeyparrish pushed a commit that referenced this issue Aug 31, 2017
Text parsers set several cue enum properties with
parsedValue.toUpper() rather than explicitely stating the enum
value. Renaming enum keys in compiled mode interferes with it.

Closes #987

Change-Id: Id3ff878eaa96bce28eaddd379a457f8743c97ea2
@joeyparrish
Copy link
Member

Cherry-picked to v2.2.1

@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@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
flag: Why didn't we catch this sooner This issue is embarassing; we may still need an automated test that could have prevented this issue 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

4 participants