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(dash): Enable Stateful Mode and rsq Corruptions for DASH #66

Merged
merged 10 commits into from
Jul 2, 2024

Conversation

Kajlid
Copy link
Contributor

@Kajlid Kajlid commented Jun 26, 2024

  • Imported STATEFUL and newState utilities for DASH (segment.ts, dashManifestUtils.ts and dashManifestUtils.test.ts) to support stateful mode.
  • Ensured rsq corruptions are possible for DASH when the proxy is running in stateful mode, similar to how rsq corruptions are enabled for HLS.

This should solve the issue #63: Support for relative corruptions (rsq) for DASH live streams.

- Imported STATEFUL and newState utilities for DASH (segment.ts, dashManifestUtils.ts and dashManifestUtils.test.ts) to support stateful mode.
- Ensured rsq corruptions are possible for DASH when the proxy is running in stateful mode, similar to how rsq corruptions are enabled for HLS.

This update allows for the use of relative sequence numbers in DASH manifests, aligning the functionality with HLS. The DASH proxy now properly handles stateful operations and rsq parameters, enabling more flexible stream corruptions.
@Kajlid Kajlid added enhancement New feature or request In sprint labels Jun 26, 2024
@Kajlid Kajlid self-assigned this Jun 26, 2024
@Kajlid Kajlid linked an issue Jun 26, 2024 that may be closed by this pull request
Copy link
Contributor

@Nfrederiksen Nfrederiksen left a comment

Choose a reason for hiding this comment

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

It looks good. The feature is indeed implemented and behaves as expected 👍
Only a few thing I'd need before approving.

  1. .history can be included in .gitignore as well. No need to include it in the main branch.
  2. Now sure in what update it got missing but add back import { URLSearchParams } from 'url'; in utils.ts,
    just to be consistent in the project as it is now currently using the global URLSearchParams in that script when without the import.
  3. I like the handling of the case when a relative sequence corruption is attempted but the proxy is not in a stateful mode in config.ts.
    And although it is originally outside of the scope of the original issue Support for relative corruptions (rsq) for DASH live streams #63 , it would be much appreciated if
    a unit test could be added for that case in config.test.ts method getAllManifestConfigs.
    It needs to test if isDash AND !STATEFUL, and also case if !isDash AND !STATEFUL. This would be super nice to have in place :)
  4. Lastly, after all other points are completed. One final task is to update the READme file with the new feature and an example url. Just follow other examples. :)

@Kajlid
Copy link
Contributor Author

Kajlid commented Jun 27, 2024

Hi Nicholas!
Thank you for your answer and for giving feedback in such a structured and understandable way :). I have updated the code with the changes you suggested, including the new unit test and updated README.

I did not quite understand the first sentence of point 3, since the code I added in config.ts was inspired from the similar if statement above, giving an error message if !STATEFUL and !isDash, so I added a similar error message for !STATEFUL and isDash. This is also what the new unit test in config.test.ts is checking for, those error messages. In other words: I added code in config.test.ts now, but did not understand if I also need to make some change in config.ts.

Thanks again for the feedback and please get back if I need to make additional changes.

@Kajlid Kajlid requested a review from Nfrederiksen July 1, 2024 07:34
Copy link
Contributor

@Nfrederiksen Nfrederiksen left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@Kajlid Kajlid removed the request for review from friday July 1, 2024 13:26
@Nfrederiksen Nfrederiksen merged commit 1a1fea7 into main Jul 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request In sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for relative corruptions (rsq) for DASH live streams
2 participants