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

CMCD: Implement nor and nrr CMCD keys #1518

Merged
merged 6 commits into from
Sep 10, 2024
Merged

CMCD: Implement nor and nrr CMCD keys #1518

merged 6 commits into from
Sep 10, 2024

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Aug 27, 2024

Based on #1467

We're planning to add CMCD (Common Media Client Data) v1 capabilities to our next release of the RxPlayer v4.

The CMCD feature, when explicitly enabled, adds multiple key-value pairs with most request (either from the query string, either from headers) to inform the server-side of the current playback conditions as seen from the player which made the request.

We were previously missing two keys used for a specific use-case: nor (Next Object Request) and nrr (Next Range Request). Those two allow the player to announce which segment it will probably load next so that the server could theoretically prefetch it in advance.

Implementing those two means that our CMCD logic needs to be aware of the next segment that will probably be requested after a current one. This is made much simpler by #1467, as now our fetcher logic is aware of the whole queue of segments for a given type (audio/video etc.) in most cases.

The main meat of this PR is just computing a relative URL, because that's what CMCD wants (in the name of DoS mitigation, I did not dive into why). I thought we already had that logic somewhere but if we have, I did not find it, so I re-coded and tested it.

@peaBerberian peaBerberian added the CMCD Concerns "Common Media Client Data" label Aug 27, 2024
@peaBerberian peaBerberian added this to the 4.2.0 milestone Aug 27, 2024
@peaBerberian peaBerberian force-pushed the misc/cmcd-nor-nrr branch 4 times, most recently from 9e5b22f to 2af4c00 Compare August 27, 2024 18:08
@peaBerberian peaBerberian added the Priority: 2 (Medium) This issue or PR has a medium priority. label Sep 3, 2024
@peaBerberian peaBerberian changed the base branch from misc/segment-queue-fetcher2 to dev September 3, 2024 17:28
Based on #1467

We're planning to add CMCD (Common Media Client Data) v1 capabilities to
our next release of the RxPlayer v4.

This features adds multiple key-value pairs with most request (either
from the query string, either from headers) to inform the server-side of
the current playback condition from the player which made the request.

We were previously missing two keys used for a specific use-case: `nor`
(Next Object Request) and `nrr` (Next Range Request). Those two allow
the player to anounce which segment it will probably load next so that
the server could theoretically prefetch it in advance.

Implementing those two means that our CMCD logic needs to be aware of
the next segment that will probably be requested after a current one.
This is made much simpler by #1467, as now our fetcher logic is aware of
the whole queue of segments for a given type (audio/video etc.) in most
cases.

The main meat of this PR is just computing a relative URL, because
that's what CMCD wants (in the name of DoS mitigation, I did not dive
into why). I thought we already had that logic somewhere but if we have,
I did not find it, so I re-coded and tested it.
@peaBerberian peaBerberian added Priority: 1 (High) This issue or PR has a high priority. and removed Priority: 2 (Medium) This issue or PR has a medium priority. labels Sep 6, 2024
Comment on lines 92 to 97
if (
(baseParts.path[0] === "/" || newParts.path[0] === "/") &&
baseParts.path[0] !== newParts.path[0]
) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

baseURL: http://github.com
newURL : http://github.com/rx-player

The expected relative url should be rx-player right ?
I think it's returning null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add tests to check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I just pushed seems to fix the issue, though it's hard to wrap my head against all those cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

It "fixes" this particular test case but I'm not sure it's the expected fix, the authority length is always >0 because its the part "github.com" here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  getRelativeUrl(
    "http://github.com/"
    "http://github.com/rx-player",
  )

this gives "../rx-player" it does not seems right

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the authority length is always >0 because its the part "github.com" here

Yes that was on purpose, because I wanted to return null if for example getRelativeUrl("a/b", "/a/c") is provided.

[...] this gives "../rx-player" it does not seems right

You're right, are you succeeding to see those cases just by reading the code here?
I'm unable to, if it's easier for you to understand the logic I could let you write it.

I added the breaking test

@peaBerberian peaBerberian merged commit 4d203f8 into dev Sep 10, 2024
8 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMCD Concerns "Common Media Client Data" Priority: 1 (High) This issue or PR has a high priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants