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

Update Common-Media-Client-Data implementation #3402

Closed
wilaw opened this issue Sep 10, 2020 · 11 comments · Fixed by #3420
Closed

Update Common-Media-Client-Data implementation #3402

wilaw opened this issue Sep 10, 2020 · 11 comments · Fixed by #3420
Assignees
Milestone

Comments

@wilaw
Copy link
Member

wilaw commented Sep 10, 2020

The CMCD specification has now completed development at CTA and is available as CTA-5004. It has some changes compared to the draft version used for the initial implementation within dash.js. This issue is a request to bring the dash.js implementation up to spec with the published version. The final v1 verison can be found here:

https://docs.google.com/document/d/1S_Dj_aHVnbWnjeJYMfLU1D6tZoqYdgHT1stfznBvxig/

Changes from version implemented in dash.js

  1. Query arg changed from 'Common-Media-Client-Data' to 'CMCD'
  2. DeviceID key deprecated
  3. Keys are now ordered alphabetically
  4. New 'ot' object types added , now one of [m,a,v,av,i,c,tt,k,o]
  5. Measured throughput, requested maximum throughput, deadline and buffer length must now be rounded to the nearest unit of 100 to reduce fingerprinting entropy.
  6. Version key should only be reported if it is != 1
  7. New keys added:
    1. 'startup' - su
    2. 'top bitrate' - tb
    3. 'buffer starvation' - bs
    4. 'buffer length' - bl (you can stop reporting 'dl' and instead report this value)
@dsilhavy dsilhavy added this to the 3.1.4 milestone Sep 10, 2020
@dsilhavy dsilhavy assigned dsilhavy and FritzHeiden and unassigned dsilhavy Sep 10, 2020
@wilaw
Copy link
Member Author

wilaw commented Nov 5, 2020

@dsilhavy @FritzHeiden - i am trying to use this implementation in 3.1.4 nightly and it seems the CMCD payload is getting double URLencoded.

Per the spec, the query arg string should be URLEncoded. This means it would look something like

?CMCD=nor%3D%22..%252F300kbps%252Fsegment35.m4v%22%2Csid%3D%22 6e2fb550-c457-11e9-bb97-0800200c9a66%22

If we decoded that, it shows as

?CMCD=nor="..%2F300kbps%2Fsegment35.m4v",sid="6e2fb550-c457-11e9-bb97-0800200c9a66"

If I take a sample query arg from 3.1.4 implementation, it looks like this

CMCD=sid%253D%2522341d5f1d-deb5-4228-83e2-c428d3fa6be2%2522%252Ccid%253D%2522725369494%2522%252Cst%253D%2522l%2522%252Csf%253D%2522d%2522%252Cbr%253D3000%252Cot%253D%2522v%2522%252Cd%253D2002%252Cmtp%253D55400%252Cdl%253D3800%252Cbl%253D3800%252Ctb%253D3000000

if we decode this, we get

CMCD=sid%3D%22341d5f1d-deb5-4228-83e2-c428d3fa6be2%22%2Ccid%3D%22725369494%22%2Cst%3D%22l%22%2Csf%3D%22d%22%2Cbr%3D3000%2Cot%3D%22v%22%2Cd%3D2002%2Cmtp%3D55400%2Cdl%3D3800%2Cbl%3D3800%2Ctb%3D3000000

Note that the key/value delimiters and quotes are not accessible. If apply a second level of URIDecode, then we get

CMCD=sid="341d5f1d-deb5-4228-83e2-c428d3fa6be2",cid="725369494",st="l",sf="d",br=3000,ot="v",d=2002,mtp=55400,dl=3800,bl=3800,tb=3000000

which is what we want.

Can you correct the implementation so that only one level of URLencoding is getting applied?

Thanks

Will

@wilaw wilaw reopened this Nov 5, 2020
@dsilhavy
Copy link
Collaborator

dsilhavy commented Nov 6, 2020

@wilaw
Thanks for the testing and the feedback:

  • Additional encodeUriComponent should be removed
  • There is a difference in the network developer tools of Firefox and Chrome. Firefox gives you a decoded version of the url while Chrome prints the encoded one
  • Changes should be visible in nightly, please remember to deactivate the cache to get the latest dist file
  • We are using the URL object of the browser to add new query params: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/set . In order to get a decoded version of the url, decodeURIComponent has to be called on the encoded url string.

@wilaw
Copy link
Member Author

wilaw commented Nov 6, 2020

@dsilhavy - thank you for that quick fix. I can confirm that the double URlEncode has been fixed.

This has revealed two other another problems.

  1. Both the bitrate and top bitrate keys have units of "kbps". However, nightly build seems to be using bps as the units for "tb" (top bitrate)

CMCD: v=1,sid="bfe690f2-1ec0-407d-873a-52babb386bc2",cid="null",st="l",sf="d",br=2000,ot="v",d=2002,mtp=43500,dl=1400,bl=1400,tb=3000000

Request is to correct the units of tb to be kbps.

  1. Second issue is that a cid of "null" is being added. If cid is not defined by the application, then the cid parameter should be omitted.

@dsilhavy
Copy link
Collaborator

dsilhavy commented Nov 9, 2020

@wilaw

  • top bitrate is fixed to kbps return Math.round(info.bitrate / 1000);
  • Regarding cid: We generate a random hash if this value is not set by the application. Consequently if you leave this value blank in the reference UI it should not be null. If you explicitly enter "null" in the reference UI we treat this value as a string since cid is defined as a string. What could be improved is the format of the auto generated hash. This should be a uuid according to the examples in the spec.

@wilaw
Copy link
Member Author

wilaw commented Nov 9, 2020

@dsilhavy - thanks for fixing tb units.

Regarding 'cid' - I would suggest that unless application sets it explicitly, that you do not auto-generate a 'cid' (even as a UUID). The spec only recommends one key to always be added, and that is 'sid'. An implementer who wishes to not send 'cid' would currently have to explicitly set an empty string value, which is a bit strange.

@dsilhavy
Copy link
Collaborator

@wilaw Agreed, we fixed this in #3450

@wilaw
Copy link
Member Author

wilaw commented Nov 10, 2020

Sorry, another issue I see , per item (3) "Keys are now ordered alphabetically"

Current keys from nightly build: CMCD=v%3D1%2Csid%3D%2295de21f0-80cc-4746-9549-e9bf3dfdcdec%22%2Cst%3D%22v%22%2Csf%3D%22d%22%2Cbr%3D67%2Cot%3D%22a%22%2Cd%3D4011%2Cmtp%3D13800%2Cdl%3D4000%2Cbl%3D4000%2Ctb%3D67

Decoded, this is

CMCD=v=1,sid="95de21f0-80cc-4746-9549-e9bf3dfdcdec",st="v",sf="d",br=67,ot="a",d=4011,mtp=13800,dl=4000,bl=4000,tb=67

It should be
CMCD=bl=4000,br=67,d=4011,dl=4000,mtp=13800,ot="a",sf="d",sid="95de21f0-80cc-4746-9549-e9bf3dfdcdec",st="v",tb=67,v=1

@wilaw
Copy link
Member Author

wilaw commented Nov 10, 2020

And one more, per item [6] "Version key should only be reported if it is != 1" , so the v=1 should be removed.

@dsilhavy
Copy link
Collaborator

Thanks for reporting @wilaw your tests are really helpful for us. Both issues should be fixed in nightly: 89e7527

@dsilhavy
Copy link
Collaborator

@wilaw Did you have the chance to test the fixes? Did you encounter any other issues or can I close this ticket?

@wilaw
Copy link
Member Author

wilaw commented Nov 18, 2020

It's looking good. Fixes are confirmed. I did some additional tests to verify that the 'bs' parameter was sent on buffer starvation and they passed. Thanks for all your work on this. If there are any remaining issues, they can be address in future sprints. You may go ahead and close this ticket.

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 a pull request may close this issue.

3 participants