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 1 commit into
base: main
Choose a base branch
from
Open

feat: hls-parser from service locator #50

wants to merge 1 commit 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

All modified and coverable lines are covered by tests ✅

Project coverage is 64.30%. Comparing base (756c570) to head (3522edb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   63.95%   64.30%   +0.35%     
==========================================
  Files         114      114              
  Lines        4830     4836       +6     
  Branches      633      636       +3     
==========================================
+ Hits         3089     3110      +21     
+ Misses       1733     1719      -14     
+ Partials        8        7       -1     
Flag Coverage Δ
dash-parser 64.30% <100.00%> (+0.35%) ⬆️
env-capabilities 64.30% <100.00%> (+0.35%) ⬆️
hls-parser 64.30% <100.00%> (+0.35%) ⬆️
playback 64.30% <100.00%> (+0.35%) ⬆️

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 3522edb
🔍 Latest deploy log https://app.netlify.com/sites/videojsdev/deploys/67ac287c2b7d360008ef6aaf
😎 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%) 63 ms (+171.09% 🔺) 135 ms
dash-parser/dist/es/index.min.js 2.67 KB (0%) 54 ms (0%) 56 ms (+878.7% 🔺) 109 ms
dash-parser/dist/iife/index.min.js 3.65 KB (0%) 74 ms (0%) 33 ms (+27.96% 🔺) 106 ms
hls-parser/dist/cjs/index.min.js 5.77 KB (0%) 116 ms (0%) 87 ms (+101.61% 🔺) 202 ms
hls-parser/dist/es/index.min.js 3.37 KB (0%) 68 ms (0%) 24 ms (+116.62% 🔺) 91 ms
hls-parser/dist/iife/index.min.js 5.78 KB (0%) 116 ms (0%) 34 ms (-28.76% 🔽) 150 ms
playback/dist/player/core/cjs/index.min.js 6.29 KB (+6.13% 🔺) 126 ms (+6.13% 🔺) 59 ms (+480.76% 🔺) 185 ms
playback/dist/player/core/es/index.min.js 3.85 KB (+12.69% 🔺) 78 ms (+12.69% 🔺) 10 ms (-78.79% 🔽) 87 ms
playback/dist/player/core/iife/index.min.js 6.3 KB (+6.23% 🔺) 127 ms (+6.23% 🔺) 50 ms (+474.96% 🔺) 176 ms
playback/dist/player-with-worker/core/cjs/index.min.js 8.55 KB (+4.27% 🔺) 172 ms (+4.27% 🔺) 58 ms (-15.07% 🔽) 230 ms
playback/dist/player-with-worker/core/es/index.min.js 6.6 KB (+5.8% 🔺) 132 ms (+5.8% 🔺) 72 ms (+8.39% 🔺) 204 ms
playback/dist/player-with-worker/core/iife/index.min.js 8.55 KB (+4.22% 🔺) 172 ms (+4.22% 🔺) 121 ms (+68.07% 🔺) 292 ms
env-capabilities/dist/cjs/index.min.js 1.61 KB (0%) 33 ms (0%) 7 ms (-1.29% 🔽) 39 ms
env-capabilities/dist/es/index.min.js 0 B (+100% 🔺) 0 ms (+100% 🔺) 1 ms (-41.78% 🔽) 1 ms
env-capabilities/dist/iife/index.min.js 1.62 KB (0%) 33 ms (0%) 4 ms (-62.14% 🔽) 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so, we should have a static members to register hls parser factory (not instance) in the hls-pipeline-loader (not pipeline itself)

For clarification, do we want to expose the instance of the parser (this.parser_) from the hls-pipeline-loader via the service-locator or do we want to simply expose the factory? I refactored to expose registered factory, per the feedback above, however the rfc example indicates we would want to expose the current instance of the parser instead:

const hlsParser = serviceLocator.getHlsParser();

hlsParser.parse(playlist);

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