Integrate CMCD data into outgoing requests. #4346

littlespex · 2021-10-06T00:16:02Z

This PR will...

Implement Client Media Common Data (CMCD) in outgoing media requests.

Why is this Pull Request needed?

Provide client data to deliver services and CDNs.

Are there any points in the code the reviewer needs to double check?

This changes effects the playlist and fragment loaders, so double check that the general architectural choices match the rest of the project.

Resolves issues:

Resolves #4324

Checklist

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

src/controller/cmcd-controller.ts

dsparacio

@mangui Thanks for reviewing this! Great Talk.

- Add support for av object type

api-extractor/report/hls.js.api.md

…d video

feature/cmcd-loaders

…teractive/hls.js into feature/cmcd-config-object * 'feature/cmcd-config-object' of https://github.com/cbsinteractive/hls.js: no message

mangui

lgtm ! maybe @tjenkinson or others have other comments ?
really nice work 👏 , moreover you were really responsive to address comments/remarks 🙇

tjenkinson

Agree this looks great

I've had a proper read of the spec now and have a couple more questions though

I'm assuming a server is supposed to first split on , but ignoring any ,'s inside "'s, to get each key/value pair, and then split each pair on the first =? If this is the case, then any custom value that is a token and has a value containing , will break the server? Are you sure custom values can be 'tokens'? If so we might need to be a bit stricter on the characters that are allowed in a token (like rejecting any containing a ,)? This spec is linked but I haven't read that in a lot of detail. We might also want to validate the key is also valid. I.e. if it contained an = that would also break things.
We don't support custom fields yet do we? I'm not sure using the js symbol to signal what is a token and what is a string is the best approach because I'm not sure if all browsers that could support hls.js also support symbol(?), and the purpose of symbol's is not specifically to represent tokens. I'd rather we made the api for custom fields still accept strings but support signalling if the string should be treated as a token or string.

If we don't support custom fields yet, could remove symbol handling for now and figure that out later? That also means we don't need to worry about my first point yet, because we're in control of all the keys and values.

src/controller/cmcd-controller.ts

tjenkinson · 2021-11-08T21:17:03Z

src/controller/cmcd-controller.ts

+      if (key === 'ot' || key === 'sf' || key === 'st') {
+        result = `${key}=${value}`;
+      } else if (type === 'string') {
+        result = `${key}=${JSON.stringify(value)}`;


Can we just make this the default else? Seems like the safest thing

tjenkinson · 2021-11-08T21:18:00Z

src/controller/cmcd-controller.ts

+      } else if (type === 'symbol') {
+        result = `${key}=${value.description}`;
+      } else {


If we don't support custom values yet, I think we should remove this for now, as I'm not sure we should use symbol to represent tokens and nothing should take this code path atm. See my main comment

feature/cmcd-config-object

- Make string case the fallback in serialization routine

littlespex · 2021-11-08T22:34:10Z

We don't support custom fields yet do we? I'm not sure using the js symbol to signal what is a token and what is a string is the best approach because I'm not sure if all browsers that could support hls.js also support symbol(?), and the purpose of symbol's is not specifically to represent tokens. I'd rather we made the api for custom fields still accept strings but support signalling if the string should be treated as a token or string.

The idea was pulled from an existing Structured Field library, https://www.npmjs.com/package/structured-field-values. It's an easy way to signal that a value is a token, as all other value types have one-to-one relationships with JS primitives. Otherwise we would need to write a full featured encoder, where custom fields can be registered with explicit data types (and header mappings). I'm not opposed to the idea, but I'd rather do that work in cmcd.js, and when it comes time to add custom fields to hls.js, use that library instead of the inline implementation.

I'm assuming a server is supposed to first split on , but ignoring any ,'s inside "'s, to get each key/value pair, and then split each pair on the first =? If this is the case, then any custom value that is a token and has a value containing , will break the server? Are you sure custom values can be 'tokens'? If so we might need to be a bit stricter on the characters that are allowed in a token (like rejecting any containing a ,)? This spec is linked but I haven't read that in a lot of detail.

We might also want to validate the key is also valid. I.e. if it contained an = that would also break things.

@wilaw would be a better person to answer some of these. Perhaps all string values should be url encoded? Full validation of key names and values could be rolled into the aforementioned encoder. For now, there is no way to pass in faulty data as custom fields have not been implemented.

If we don't support custom fields yet, could remove symbol handling for now and figure that out later? That also means we don't need to worry about my first point yet, because we're in control of all the keys and values.

Agreed. The symbol code has been removed, and the string case is the catch-all.

tjenkinson · 2021-11-09T09:09:42Z

The symbol code has been removed, and the string case is the catch-all.

I think you forgot to push it?

The idea was pulled from an existing Structured Field library, https://www.npmjs.com/package/structured-field-values. It's an easy way to signal that a value is a token, as all other value types have one-to-one relationships with JS primitives. Otherwise we would need to write a full featured encoder, where custom fields can be registered with explicit data types (and header mappings). I'm not opposed to the idea, but I'd rather do that work in cmcd.js, and when it comes time to add custom fields to hls.js, use that library instead of the inline implementation.

Cool. I can see how it's nice that there is a 1-1 mapping between js types. I think my main concern with using symbol is whether it's supported everywhere, although I do also think it's used just because we needed another type to switch on and that existed.

- Remove tokenized custom field code

littlespex · 2021-11-09T16:10:30Z

Sorry, pushed it to the wrong branch. All good now.

tjenkinson

LGTM!

I assume this is a minor release not major? Anything specific we should call out in the release notes?

littlespex · 2021-11-09T19:41:43Z

@tjenkinson Is that question for me? If so, I would consider this a minor release. No breaking changes, no new public API methods or properties, just a new config property.

tjenkinson · 2021-11-09T19:55:29Z

Great. @mangui look good to you? If no one has any objections I’m happy to merge and release it

mangui · 2021-11-09T21:51:24Z

sorry @tjenkinson I messed up and cancelled your approval
yes LGTM !

tjenkinson · 2021-11-10T13:05:35Z

It's in v1.1.0 now :)

littlespex added 2 commits October 5, 2021 15:45

Integrate CMCD data into outgoing requests.

66e72bf

Fix sanity test errors

c0199a0

littlespex marked this pull request as draft October 6, 2021 00:16

littlespex added 2 commits October 5, 2021 17:17

no message

e7db2b2

no message

57b5b62

littlespex commented Oct 6, 2021

View reviewed changes

src/controller/cmcd-controller.ts Outdated Show resolved Hide resolved

mangui reviewed Oct 6, 2021

View reviewed changes

src/controller/cmcd-controller.ts Outdated Show resolved Hide resolved

Improve buffer length calculation

8abb3eb

littlespex commented Oct 6, 2021

View reviewed changes

src/controller/cmcd-controller.ts Outdated Show resolved Hide resolved

littlespex commented Oct 6, 2021

View reviewed changes

src/controller/cmcd-controller.ts Outdated Show resolved Hide resolved

littlespex mentioned this pull request Oct 6, 2021

Additional Common Media Client Data (CMCD) support. #4324

Closed

littlespex marked this pull request as ready for review October 6, 2021 03:26

littlespex marked this pull request as draft October 6, 2021 03:28

dsparacio previously approved these changes Oct 6, 2021

View reviewed changes

- Fix for init object type bug

46cc8e7

- Add support for av object type

littlespex dismissed dsparacio’s stale review via 46cc8e7 October 8, 2021 21:31

littlespex marked this pull request as ready for review October 8, 2021 21:31

Merge branch 'master' into feature/cmcd

4710b36

littlespex requested review from dsparacio and mangui October 20, 2021 18:16

dsparacio previously approved these changes Oct 25, 2021

View reviewed changes

mangui reviewed Nov 3, 2021

View reviewed changes

api-extractor/report/hls.js.api.md Outdated Show resolved Hide resolved

Add support for separate buffer and top bandwidth values for audio an…

4d49ecd

…d video

littlespex dismissed dsparacio’s stale review via 4d49ecd November 3, 2021 18:50

littlespex added 6 commits November 3, 2021 18:18

Merge branch 'master' into feature/cmcd

7645f9f

Use loader composition instead of context transformation

1b68962

no message

6366aad

no message

43cb813

Only override loaders and add listereners if cmcd is enabled

629011d

Merge pull request #2 from cbsinteractive/feature/cmcd-loaders

09dad6a

feature/cmcd-loaders

littlespex added 6 commits November 8, 2021 10:22

Optimize multiple references to this

87df218

no message

475211f

Remove unused argument in apply function

640fe43

no message

6cc8799

Merge branch 'feature/cmcd-config-object' of https://github.com/cbsin…

54581b1

…teractive/hls.js into feature/cmcd-config-object * 'feature/cmcd-config-object' of https://github.com/cbsinteractive/hls.js: no message

Update cmcd API docs

933cf1c

mangui previously approved these changes Nov 8, 2021

View reviewed changes

tjenkinson reviewed Nov 8, 2021

View reviewed changes

Merge pull request #3 from cbsinteractive/feature/cmcd-config-object

2f9b6b6

feature/cmcd-config-object

littlespex dismissed mangui’s stale review via 2f9b6b6 November 8, 2021 21:53

- Remove tokenized custom field code

c8a4dc2

- Make string case the fallback in serialization routine

Merge pull request #4 from cbsinteractive/feature/cmcd-config-object

2aa5d55

- Remove tokenized custom field code

tjenkinson previously approved these changes Nov 9, 2021

View reviewed changes

littlespex added 2 commits November 9, 2021 16:29

Merge branch 'master' into feature/cmcd

dd94557

Update README

3dc8057

littlespex dismissed tjenkinson’s stale review via 3dc8057 November 9, 2021 21:30

mangui requested review from tjenkinson and mangui November 9, 2021 21:49

mangui approved these changes Nov 9, 2021

View reviewed changes

tjenkinson merged commit 0403558 into video-dev:master Nov 10, 2021

robwalch added this to the 1.1.0 milestone Nov 10, 2021

tjenkinson mentioned this pull request Nov 11, 2021

set headers after XMLHttpRequest OPEN #4424

Merged

3 tasks

robwalch added the CMCD label Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate CMCD data into outgoing requests. #4346

Integrate CMCD data into outgoing requests. #4346

littlespex commented Oct 6, 2021 •

edited

Loading

dsparacio left a comment

mangui left a comment

tjenkinson left a comment

tjenkinson Nov 8, 2021

tjenkinson Nov 8, 2021

littlespex commented Nov 8, 2021 •

edited

Loading

tjenkinson commented Nov 9, 2021

littlespex commented Nov 9, 2021

tjenkinson left a comment

littlespex commented Nov 9, 2021

tjenkinson commented Nov 9, 2021

mangui commented Nov 9, 2021

tjenkinson commented Nov 10, 2021

Integrate CMCD data into outgoing requests. #4346

Integrate CMCD data into outgoing requests. #4346

Conversation

littlespex commented Oct 6, 2021 • edited Loading

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

dsparacio left a comment

Choose a reason for hiding this comment

mangui left a comment

Choose a reason for hiding this comment

tjenkinson left a comment

Choose a reason for hiding this comment

tjenkinson Nov 8, 2021

Choose a reason for hiding this comment

tjenkinson Nov 8, 2021

Choose a reason for hiding this comment

littlespex commented Nov 8, 2021 • edited Loading

tjenkinson commented Nov 9, 2021

littlespex commented Nov 9, 2021

tjenkinson left a comment

Choose a reason for hiding this comment

littlespex commented Nov 9, 2021

tjenkinson commented Nov 9, 2021

mangui commented Nov 9, 2021

tjenkinson commented Nov 10, 2021

littlespex commented Oct 6, 2021 •

edited

Loading

littlespex commented Nov 8, 2021 •

edited

Loading