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

generateRequest() should allow the first message to not be a license request based on initData #31

Closed
ddorwin opened this issue Feb 10, 2015 · 14 comments

Comments

Projects
None yet
4 participants
@ddorwin
Copy link
Contributor

commented Feb 10, 2015

The generate license request algorithm was always intended to support multiple messages/round trips. This could be part of the protocol or for other things, such as to obtain a server certificate when one has not been provided otherwise and an individualization-request message (see issue #19). The algorithm (now generateRequest()) has evolved to require the first message be a license request generated from the initData.

We should change the algorithm to allow such other types of messages while still ensuring that a license request is generated (in the absence of errors) following the existing algorithm. This probably involves breaking that algorithm out and specifying when to call it from the generateRequest() and update() algorithms.

We need to address #19 first so that we can make these changes in a way that ensures the correct ordering.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2015

Since #19 seeks to ensure that events are not fired until after the promise is resolved, we will need to ensure that these changes maintain that. For example, we may need to ensure that initData, etc. are processed (and perhaps the request partially generated) before the promise is resolved so that any errors can be reported by rejecting the returned promise.

Thus, rather than extracting the generation algorithm, we may just need to save and restore its state.

@cpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

Yes, sometimes we will need the first message to be an indiv request, and subsequent messages may be required until a license request can be sent.

If multiple messages can be sent after the promise returned by a generateRequest() call is resolved, we'll need a way to report errors that result from the responses from those errors. I'm not sure if reporting errors by rejecting the promises returned by subsequent update() calls (as you proposed in issue #14 ) is sufficient. I suspect it is, as the only thing not covered by this would be a network timeout, but that can be handled by the JS layer.

@ddorwin ddorwin modified the milestone: V1 Oct 20, 2015

@steelejoe

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2015

I don't believe this should be blocked by issue #19. I do believe this is blocked by issue #41.

If issue #41 is resolved to allow for not sending requests when they are not required by the CDM, then the text change is very simple:

Generates a license request based on the initData. A message of type "individualization-request" or "license-request" may be queued if the algorithm succeeds and the promise is resolved.

If issue #41 is deferred or resolved to continue requiring requests to always be generated, then the text is also very simple:

Generates a license request based on the initData. A message of type "individualization-request" or "license-request" will be queued if the algorithm succeeds and the promise is resolved.

In either case the update 6.6.5.2 algorithm must change to reflect the new message type added. I believe we should replace the current text:

Let message type be the appropriate MediaKeyMessageType for the message.

With this:

Let message type be either "individualization-request" or "license-request" depending on which is the appropriate MediaKeyMessageType for the message.

I don't believe that any of this text would change based on the resolution of issue #19.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

Resolution of #19 might change the algorithms in ways that would affect the resolution of this issue, which could include a sub-algorithm. I would like to resolve #19 clearly, since that is very important, before tackling any changes related to this issue.

This is the meaning of the blocked label. Once #19 is resolved (and we reach a conclusion on #41 for v1), this will become needs implementation, though there might be some discussion of exact changes.

@ddorwin ddorwin removed the blocked label Apr 12, 2016

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

Are we in agreement that the message generated by generateRequest may be of type "individualization-request" as well as of type "license-request" ?

@mwatson2 mwatson2 self-assigned this May 13, 2016

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 13, 2016

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 13, 2016

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

Pull Request for this issue: #190

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2016

Of the current message types, I think it only makese sense for "license-request" or "individualization-request" to be the result of generateRequest(). (We are not explicit about the type for a server certificate request message, though I assume that is "license-request" since it should be handled by the same server.)

While the current #190 allows the second type, it does not provide any mechanism for later generating a "license-request" message based on the provided initData nor solve the larger issue outlined in the original comment. To fully address this issue, I believe we need to store some type of pending request state and check it in update() calls. We may also want to extract much of the CDM portion of the generateRequest() algorithm so it can be called from both places.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

I agree it would be useful to model the CDM state to better describe the expected behavior in terms of message sequences etc. However, the specification has so far not addressed this, allowing CDMs to fully define message sequences, support multi-message license flows, spontaneous license renewal message flows etc.

I wonder if that next level of detail with respect to CDM message sequences should be left to VNext ?

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

Also, we have long agreed that there can be an individualization exchange as represented by the inclusion of this message type. But as the specification stands, it cannot be used (at least, not at the start of a session). It's clearly a V1 issue to fix that, but I do not see any way we can address the wider issue you raise in the timeframe we need to.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

I propose we handle this in two phases:

  1. In the current PR:
    • Add a statement that if message does not contain the request for the parameters, the implementation SHOULD/MUST store the inputs and generate a request in update(). (This covers the intent.)
    • Add an Issue box referencing this issue that says we need to describe this in the algorithms.
  2. After committing the PR, move this to V1NonBlocking to track the remaining work.
@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

I'm fine adding a requirement that a message of type license-requestgenerated from the initData must be sent eventually, though I am not sure how you test this. This aligns with the current intent.

Specifying CDM state that drives message exchange sequences is a departure from the current architecture where we leave all this CDM behavior unspecified. I'm ok with it myself, but we should have a wider discussion of whether this is agreeable to all.

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 21, 2016

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 21, 2016

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

I have updated the PR.

@mwatson2

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

@ddorwin @mwatson2 @jdsmith3000 reviewed this. Waiting for final review from @ddorwin.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2016

@mwatson2, I provided feedback in the PR and filed #207 to track the future work as discussed.

mwatson2 added a commit to mwatson2/encrypted-media that referenced this issue May 31, 2016

@mwatson2 mwatson2 closed this in 2fe3f27 May 31, 2016

@mwatson2 mwatson2 removed the needs review label May 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.