CMCD data is being appended multiple times when playing HLS live streams

[littlespex]

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
3.3.0

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
Both

If custom app, can you reproduce the issue using our demo app?

What browser and OS are you using?
Chrome, Firefox

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?

What configuration are you using? What is the output of player.getConfiguration()?

{
  cmcd: {
    enabled: true
  }
}

What did you do?

1. Load any HLS live stream
2. Let content play until the manfiests/playlists are re-requested

What did you expect to happen?
There should only be one set of CMCD data appended to the query string

What actually happened?

Each re-request appends another CMCD arg.

[littlespex]

@joeyparrish I have a fix for this issue, but it uses URLSearchParams. I'm not sure if this meets the minimum browser support requirements. I've tried using the closure library's goog.Uri and goog.Uri.QueryData classes, but the version that is loaded don't seem to match the documentation. Specifically, I need goog.Uri.QueryData.set, and its coming up undefined.

https://google.github.io/closure-library/api/goog.Uri.html
https://google.github.io/closure-library/api/goog.Uri.QueryData.html#set

[avelad]

WebOS 3.X uses Chromium 38, see https://webostv.developer.lge.com/discover/specifications/web-engine/

The first version of Chromium that includes URLSearchParams is 49…

Sorry…

[littlespex]

I figured URLSearchParams wouldn't meet the minimum requirements. The question is: Do we polyfill it, update the Closure library to a version that supports goog.Uri.QueryData.set,or write yet another manual query string manager (👎)?

[TheModMaker]

I don't think we need to use either of those types, we should check where the params are getting added twice. We should just take the base URL and add the params once, so using a type like that shouldn't be needed.

[littlespex]

The safest way to replace a query param is to use to parse the search params, replace/add the CMCD values, then serialize the string. Using URLSearchParams it's as simple as:

const url = new URL(uri);
url.searchParams.set('CMCD', query);
return url.toString();

Otherwise, we need to manually parse the string, or write a regex that accounts for all the possible use cases (does the search string have only one param or multiple, etc). It seems hack to my to manipulate the string manually when the w3c and the closure library both provide utilities for doing this exactly thing.

Furthermore, in the original PR for the CMCD feature, using hand written utils instead of existing standard's based solutions prevented the PR from being excepted.

[joeyparrish]

The problem with the closure library is that it is very difficult to get "just" one feature. In for a penny, in for a pound. We've tried to depend only on the dependency system (goog.require/goog.provide), which is relatively independent. To use the closure URI classes without dependencies on a million pounds of nonsense (bloating our binary in the process), we have had to fork and strip down the implementation.

If you really want an updated closure URI implementation, I believe we should update our fork in third_party/closure-uri/.

Alternately, this polyfill has no dependencies: https://www.npmjs.com/package/url-search-params-polyfill

As for web standards vs other utilities like Closure, I tried at one time to replace closure-uri entirely with the web standard URI, and I ultimately failed. It parsed differently on different browsers, specifically with non-standard URI schemes like our "offline://". We decided to stick to a JS implementation for consistency.

So in this case, I think it might be best to update the closure-uri folder. Since we already depend on that internally, it's not any additional bloat to the library.

[littlespex]

I will give that a try.