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.SimpleTextDisplayer is not a constructor #1819

Closed
marcbaechinger opened this issue Feb 23, 2019 · 7 comments
Closed

shaka.text.SimpleTextDisplayer is not a constructor #1819

marcbaechinger opened this issue Feb 23, 2019 · 7 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@marcbaechinger
Copy link

marcbaechinger commented Feb 23, 2019

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

Yes. Searched issues, read release notes of 2.5.0-beta3 and plugins page/doc around text displayer.

What version of Shaka Player are you using?

2.5.0-beta3

Can you reproduce the issue with our latest release version?

With 2.5.0-beta3.

Can you reproduce the issue with the latest code from master?

With 2.5.0-beta3.

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

Custom app.

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

No.

What browser and OS are you using?

Chrome/OSX.

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

NA.

What are the manifest and license server URIs?

Happens when calling the constructor of shaka.Player

What did you do?

const shakaPlayer = new ShakaPlayer(/** @type {!HTMLMediaElement} */
    (videoElement));

What did you expect to happen?

No exception in console log.

What actually happened?

After upgrading from 2.5.0-beta2 to beta3 without any other changes, I see an exception prompted to the console. The playback works as expected though, when continuing and/or I found some workarounds but I wanted to report (not sure about subtitles yet).

It appears to me that the js compiler concatenates the javascript differently then with beta2. With beta3 the SimpleTextDisplayer is now appended at the very end of the concatenated javascript file. This seems to be outside the closure dependency system, so when I start our app and construct Shaka the SimpleTextDisplayer is not available in shaka.Player.prototype.defaultConfig_:L2417 which causes this stack trace:

app_desktop.js:46803 Uncaught (in promise) TypeError: shaka.text.SimpleTextDisplayer is not a constructor
    at new config.textDisplayFactory (app_desktop.js:46803)
    at shaka.Player.createMediaSourceEngine (app_desktop.js:45598)
    at shaka.Player.attach (app_desktop.js:44987)
    at new shaka.Player (app_desktop.js:44591)
    at app_desktop.js:50363
    at Object.goog.loadModule (app_desktop.js:1170)
    at app_desktop.js:50212

I can avoid this stack trace constructing Shaka in an async function like with setTimeout or onload=() => {}. This makes me constructing Shaka in the next looper event, when shaka.text.SimpleTextDisplayer will available because the js has been interpreted to the very end.

I can actually imagine this is WAI and I need to adjust my build somehow. I'd prefer though, having this working inside the closure dependency system instead of the asyn onload thingy.

So if WAI, what is the recommended tweak? Should I just require shaka.text.SimpleTextDisplayer somewhere in my code just to make the compiler concatenated it before I construct Shaka? Or can Shaka require it to use it safely in shaka.Player.prototype.defaultConfig_:L2417?

A temporary workaround which works is requiring the SimpleTextDisplayer:

const SimpleTextDisplayer = goog.require('shaka.text.SimpleTextDisplayer');
// ... code, code, code
new SimpleTextDisplayer(mediaElement); // avoid lint warnings
new ShakaPlayer(mediaElement);

This makes the compiler concatenated the SimpleTextDisplayer earlier.

@vaage vaage added type: bug Something isn't working correctly and removed needs triage labels Feb 24, 2019
@vaage vaage added this to the v2.5 milestone Feb 24, 2019
@vaage
Copy link
Contributor

vaage commented Feb 24, 2019

Thanks for filing the issue @marcbaechinger, thankfully we are a little ahead of you. This issues has been resolved as of commit df3926916e791a95863b86fb423de35724e439ae.

We will be converting all factories to not use new in our up-coming 2.6 release. After which the chance for issues like this should go away.

@vaage vaage closed this as completed Feb 24, 2019
@marcbaechinger
Copy link
Author

Thanks. When will this be available in the internal tip of tree?

@vaage
Copy link
Contributor

vaage commented Feb 25, 2019

@marcbaechinger You should already see it in master (Submitted Feb 20th). Whether there will be another 2.5 beta release, that is up to @joeyparrish but we are closing in on the src= work, so a full 2.5 release should be with-in sight soon.

@marcbaechinger
Copy link
Author

Thanks Aaron! I can't see it on master or google3. Not an big issue for me though. I have the workaround in place for now and am happy to see a full 2.5 release soon. Many thanks!

@joeyparrish
Copy link
Member

df39269 doesn't fix anything, it just adds a comment and reformats the code. I think two of us fixed the bug separately, and then the two fixes were merged. The commit that landed first and actually fixed the bug was d420b46.

However, that had to do with shaka.ui.TextDisplayer, and this issue is about shaka.text.SimpleTextDisplayer, so we should take the time to test without the UI library and see if we can reproduce. Our demo app uses the UI library, so it's possible that something slipped by unnoticed in the other build config.

@joeyparrish joeyparrish reopened this Feb 25, 2019
@joeyparrish joeyparrish self-assigned this Feb 25, 2019
@joeyparrish
Copy link
Member

Using the exact tag v2.5.0-beta3, I can't reproduce your results with the sample application in our "basic usage" tutorial. I see no exception, and I can display subtitles by running:

player.selectTextTrack(player.getTextTracks()[0]);
player.setTextTrackVisibility(true);

So the text displayer is working properly.

Two things worth noting, though:

  1. The sample app waits for document's DOMContentLoaded event before constructing a Player instance. In general, you should probably be waiting for some event to know that your JS is fully loaded and that the necessary DOM elements are available. If you aren't doing that, this could be the key difference between your app and mine.
  2. Although Player references SimpleTextDisplayer, it doesn't goog.require it. This would cause issues with the dependency ordering in the compiler, which would explain how beta2 and beta3 differ. This is a bug on us, but one you should be naturally working around by waiting for some appropriate event as described above.

The missing goog.require is a pretty trivial fix, so I'll get that done right now.

@marcbaechinger
Copy link
Author

Thanks all! I can confirm your findings. Both approaches work for me.

I prefer relying on the dependency ordering only, so I was able to workaround it by requiring SimpleTextDisplayer in my code and do nothing with it just to have the dependency ordering differently as you mention in 2. above.

Thanks for adding the goog.require, that's the best solution for me.

@shaka-project shaka-project locked and limited conversation to collaborators Apr 26, 2019
@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
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