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: support multiple bitrate values in stream proxy filter #67

Conversation

edvinhed
Copy link

@edvinhed edvinhed commented Jul 2, 2024

Updated the stream proxy filter to parse both individual bitrate values and arrays of bitrate values for filtering.

  • Modified the input type br in the filter function to accept:
    • number: Individual bitrate values.
    • number[]: Arrays of bitrate values.
    • "*": A wildcard to match any bitrate.

This enhancement allows more flexible filtering based on bitrate specifications.

This is done by instead of checking it against a direct number value we check if the provided bitrates (whether they are in an array or as a singular number) includes the bitrate in the segmentBitrate and checks true or false if, or if not, that is the case.

Let me know if anything has to be fixed, added examples of a test situation in the README as well as restructuring the type there (even if I have not changed the actual type, since that did not need to be done to be able to input several bitrates).

Closes #59

Updated the stream proxy filter to parse both individual bitrate values and arrays of bitrate values for filtering.

- Modified the input type `br` in the filter function to accept:
  - `number`: Individual bitrate values.
  - `number[]`: Arrays of bitrate values.
  - `"*"`: A wildcard to match any bitrate.

This enhancement allows more flexible filtering based on bitrate specifications.
@edvinhed edvinhed marked this pull request as ready for review July 2, 2024 13:04
!config?.br || config?.br === '*' || config?.br === segmentBitrate
!config?.br ||
config?.br === '*' ||
config?.br.toString().includes(segmentBitrate.toString()) //Checks if .br contains bitrates that need to be filtered
Copy link
Member

@friday friday Jul 2, 2024

Choose a reason for hiding this comment

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

This part doesn't look safe to me. At this point params is JSON parsed, so it should be either a number, the string "*" or an array of numbers. We already handled if it's "*", so what we need to handle is if it's a number or array of numbers.

I would do something like this: Array.isArray(config?.br) ? config.br : [config.br]. Then you know it's an array and can check .includes consistently. (Array.from(something) does this too, but it's not safe to use in this specific case because it works differently with numbers).

I would also break up the filter function a bit more. Since it grew in complexity I think it's better / more clear to add a scope ({}) and explicit returns now.

The way you're doing it now with string.includes will detect false positives, for example if you specify 500000 it will also match with 1500000 or 5000000

Instead of converting it, we now just check if it's an array and then uses .includes()
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