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

feat: add event stream support #1382

Merged
merged 18 commits into from
Apr 3, 2023
Merged

feat: add event stream support #1382

merged 18 commits into from
Apr 3, 2023

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented Mar 20, 2023

Description

Note: This is marked as a draft until the mpd-parser changes are integrated.
See EventStream PR to mpd-parser: videojs/mpd-parser#170

This pull request will take the parsed EventStream data and convert it into a id3-like object which can be added to the metadata text track for use as timed metadata.

Specific Changes proposed

Taking the EventStream data from the parsed manifest produced in the handleMain function in the dash-playlist-loader then adding it to the timed metadata text-track via the addMetadata function.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@adrums86 adrums86 self-assigned this Mar 20, 2023
@adrums86 adrums86 marked this pull request as ready for review March 20, 2023 22:45
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #1382 (421adde) into main (6bd98d0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
+ Coverage   85.37%   85.40%   +0.03%     
==========================================
  Files          40       40              
  Lines        9954     9963       +9     
  Branches     2308     2308              
==========================================
+ Hits         8498     8509      +11     
+ Misses       1456     1454       -2     
Impacted Files Coverage Δ
src/dash-playlist-loader.js 90.51% <100.00%> (+0.18%) ⬆️
src/playlist-controller.js 95.16% <100.00%> (+0.03%) ⬆️
src/segment-loader.js 96.46% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

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

Looks good to me once my comment is addressed! I am not super familiar with what this metadata looks like, but informational nonetheless.

src/dash-playlist-loader.js Outdated Show resolved Hide resolved
Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

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

+1, Good catch on the spec.

@adrums86
Copy link
Contributor Author

@dzianis-dashkevich and @wseymour15 I pushed the refactor, I would appreciate another look at the PR when you have time. Thanks!

@wseymour15
Copy link
Contributor

wseymour15 commented Mar 31, 2023

@dzianis-dashkevich and @wseymour15 I pushed the refactor, I would appreciate another look at the PR when you have time. Thanks!

Looks good to me!

@adrums86 adrums86 merged commit f6b9498 into main Apr 3, 2023
@adrums86 adrums86 deleted the event-stream branch April 3, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants