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: update m3u8 parser version and restore dateTimeObject and dateTimeString usage #1412

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

harisha-swaminathan
Copy link
Contributor

@harisha-swaminathan harisha-swaminathan commented Aug 4, 2023

This PR shouldn't be merged before

  • the corresponding m3u8-parser PR is merged and released
  • m3u8-parser is updated to the latest version containing the above PR
  • tests pass

Description

Restores dateTimeObject and dateTimeString segment properties that were previously removed and refactored in these two PRs

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

LGTM. Obviously lets get the parser update in here then +1 from me.

@@ -421,7 +421,7 @@ export default class PlaylistLoader extends EventTarget {
this.dateRangesStorage_.setPendingDateRanges(mediaPlaylist.dateRanges);
const availableDateRanges = this.dateRangesStorage_.getDateRangesToProcess();

if (!availableDateRanges.length) {
if (!availableDateRanges.length || !this.addDateRangesToTextTrack_) {
Copy link
Contributor Author

@harisha-swaminathan harisha-swaminathan Aug 4, 2023

Choose a reason for hiding this comment

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

Noticed that there were several !this.addDateRangesToTextTrack_ is not a function errors when I tested dateRanges with a specific stream. On logging !!his.addDateRangesToTextTrack_, noticed that the function was undefined on every alternate loadedplaylist event (line 410). Not sure why this is happening, so added a quick fix here before debugging in detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be because the test stream has subtitles, which in this case we create a PlaylistLoader for, this does not have the addDateRangesToTextTrack_ function defined, as it is only needed for the mainPlaylistLoader for HLS.

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1412 (ea0bb04) into main (4153b8a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1412      +/-   ##
==========================================
- Coverage   85.56%   85.55%   -0.01%     
==========================================
  Files          41       41              
  Lines       10147    10145       -2     
  Branches     2353     2351       -2     
==========================================
- Hits         8682     8680       -2     
  Misses       1465     1465              
Files Changed Coverage Δ
src/segment-loader.js 96.47% <ø> (-0.01%) ⬇️
src/playlist-loader.js 95.19% <100.00%> (ø)
src/sync-controller.js 97.44% <100.00%> (ø)
src/util/time.js 92.68% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

:shipit:

@harisha-swaminathan harisha-swaminathan merged commit 5e425c0 into main Aug 7, 2023
@harisha-swaminathan harisha-swaminathan deleted the fix/dateTimeObject-dateTimeString branch August 7, 2023 19:20
@harisha-swaminathan harisha-swaminathan changed the title fix: restore dateTimeObject and dateTimeString usage fix: update m3u8 parser version and restore dateTimeObject and dateTimeString usage Aug 7, 2023
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.

2 participants