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

fix(cxl-ui): cxl-jw-player handle tracks which are improperly formatted #435

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented Oct 7, 2024

@anoblet anoblet requested review from lkraav and pawelkmpt October 7, 2024 15:15
Copy link

github-actions bot commented Oct 7, 2024

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 44.75 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 15.04 KB (+0.45% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 29.23 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 157.96 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-institute.js, packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 291.42 KB (+0.03% 🔺)

@@ -84,7 +84,13 @@ export function TranscriptMixin(BaseClass) {
});
}

return tracks;
const filteredTracks = tracks.map((track) => {
track.data.text = track.data.text.replace("<v ->", "").replace("</v>", "");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk this looks weird, like it's unclear what the real root problem is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML seems to be coming directly from the response.

image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<v> seems to be a WebVTT Voice tag (TIL!) https://developer.mozilla.org/en-US/docs/Web/API/WebVTT_API/Web_Video_Text_Tracks_Format#voice_tag_

https://developer.mozilla.org/en-US/docs/Web/API/WebVTT_API

So it seems like legit output, although

  • unclear why it's sent, especially with - value.. does it mean voice owner unknown, or what's generating this?
  • I think our implementation needs to be able to render WebVTT tags, not try to remove them?

Copy link
Collaborator Author

@anoblet anoblet Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear why it's sent, especially with - value.. does it mean voice owner unknown, or what's generating this?

This is coming directly from JW Player, so I believe whichever software we used to generate these captions is responsible.

I think our implementation needs to be able to render WebVTT tags, not try to remove them?

We aren't using the WebVTT API. We use this library to parse the VTT files, so that we can render them outside of the video: https://github.com/gsantiago/subtitle.js

The WebVTT API, and the tags associated with it seem to only be useful when adding captions to the actual video itself. Since we are rendering the captions outside of the video, I'm not sure how helpful they are.

Edit:

This issue might be related to why our chapters sometimes fail: gsantiago/subtitle.js#90

Copy link

@pawelkmpt pawelkmpt Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anoblet what did we do differently in the old code? I recall we had some JS plugins for JWP. Please check what's the difference between parsing done by the old code and Subtitle lib

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our implementation needs to be able to render WebVTT tags, not try to remove them?

I think it's unnecessary. VTT tags are useful for captions but not important for transcript. I'd even think we should remove all of them, but at this point didn't see the presence of other tags so no need to spend time unless @anoblet knows some JS function which strips all tags. But not importing another library to do so.

@pawelkmpt pawelkmpt merged commit a33420a into master Oct 9, 2024
5 checks passed
@pawelkmpt pawelkmpt deleted the anoblet/fix/cxl-jw-player branch October 9, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants