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

addChaptersTrack() promise resolves before the chapters file has been downloaded and parsed by the browser #4186

Closed
hochhaus opened this issue May 3, 2022 · 4 comments · Fixed by #4228 or #4235
Assignees
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@hochhaus
Copy link
Contributor

hochhaus commented May 3, 2022

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

Yes

What version of Shaka Player are you using?

v3.3.4

Can you reproduce the issue with our latest release version?

Yes (tested against v4.0.0)

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

Yes (tested against c2f1fb5)

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?

The demo app does not use chapters to the best of my understanding so no I cannot reproduce using the demo app

What browser and OS are you using?

FF 99 on Debian Linux

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

N/A

What are the manifest and license server URIs?

Sent via email.

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

Send via email.

What did you do?

Call addChaptersTrack() and wait for the promise to resolve (successfully).

What did you expect to happen?

The browser to download and parse the chapters VTT file before resolving the promise (similar to the captions behavior).

What actually happened?

When shaka downloads a chapters VTT file it appears to do so by appending a <track> element to the DOM and then resolving the addChaptersTrack() promise. The problem is that the <text> element downloads and parses the VTT file asynchronously meaning that the calling code can't safely use the chapters VTT immediately after the promise resolves.

Note that when shaka downloads a captions VTT file it (correctly) appears to do so from a JS fetch().

As a work around we are using the following code to parse our own chapters VTT files and avoid calling
addChaptersTrack() / getChapters()

  const resp = await fetch(url);
  // error handling for !resp.ok

  const data = new Uint8Array(await resp.arrayBuffer());
  const time = {periodStart: 0, segmentStart: 0, segmentEnd: 0};
  const result = new shaka.text.VttTextParser().parseMedia(data, time);

  const chapters = [];
  for (const item of result) {
    const chapter = {
      id: item.id,
      title: item.payload,
      startTime: item.startTime,
      endTime: item.endTime,
    };
    chapters.push(chapter);
  }

References #3895

@hochhaus hochhaus added the type: bug Something isn't working correctly label May 3, 2022
@github-actions github-actions bot added this to the v3.3 milestone May 3, 2022
@avelad avelad added the priority: P2 Smaller impact or easy workaround label May 3, 2022
@avelad avelad modified the milestones: v3.3, v4.1 May 4, 2022
@joeyparrish
Copy link
Member

Inside addChaptersTrack(), we await this.addSrcTrackElement_(), but that resolves as soon as the Track element is appended to the media element as a child. We could await some other event that would indicate the track has been parsed. Let me look into it.

@joeyparrish joeyparrish self-assigned this May 11, 2022
@joeyparrish
Copy link
Member

I never received an email with your content URLs, but I am going to experiment with some arbitrary chapter tracks and see if there is a useful event on the Track element that will resolve this.

@joeyparrish
Copy link
Member

With a little refactoring around the addition of a track element, it looks like the load event on the HTML element will do the trick.

joeyparrish added a commit to joeyparrish/shaka-player that referenced this issue May 16, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes shaka-project#4186
joeyparrish added a commit to joeyparrish/shaka-player that referenced this issue May 16, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes shaka-project#4186
@hochhaus
Copy link
Contributor Author

Thanks @joeyparrish

joeyparrish added a commit that referenced this issue May 16, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes #4186
joeyparrish added a commit that referenced this issue May 17, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes #4186
joeyparrish added a commit that referenced this issue May 17, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes #4186
joeyparrish added a commit that referenced this issue May 17, 2022
addChaptersTrack() was already an async method, but it did not wait
for the chapter data to be loaded by the browser.  The solution is to
wait for the `load` event on the `<track>` element we create.

To accomplish this, some cleanup and refactoring was done in how
tracks are managed.  Summary of changes:

 - The `addtrack` event, which triggered management of tracks in src=
   playback, is now used for all types of playback.  In src= mode, it
   manages all tracks, and in MSE mode, it only manages chapters
   tracks (which are added to the video element as in src= mode).
 - `processChaptersTrack_()` has been renamed to
   `activateChaptersTrack_()`, since its only job is to set the
   track's mode field to make the browser load it.
 - `activateChaptersTrack_()` is now only ever called via the
   `addtrack` event.
 - `activateChaptersTrack_()` no longer loops over all chapter tracks
   on a timer, and instead only touches the single track it was
   called for.
 - `addSrcTrackElement_()` now returns the HTML `<track>` element it
   creates.
 - `addChaptersTrack()` now awaits a `load` or `error` event to
   complete (or fail) the operation.
 - Existing tests for addChaptersTrack had long delays to work around
   this issue; these delays have simply been removed.

Fixes #4186
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
3 participants