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

Define EME promise fulfill behavior on hardware context reset #494

Open
xhwang-chromium opened this issue Mar 16, 2022 · 10 comments
Open

Define EME promise fulfill behavior on hardware context reset #494

xhwang-chromium opened this issue Mar 16, 2022 · 10 comments
Assignees
Labels
Milestone

Comments

@xhwang-chromium
Copy link
Contributor

Now a MediaKeySession can be closed by itself when hardware context is reset (see #473). If there is a pending promise on the session, e.g. pending generateRequest(), it's unclear what the user agent should do with the promise. I can see two options:

  1. Reject the promise because the user agent cannot complete the operation due to hardware context reset. But a rejected promise could trigger a JS player error, which defeats the purpose of providing the MediaKeySessionClosedReason for this non-failure case.

  2. Resolve the promise (pretending it succeeded). Even though we didn't really finished the operation, it won't make a difference in real world because the session will be closed immediately with hardware-context-reset anyways. And this can help prevent unnecessary JS player error.

I propose we do option (2), and define this behavior in the EME spec.

Any thoughts or comments?

@joeyparrish
Copy link
Member

Sounds good to me. If option 2 is unpalatable for others, though, I'm also fine with option 1.

@gregwfreedman, your thoughts?

@jernoble, any feedback from Safari's perspective?

@joeyparrish joeyparrish added this to the V2-Bugfixes milestone Mar 16, 2022
@gregwfreedman
Copy link
Contributor

Option 2 also sounds good to me.

@xhwang-chromium
Copy link
Contributor Author

xhwang-chromium commented Sep 16, 2022

I prefer option 2 as well.

The implementation in Chromium is tracked at https://bugs.chromium.org/p/chromium/issues/detail?id=1298192

@xhwang-chromium
Copy link
Contributor Author

Another corner case. If hardware context reset happened during a close() call by the application, should the MediaKeySessionClosedReason be closed-by-application or hardware-context-reset? I assume both are fine.

@joeyparrish
Copy link
Member

Sounds like a race. I think either would be fine, and it might be difficult to control the ordering of two events generated independently by different threads anyway.

@xhwang-chromium
Copy link
Contributor Author

Sounds good. Thanks for the perspective!

@gregwfreedman
Copy link
Contributor

I agree, either is fine.

@joeyparrish joeyparrish modified the milestones: V2-Bugfixes, V2 Mar 22, 2023
@joeyparrish
Copy link
Member

I'm going to add to the "CDM Unavailable" algorithm. It currently uses the internal-error reason, but otherwise the shape of the algorithm is exactly what we want to use to close all sessions. I'll add a parameter to specify the reason, which will allow hardware-context-reset to be used.

Then that algorithm can be invoked from the "Monitor for CDM State Changes" to give an explicit location to notice and react to hardware-context-reset.

Finally, generateRequest's steps can be modified so that its promise is resolved during "CDM Unavailable".

joeyparrish added a commit that referenced this issue May 14, 2024
Related to issue #494, where we want to specify `generateRequest` resolution on `hardware-context-reset`.
@joeyparrish
Copy link
Member

@chrisn, @gregwfreedman, @xhwang-chromium, @tidoust:

I'm unsure how to complete the changes for this. I know how I would structure an implementation, but I'm not sure how to convey it in spec. I need some advice if anyone knows the right way to state this in spec language:


In generateRequest's steps, it currently states:

Let promise be a new promise.

Ultimately, I want another algorithm to be able to resolve this and any other pending promises from generateRequest().

What I would do as an implementor is keep a list, set, or other structure of all unresolved generateRequest promises on MediaKeys. This would be a private structure in C++, not an attribute of MediaKeys in JavaScript.

When running the "CDM Unavailable" algorithm, which closes all open sessions, I would first resolve all pending promises from generateRequest.

In "CDM Unavailable", we say:

For each MediaKeySession created by the media keys that is not closed, ...

To do what I want, is it as simple as saying something like:

For each pending promise from generateRequest(), ...

Or do I need to say something in generateRequest about how to store and manage those promises (removing them from tracking once resolved or rejected, for example)?

@joeyparrish joeyparrish self-assigned this May 14, 2024
@chrisn
Copy link
Member

chrisn commented May 22, 2024

I think it's good to write the exact steps in the spec. This would mean adding an internal slot to MediaKeySession for the list of pending promises. It's not exactly the same, but have a look at WebCodecs pending flush promises.

joeyparrish added a commit that referenced this issue May 23, 2024
Related to issue #494, where we want to specify `generateRequest` resolution on `hardware-context-reset`.


<!--
    This comment and the below content is programmatically generated.
    You may add a comma-separated list of anchors you'd like a
    direct link to below (e.g. #idl-serializers, #idl-sequence):

    Don't remove this comment or modify anything below this line.
    If you don't want a preview generated for this pull request,
    just replace the whole of this comment's content by "no preview"
    and remove what's below.
-->
***
<a href="https://pr-preview.s3.amazonaws.com/w3c/encrypted-media/pull/537.html" title="Last updated on May 14, 2024, 10:21 PM UTC (efd8b85)">Preview</a> | <a href="https://pr-preview.s3.amazonaws.com/w3c/encrypted-media/537/c01e7e5...efd8b85.html" title="Last updated on May 14, 2024, 10:21 PM UTC (efd8b85)">Diff</a>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants