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

Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

stevemayhew
Copy link
Contributor

This is a first pass at what we talked about in issue #1001

  • added the wrapper class KeyLoadInfo to embody the requests required to fetch the key
  • added KeyLoadInfo to the DrmSessionEventListener.drmKeysLoaded() method
  • Deprecated DrmSessionEventListener.onDrmKeysLoaded(int, MediaPeriodId)
  • added new onDrmKeysLoaded() with the KeyLoadInfo, setup so both method are called

Next steps are to add it to AnalyticsListener and update any broken test cases possibly add some new ones

@@ -644,6 +651,19 @@ public void handleMessage(Message msg) {
break;
case MSG_KEYS:
response = callback.executeKeyRequest(uuid, (KeyRequest) requestTask.request);
if (currentKeyLoadInfo != null) {
String requestUrl = ((KeyRequest) requestTask.request).getLicenseServerUrl();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs some work, the knowledge of URL's and such is really all private to the MediaDrmCallback so either we need to:

  1. Ignore this and provide vacuous URL's (which, unless the KeyRequest has the data this is already true)
  2. add methods to MediaDrmInfo to fill in the LoadEventInfo

Our use case does not care about the URL, simply the load times and retries. So either way is good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, i wonder if there's another approach here where we add plumbing to pass a TransferListener into HttpMediaDrmCallback which then passes it on to the underlying DataSource impl. That would give you access to the 'real' URL (including redirects handled inside the DataSource as I flagged below).

Downsides of this approach:

  1. Not (easily) compatible with LocalMediaDrmCallback and other MediaDrmCallback impls. So what would we do if we didn't have an instance of HttpMediaDrmCallback?
  2. It leaks the DataSource instance back out to DefaultDrmSession (probably not that big a deal).

Our use case does not care about the URL, simply the load times and retries. So either way is good.

Or maybe we can skip URL for now then (in the spirit of YAGNI). That means we can't use LoadEventInfo. What is the minimal set of fields you need? I feel we should include some way to identify which DRM keys are being loaded - maybe the @Nullable List<SchemeData> that we pass into ExoMediaDrm.getKeyRequest to generate the request bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icbaker, thanks all great ideas... How about we just let the MediaDrmCallback built the object, most of the info is already in the HttpMediaDrmCallback, the other implementations MediaDrmCallback don't have to change as the default (null return) makes sense for them.

  /**
   * Get the {@link LoadEventInfo} for the last executed request.
   * This can be null if no load is performed to execute the request
   *
   * @return the {@link LoadEventInfo} or null if no load was performed
   */
  @Nullable default LoadEventInfo getLastLoadEventInfo() { return null; }
  

With this, the LoadEventInfo is easy to produce so it is completely internally consistent (redirect included).

BTW, a few WV License Proxies we work with use delayed re-directs to manage load (Apache Traffic Server's "thundering herd" for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See latest commit, adds this method. With the LoadEventInfo produced in the object responsible for the actual load request it is completely consistant with what Loader would produce.

@@ -699,6 +719,7 @@ private boolean maybeRetryRequest(Message originalMsg, MediaDrmCallbackException
// The error is fatal.
return false;
}
currentKeyLoadInfo.addRetryLoadRequest(loadEventInfo);
synchronized (this) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be useful if you want the trails of LoadEventInfo's including redirects in the path. The main LoadEventInfo should include all the time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will report all (most? any?) redirects - because at least some types of redirect will be 'silently' handled inside the HttpDataSource implementation underneath HttpMediaDrmCallback, e.g.

So I think we need to be quite careful we can usefully describe what this field holds, because i think it will only show retries that happened at this DefaultDrmSession layer.

Copy link
Contributor Author

@stevemayhew stevemayhew Feb 27, 2024

Choose a reason for hiding this comment

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

That is certainly true for DefaultHttpDataSource, not sure for Cronet. something we'll look into, as our servers are using this as a method to hold off clients when they are loaded. Also, the POST data is retained in the HttpMediaDrmCallback so not sure if the POST redirect in the stack will do this correctly.

We'll have a look and let you know, definitely preference would be to do it in the HttpMediaDrmCallback as the default redirect following code in lower layers usually is much higher than 5 tries.

FWIW this is the same as how other Loader / Loadable implementations handle this.

Definitely something to look into.

@@ -489,6 +492,9 @@ private long getLicenseDurationRemainingSec() {

private void postKeyRequest(byte[] scope, int type, boolean allowRetry) {
try {
if (type == ExoMediaDrm.KEY_TYPE_STREAMING) {
currentKeyLoadInfo = new KeyLoadInfo();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably just check for mode != DefaultDrmSessionManager.MODE_RELEASE. That way we cover download requests too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think we should probably include release calls too, since they can theoretically also take time right?

In any case, we should indicate what type of request is being reported in KeyLoadInfo. But not sure exactly how to do that. @Mode is on DefaultDrmSessionManager so we can't really reference it from a 'generic' DRM context. Maybe @ExoMediaDrm.KeyType would work instead (though it feels kinda low-level).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds like a good idea. We can start a list for what to add to KeyLoadInfo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done in the latest commit, except for adding the KeyLoadInfo to the drmKeysRemoved() method. I'm fine with doing this too, however then we should rename KeyLoadInfo to KeyRequestInfo. I'm all for the YAGNI thinking too ;-)

But, seems most of the work will be rebasing all the files open for this change so it is probably now or never.

I'll follow your lead on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with doing this too, however then we should rename KeyLoadInfo to KeyRequestInfo

Yeah, I think we should do this rename and include key releasing - I agree it's going to be easier to do this now than later.

@icbaker icbaker self-assigned this Feb 27, 2024
@icbaker icbaker self-requested a review February 27, 2024 08:44
@@ -489,6 +492,9 @@ private long getLicenseDurationRemainingSec() {

private void postKeyRequest(byte[] scope, int type, boolean allowRetry) {
try {
if (type == ExoMediaDrm.KEY_TYPE_STREAMING) {
currentKeyLoadInfo = new KeyLoadInfo();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think we should probably include release calls too, since they can theoretically also take time right?

In any case, we should indicate what type of request is being reported in KeyLoadInfo. But not sure exactly how to do that. @Mode is on DefaultDrmSessionManager so we can't really reference it from a 'generic' DRM context. Maybe @ExoMediaDrm.KeyType would work instead (though it feels kinda low-level).

@@ -699,6 +719,7 @@ private boolean maybeRetryRequest(Message originalMsg, MediaDrmCallbackException
// The error is fatal.
return false;
}
currentKeyLoadInfo.addRetryLoadRequest(loadEventInfo);
synchronized (this) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will report all (most? any?) redirects - because at least some types of redirect will be 'silently' handled inside the HttpDataSource implementation underneath HttpMediaDrmCallback, e.g.

So I think we need to be quite careful we can usefully describe what this field holds, because i think it will only show retries that happened at this DefaultDrmSession layer.

new LoadEventInfo(
requestTask.taskId,
new DataSpec.Builder().setUri(requestUrl).build(),
Uri.parse(requestUrl),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also meant to the be the URI after redirects, so we need to be careful here too.

@@ -644,6 +651,19 @@ public void handleMessage(Message msg) {
break;
case MSG_KEYS:
response = callback.executeKeyRequest(uuid, (KeyRequest) requestTask.request);
if (currentKeyLoadInfo != null) {
String requestUrl = ((KeyRequest) requestTask.request).getLicenseServerUrl();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, i wonder if there's another approach here where we add plumbing to pass a TransferListener into HttpMediaDrmCallback which then passes it on to the underlying DataSource impl. That would give you access to the 'real' URL (including redirects handled inside the DataSource as I flagged below).

Downsides of this approach:

  1. Not (easily) compatible with LocalMediaDrmCallback and other MediaDrmCallback impls. So what would we do if we didn't have an instance of HttpMediaDrmCallback?
  2. It leaks the DataSource instance back out to DefaultDrmSession (probably not that big a deal).

Our use case does not care about the URL, simply the load times and retries. So either way is good.

Or maybe we can skip URL for now then (in the spirit of YAGNI). That means we can't use LoadEventInfo. What is the minimal set of fields you need? I feel we should include some way to identify which DRM keys are being loaded - maybe the @Nullable List<SchemeData> that we pass into ExoMediaDrm.getKeyRequest to generate the request bytes?

@stevemayhew
Copy link
Contributor Author

Unless we want to go for renaming KeyLoadInfo to KeyRequestInfo and moving forward with the drmKeysRemoved() update too... I would squash the commits and run the code beautifier on what we have.

I'm good either way.

@stevemayhew stevemayhew force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from d835d9b to d245f71 Compare February 28, 2024 01:11
SystemClock.elapsedRealtime(),
/* loadDurationMs= */ SystemClock.elapsedRealtime() - startTimeMs,
((byte []) response).length);
return response;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating the LoadEventInfo in the actual HttpMediaDrmCallback addresses all the todo's with its correctness issues. Also note now the loadDurationMs does not include any queuing time in the RequestHandler (so it's pure network and proxy overhead measured).

Lastly, DataSource.getLastOpenedUri() should get the redirected URI (if any), at least this is the contract for DataSource.getUri()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks generally good, thanks

Copy link
Collaborator

@icbaker icbaker left a comment

Choose a reason for hiding this comment

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

Sorry this took me so long to get back to, I'll try harder not to leave it so long on the next round of comments!

byte[] response = Util.toByteArray(inputStream);
lastLoadEventInfo =
new LoadEventInfo(
-1, // note this is replaced with the actual taskId from the request
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about plumbing an (optional) taskId into ExoMediaDrm.KeyRequest and ExoMediaDrm.ProvisionRequest? Then it would be available here and you wouldn't need to use this placeholder value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is a bit ugly this way... I stopped short because changing the MediaDrmCallback.executeProvisionRequest() signature, as it has lots of implementations (unless we add new methods, which is also kind of clumsy feeling. What if we added the value to ProvisionRequest and KeyRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icbaker this may fall into the category of perfect is the enemy of done category, but yes... It looks distatesful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we added the value to ProvisionRequest and KeyRequest

This is what I originally intended, but I just had a play around with this and it would require adding taskId to ExoMediaDrm.getKeyRequest, so it also has interface evolution problems (and this would also be the first piece of KeyRequest that isn't derived directly (or slightly indirectly) from the framework MediaDrm.getKeyRequest.


I agree that your current approach is probably the least bad option here.

However unfortunately while thinking about this I spotted a different/bigger problem which I'll raise in a new comment.

SystemClock.elapsedRealtime(),
/* loadDurationMs= */ SystemClock.elapsedRealtime() - startTimeMs,
((byte []) response).length);
return response;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks generally good, thanks


/** The DRM {@link SchemeData} that identifes the loaded key
*/
public final List<SchemeData> schemeDatas;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please make these two ImmutableList and take defensive copies in the constructor below

You can use ImmutableList.Builder for retriedLoadRequests in the builder, and then ImmutableList.copyOf in the constructor will be a no-op because it does an instanceof ImmutableList check first.

@@ -489,6 +492,9 @@ private long getLicenseDurationRemainingSec() {

private void postKeyRequest(byte[] scope, int type, boolean allowRetry) {
try {
if (type == ExoMediaDrm.KEY_TYPE_STREAMING) {
currentKeyLoadInfo = new KeyLoadInfo();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with doing this too, however then we should rename KeyLoadInfo to KeyRequestInfo

Yeah, I think we should do this rename and include key releasing - I agree it's going to be easier to do this now than later.

@stevemayhew
Copy link
Contributor Author

stevemayhew commented Apr 17, 2024 via email

@stevemayhew
Copy link
Contributor Author

I'm on vacation next week @icbaker so it will take a bit to get to these changes. Only one I need your input on is best path for the taskId

stevemayhew added a commit to stevemayhew/media that referenced this pull request Apr 19, 2024
This should cover the comments:

1. androidx#1134 (comment)
2. androidx#1134 (comment)

Also, fixed lint issues and deaI will the nullability of `schemeDatas`
note will need addtional idenfitying info in KeyRequestInfo to support
offline
public Builder(List<SchemeData> schemeDatas) {
this.schemeDatas = schemeDatas;
retriedLoadRequests = new ArrayList<>();
loadEventInfo = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, in the renamed version handled the case where schemeDatas can be null (the caller has a Nullable member here).

* Encapsulates info for the sequence of load requests ({@link LoadEventInfo}, which were required
* to complete loading a DRM key
*/
public class KeyRequestInfo {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactor renamed, in prep for other license requests beside Key load.

@MonotonicNonNull private LoadEventInfo loadEventInfo;
private final List<LoadEventInfo> retriedLoadRequests;
@Nullable private final List<SchemeData> schemeDatas;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clean up the lint, schemeDatas is nullable for offline keys (we may want to add an additional identifying member for this case).

loadEventInfo is non-null after the main request is indemnified (which must be before build()

icbaker pushed a commit to stevemayhew/media that referenced this pull request Apr 22, 2024
This should cover the comments:

1. androidx#1134 (comment)
2. androidx#1134 (comment)

Also, fixed lint issues and deaI will the nullability of `schemeDatas`
note will need addtional idenfitying info in KeyRequestInfo to support
offline
@icbaker icbaker force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from 9d27d45 to 180b20e Compare April 22, 2024 08:50
stevemayhew and others added 5 commits October 3, 2024 12:40
This is a first pass at what we talked about in issue androidx#1001

* added the wrapper class `KeyLoadInfo` to embody the requests required  to fetch the key
* added `KeyLoadInfo` to the `DrmSessionEventListener.drmKeysLoaded()` method
* Deprecated `DrmSessionEventListener.onDrmKeysLoaded(int, MediaPeriodId)`
* added new `onDrmKeysLoaded()` with the `KeyLoadInfo`, setup so both method are called

Next steps are to add it to `AnalyticsListener` and update any broken test cases possibly add some new ones
Addressed comments from pull request:

1. Converted `KeyLoadInfo` to builder pattern, made immutable
2. Added `SchemaData` for the loaded key to the info
3. Added method `MediaDrmCallback.getLastLoadEventInfo()`, allows the called object
   to create a true to spec `LoadEventInfo` object
This is just renaming. I will squash this commit out ultimately, but
making it explict for pull request review purpose.

Only other change was making the constructor private
This should cover the comments:

1. androidx#1134 (comment)
2. androidx#1134 (comment)

Also, fixed lint issues and deaI will the nullability of `schemeDatas`
note will need addtional idenfitying info in KeyRequestInfo to support
offline
@stevemayhew stevemayhew force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from 180b20e to d7d54f4 Compare October 4, 2024 18:02
@stevemayhew
Copy link
Contributor Author

This latest force-push was to rebase to latest main branch, dropped changes to HttpMediaDrmCallback to "resolve" the conflicts. Will add code back that was moved from HttpMediaDrmCallback to DrmUtil.executePost() in the next commit / push.

@stevemayhew stevemayhew force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from 8201f43 to 7cc8931 Compare October 4, 2024 20:09
1. Restore creation of `LoadEventInfo` to the code that was migrated to `DrmUtil.executePost()`
   that was removed in the rebase.
2. Refactor change signature of `MediaDrmCallback` to return both the byte[] response and an
   assoicated `LoadEventInfo`
@stevemayhew stevemayhew force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from 7cc8931 to 4a00318 Compare October 4, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants