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: Add config for add headers to license requests #6650

Merged
merged 4 commits into from
May 23, 2024

Conversation

avelad
Copy link
Collaborator

@avelad avelad commented May 22, 2024

No description provided.

@avelad
Copy link
Collaborator Author

avelad commented May 22, 2024

I need review the error: Invalid config, unrecognized key .drm.advanced.com.widevine.alpha.headers.X-AxDRM-Message in the demo

@avelad avelad added type: enhancement New feature or request priority: P3 Useful but not urgent labels May 22, 2024
@avelad avelad added this to the v4.9 milestone May 22, 2024
@shaka-bot
Copy link
Collaborator

shaka-bot commented May 22, 2024

Incremental code coverage: 91.30%

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Looks good to me, apart from those leftover console.log statements and any errors you're seeing in the demo

lib/media/drm_engine.js Outdated Show resolved Hide resolved
lib/util/config_utils.js Outdated Show resolved Hide resolved
@avelad
Copy link
Collaborator Author

avelad commented May 22, 2024

I need review the error: Invalid config, unrecognized key .drm.advanced.com.widevine.alpha.headers.X-AxDRM-Message in the demo

@joeyparrish can you help me to resolve it?

@joeyparrish
Copy link
Member

I need review the error: Invalid config, unrecognized key .drm.advanced.com.widevine.alpha.headers.X-AxDRM-Message in the demo

@joeyparrish can you help me to resolve it?

I found the source of the problem. It's really a design problem.

The configuration system wants to map out and type-check every input. However, some parts of the config are dictionaries with arbitrary keys. Ex: drm.advanced[keySystem]...

We still want to type-check the values under those arbitrary keys, so the config system has this "overrides" system. It basically allows the config algorithm to skip a level in the hierarchy. A simplified example:

    const overrides = {
      '.drm.servers': '',
      '.drm.advanced': {
        videoRobustness: '',
        audioRobustness: '',
        sessionType: '',
      },
    };

This means the path .drm.servers is a dictionary with arbitrary keys, and the values are strings. So .drm.servers[keySystem] should always be a string, if present.

And the path .drm.advanced is a dictionary with arbitrary keys, and the values are these more complex objects with videoRobustness, audioRobustness, and sessionType. So .drm.advanced[keySystem] should always be one such object, if present.

The trouble is that headers is a second layer of a dictionary with arbitrary keys, and the hacky system I described above isn't designed to handle that. Ex:

    const overrides = {
      '.drm.advanced': {
        videoRobustness: '',
        audioRobustness: '',
        sessionType: '',
        headers: {},
      },
    };

Now .drm.advanced[keySystem].headers is an object, which is fine. But we don't have any "valid" keys in that object if you go deeper.

I'm pondering the "best" solution, where "best" either means "cleanest", "most extensible", or "quickest"...

@joeyparrish
Copy link
Member

I'm pondering the "best" solution, where "best" either means "cleanest", "most extensible", or "quickest"...

I found a relatively quick solution... the merge system will now recognize "{}" in the template as "just an object, do a shallow copy and don't recurse"

@joeyparrish
Copy link
Member

@avelad, please check the demo and confirm, then merge when you're ready.

@avelad
Copy link
Collaborator Author

avelad commented May 23, 2024

Yes it works, but now the tests are failing

@avelad avelad merged commit e7b893b into shaka-project:main May 23, 2024
15 checks passed
@avelad avelad deleted the drm-headers-config branch May 23, 2024 07:45
let genericObject = false;
if (overrideTemplate) {
genericObject = template.constructor == Object &&
Object.keys(overrides).length == 0;
Copy link
Member

Choose a reason for hiding this comment

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

This is not right. overrides always has keys (eg ".drm.advanced"), and I don't see why you would check it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It’s the only solution that I found to fix the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: P3 Useful but not urgent type: enhancement New feature or request
3 participants