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: hls-parser from service locator #50

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: hls-parser from service locator #50

wants to merge 3 commits into from

Conversation

adrums86
Copy link
Collaborator

@adrums86 adrums86 commented Feb 1, 2025

Description

  • add getHlsParser functionality to the service locator.
  • add basic service locator tests

Specific Changes proposed

expose the full HLS parser through the a getHlsParser function and add tests.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Change has been verified in an actual browser (Chrome, Firefox, Safari, Edge) (if applicable)
  • Unit Tests updated or fixed (if applicable)
  • Docs/guides updated (if applicable)

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 53.57143% with 26 lines in your changes missing coverage. Please review.

Project coverage is 63.90%. Comparing base (cef7ecb) to head (2877469).

Files with missing lines Patch % Lines
...ges/playback/src/lib/pipelines/mse/mse-pipeline.ts 38.09% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   63.89%   63.90%   +0.01%     
==========================================
  Files         114      115       +1     
  Lines        4833     4887      +54     
  Branches      633      635       +2     
==========================================
+ Hits         3088     3123      +35     
- Misses       1737     1757      +20     
+ Partials        8        7       -1     
Flag Coverage Δ
dash-parser 63.90% <53.57%> (+0.01%) ⬆️
env-capabilities 63.90% <53.57%> (+0.01%) ⬆️
hls-parser 63.90% <53.57%> (+0.01%) ⬆️
playback 63.90% <53.57%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

netlify bot commented Feb 1, 2025

Deploy Preview for videojsdev ready!

Name Link
🔨 Latest commit 2877469
🔍 Latest deploy log https://app.netlify.com/sites/videojsdev/deploys/67a2bfb0829618000897145c
😎 Deploy Preview https://deploy-preview-50--videojsdev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Feb 1, 2025

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dash-parser/dist/cjs/index.min.js 3.65 KB (0%) 73 ms (0%) 47 ms (+2.57% 🔺) 120 ms
dash-parser/dist/es/index.min.js 2.67 KB (0%) 54 ms (0%) 36 ms (+43.33% 🔺) 89 ms
dash-parser/dist/iife/index.min.js 3.65 KB (0%) 74 ms (0%) 25 ms (-54.11% 🔽) 98 ms
hls-parser/dist/cjs/index.min.js 5.81 KB (0%) 117 ms (0%) 67 ms (+18.09% 🔺) 183 ms
hls-parser/dist/es/index.min.js 3.37 KB (0%) 68 ms (0%) 43 ms (+54.99% 🔺) 111 ms
hls-parser/dist/iife/index.min.js 5.79 KB (0%) 116 ms (0%) 67 ms (-18.12% 🔽) 183 ms
playback/dist/player/core/cjs/index.min.js 11.46 KB (+92.7% 🔺) 230 ms (+92.7% 🔺) 124 ms (+207.91% 🔺) 354 ms
playback/dist/player/core/es/index.min.js 6.63 KB (+94% 🔺) 133 ms (+94% 🔺) 107 ms (+596.52% 🔺) 240 ms
playback/dist/player/core/iife/index.min.js 11.47 KB (+92.63% 🔺) 230 ms (+92.63% 🔺) 147 ms (+444.87% 🔺) 377 ms
playback/dist/player-with-worker/core/cjs/index.min.js 13.72 KB (+66.76% 🔺) 275 ms (+66.76% 🔺) 135 ms (+120.24% 🔺) 409 ms
playback/dist/player-with-worker/core/es/index.min.js 9.44 KB (+50.56% 🔺) 189 ms (+50.56% 🔺) 157 ms (+125.96% 🔺) 345 ms
playback/dist/player-with-worker/core/iife/index.min.js 13.72 KB (+66.83% 🔺) 275 ms (+66.83% 🔺) 183 ms (+28.85% 🔺) 457 ms
env-capabilities/dist/cjs/index.min.js 1.61 KB (0%) 33 ms (0%) 8 ms (+92.8% 🔺) 40 ms
env-capabilities/dist/es/index.min.js 0 B (+100% 🔺) 0 ms (+100% 🔺) 2 ms (-77.36% 🔽) 2 ms
env-capabilities/dist/iife/index.min.js 1.62 KB (0%) 33 ms (0%) 4 ms (-12.81% 🔽) 36 ms

@adrums86 adrums86 marked this pull request as ready for review February 4, 2025 00:33
});
return new HlsPipeline(dependencies);
}
}
Copy link
Collaborator

@dzianis-dashkevich dzianis-dashkevich Feb 5, 2025

Choose a reason for hiding this comment

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

So, the flow is following:
user registers pipeline-loaders via pipelineLoaderFactory storage, or via separate api for worker-based player (so it is opt-in operation)
user calls load with some payload includeing mimeType
player checks and finds match for loaded mimeType and pipeline-loader-factory
player creates pipeline-loader instace via found factory and injects core deps
player passes loadPlayload to the created loader instance
loader creates parser via registered factory and injects core deps
loader loads manifest/playlist
loader parses received data
loader creates pipeline via registered factories and passes core deps and parser instance and returns this instance back to the player.
so as you can see there is a layer of dependency injections
so, we should have a static members to register hls parser factory (not instance) in the hls-pipeline-loader (not pipeline itself)
anticipated follow-up question:
why do we need pipeline-loader? can’t we just create pipeline directly in the player?
because we want separate classes for live and vod pipelines and we don’t know ahead of time which one to create. We can understand it only once we load and parse playlist/manifest.

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