Adds load info to `DrmSessionEventListeneronDrmKeysLoaded()` #1134

stevemayhew · 2024-02-26T20:50:48Z

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

stevemayhew · 2024-02-26T20:58:42Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -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();


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

Ignore this and provide vacuous URL's (which, unless the KeyRequest has the data this is already true)

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.

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:

Not (easily) compatible with LocalMediaDrmCallback and other MediaDrmCallback impls. So what would we do if we didn't have an instance of HttpMediaDrmCallback?

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?

@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)

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.

stevemayhew · 2024-02-26T21:00:00Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

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


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.

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.

media/libraries/datasource/src/main/java/androidx/media3/datasource/DefaultHttpDataSource.java

Line 594 in 0480bc3

url = handleRedirect(url, location, dataSpec);

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.

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.

stevemayhew · 2024-02-26T21:00:28Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DrmSessionEventListener.java

   */
-  default void onDrmKeysLoaded(int windowIndex, @Nullable MediaPeriodId mediaPeriodId) {}
+  default void onDrmKeysLoaded(
+      int windowIndex, @Nullable MediaPeriodId mediaPeriodId, @Nullable KeyLoadInfo keyLoadInfo) {}



followed the pattern to deprecate then call the deprecated method.

stevemayhew · 2024-02-26T23:27:08Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -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();
+      }


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

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).

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

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.

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.

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyLoadInfo.java

icbaker · 2024-02-27T08:40:32Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -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();
+      }


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).

icbaker · 2024-02-27T08:45:11Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

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


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.

media/libraries/datasource/src/main/java/androidx/media3/datasource/DefaultHttpDataSource.java

Line 594 in 0480bc3

url = handleRedirect(url, location, dataSpec);

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.

icbaker · 2024-02-27T08:54:44Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

+                  new LoadEventInfo(
+                      requestTask.taskId,
+                      new DataSpec.Builder().setUri(requestUrl).build(),
+                      Uri.parse(requestUrl),


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

icbaker · 2024-02-27T09:01:48Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -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();


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:

Not (easily) compatible with LocalMediaDrmCallback and other MediaDrmCallback impls. So what would we do if we didn't have an instance of HttpMediaDrmCallback?

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 · 2024-02-28T01:05:42Z

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 · 2024-02-28T01:14:53Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/HttpMediaDrmCallback.java

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


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()

Looks generally good, thanks

icbaker

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!

icbaker · 2024-04-16T09:40:52Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/HttpMediaDrmCallback.java

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


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.

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

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

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.

icbaker · 2024-04-16T09:41:25Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/HttpMediaDrmCallback.java

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


Looks generally good, thanks

icbaker · 2024-04-16T09:42:18Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyLoadInfo.java

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


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.

icbaker · 2024-04-16T09:44:43Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyLoadInfo.java

+   */
+  public final List<SchemeData> schemeDatas;
+
+  public KeyLoadInfo(Builder builder) {


stylistic/consistency nit: Make this private, and add an explicit public Builder buildUpon() method if needed (though I don't think you do need it?)

Again, made it private in the renamed class. Sorry for this, should have fixed it first then renamed rather than rename then fix.

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/MediaDrmCallback.java

icbaker · 2024-04-16T09:49:35Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -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();
+      }


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 · 2024-04-17T13:55:50Z

No problem, we both have day jobs too! I’ve assigned some tasks here internally to move our most important pull requests over from ExoPlayer to AndroidX

…

On Tue, Feb 27, 2024 at 1:05 AM Ian Baker ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyLoadInfo.java <#1134 (comment)>: > @@ -0,0 +1,26 @@ +package androidx.media3.exoplayer.drm; + +import androidx.media3.exoplayer.source.LoadEventInfo; +import java.util.ArrayList; +import java.util.List; + +/** + * Encapsulates info for the sequence of load requests ***@***.*** LoadEventInfo}, which were required + * to complete loading a DRM key + */ +public class KeyLoadInfo { I think we should make this immutable. If we need the addRetry accumulation logic here then it should be part of a KeyLoadInfo.Builder, but maybe we don't need that and can just accumulate in a local in DefaultDrmSession. ------------------------------ In libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java <#1134 (comment)>: > @@ -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(); + LoadEventInfo loadEventInfo = + new LoadEventInfo( + requestTask.taskId, + new DataSpec.Builder().setUri(requestUrl).build(), + Uri.parse(requestUrl), This is also meant to the be the URI after redirects, so we need to be careful here too. ------------------------------ In libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java <#1134 (comment)>: > @@ -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(); 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 <https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it>). 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? ------------------------------ In libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java <#1134 (comment)>: > @@ -699,6 +719,7 @@ private boolean maybeRetryRequest(Message originalMsg, MediaDrmCallbackException // The error is fatal. return false; } + currentKeyLoadInfo.addRetryLoadRequest(loadEventInfo); synchronized (this) { 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. https://github.com/androidx/media/blob/0480bc31a8fbec2aed354bee14e8f15e7fe9012a/libraries/datasource/src/main/java/androidx/media3/datasource/DefaultHttpDataSource.java#L594 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. ------------------------------ In libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java <#1134 (comment)>: > @@ -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(); + } 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). — Reply to this email directly, view it on GitHub <#1134 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBF6GK7DWFHTJHJRPHCJDYVWOVZAVCNFSM6AAAAABD24S3JCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBSHA3DAMZTGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

stevemayhew · 2024-04-19T16:42:26Z

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

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 · 2024-04-19T19:06:47Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyLoadInfo.java

+    public Builder(List<SchemeData> schemeDatas) {
+      this.schemeDatas = schemeDatas;
+      retriedLoadRequests = new ArrayList<>();
+      loadEventInfo = null;


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

stevemayhew · 2024-04-19T19:07:37Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyRequestInfo.java

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


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

stevemayhew · 2024-04-19T19:09:11Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/KeyRequestInfo.java

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


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()

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

icbaker · 2024-04-22T09:29:55Z

libraries/exoplayer/src/main/java/androidx/media3/exoplayer/drm/DefaultDrmSession.java

@@ -666,6 +672,12 @@ public void handleMessage(Message msg) {
            break;
          case MSG_KEYS:
            response = callback.executeKeyRequest(uuid, (KeyRequest) requestTask.request);
+            if (currentKeyRequestInfo != null) {
+              LoadEventInfo loadEventInfo = callback.getLastLoadEventInfo();


Unfortunately because of the threading model used here, I don't think we can always guarantee that getLastLoadEventInfo() here corresponds to the request made above on L674.

This can happen because each DefaultDrmSession instance has its own request thread, but two instances can easily share the same MediaDrmCallback instance.

Concretely this can happen during a fairly normal playback if a separate DefaultDrmSession is opened for audio and video (encrypted with different keys I guess?).

So then T1 can execute callback.executeKeyRequest then pause while T2 executes callback.executeKeyRequest before T1 then calls getLastLoadEventInfo() and gets the request from T2.

To resolve this I think its best to tightly couple each request to the reported LoadEventInfo, so we can keep MediaDrmCallback impls 'stateless' so it doesn't matter if they're invoked on multiple threads at the same time.

Which I think leads us to either:

Pass TransferListener into MediaDrmCallback.executeXXXRequest (my previous suggestion in Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134 (comment))

Enrich the return type of MediaDrmCallback.executeKeyRequest to KeyResponse, which has byte[] data field and optional @Nullable LoadEventInfo too.

This requires also passing taskId in somehow, either inside KeyRequest, or as a separater parameter (as we discussed: Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134 (comment)) - both of which are a bit disruptive. I think we shouldn't pass taskId down into ExoMediaDrm, but it does seem to make sense to pass it inside KeyRequest. So maybe we can do that by adding a KeyRequest.copyWithTaskId method? I'll push some commits that sketch that idea.

(1) has the issues I mentioned before with 'this listener might never be called if it's not supoprted by this MediaDrmCallback impl', but I think we might just have to document that limitation (so e.g. a caller must never block waiting for onTransferEnd unless they know more specifics about the MediaDrmCallback type they're dealing with. It also mixes up abstractions a bit, feels like maybe we shouldn't be naively re-using the datasource.TransferListener type in a not-necessarily-DataSource context.

(1) also means moving the LoadEventInfo instantiation back into DefaultDrmSession instead of HttpMediaDrmCallback.

On the other hand, (2) feels a bit weird because we end up returning (possibly) several 'time-based' even things all in one go when the request is fully completed. But it glues more easily into your existing changes to DrmSessionEventListener.

Unfortunately because of the threading model used here, I don't think we can always guarantee that getLastLoadEventInfo() here corresponds to the request made above on L674.

This can happen because each DefaultDrmSession instance has its own request thread, but two instances can easily share the same MediaDrmCallback instance.

Ah, great catch... I feel pretty stupid, given that I just solved this same problem with my custom LoadErrorHandlingPolicy I wound up keeping the state object key'd by the load taskId

When I get back from vacation next week I'll move my original commits to a branch and download your changes (the force push would make a mess if I just pulled it) so I can see what is going on and have a closer look at your suggestions. Thanks!

the force push would make a mess if I just pulled it

Ah sorry, the force push was part of me planning to send this PR for review internally before I realised this larger issue. I didn't push any 'meaningful' changes though, just formatted the changes in a commit, then rebased on top of the latest main commit. You should be able to force the branch pointer back to the 'old' version of ebec293 which I think is 9d27d45a65, then do your own force push to 'take back control' of the PR.

Probably something like this (though I'm no git expert, so there may be a better way to achieve this!):

$ git branch --force p-add-loadinfo-to-ondrmkeysloaded 9d27d45a65 $ git push --force

Unfortunately because of the threading model used here, I don't think we can always guarantee that getLastLoadEventInfo() here corresponds to the request made above on L674.

This can happen because each DefaultDrmSession instance has its own request thread, but two instances can easily share the same MediaDrmCallback instance.

Yes, stashing the LoadEventInfo anywhere other than the RequestTask, which is new'd for each request will violate the statelessness of whatever it is stuffed into. There is no clean way out of just doing this.

To resolve this I think its best to tightly couple each request to the reported LoadEventInfo, so we can keep MediaDrmCallback impls 'stateless' so it doesn't matter if they're invoked on multiple threads at the same time.

Which I think leads us to either:

Pass TransferListener into MediaDrmCallback.executeXXXRequest (my previous suggestion in Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134 (comment))

Enrich the return type of MediaDrmCallback.executeKeyRequest to KeyResponse, which has byte[] data field and optional @Nullable LoadEventInfo too.

also, we could

Add to or lift a common base class from ProvisionRequest and KeyRequest that includes the LoadEventInfo as a member.

Number 3 avoids a signature change for executeXXXRequest, but it seems uglier than your number 2, which is pretty strait forward.

So I would vote for your number 2. I can do it or feel free to branch the current code and make a pull request onto my repo with your idea, I can build and test it. Branches and pull request are best way to collaborate (once we agree the basic approach), but no worries about the force push and thanks for re-basing to keep up with head of main (this is a big set of changes to keep on the side for long)

Thanks again for catching this.

stevemayhew commented Feb 26, 2024

View reviewed changes

stevemayhew mentioned this pull request Feb 26, 2024

Analytics onLoad events for HttpMediaDrmCallback requests #1001

Open

stevemayhew commented Feb 26, 2024

View reviewed changes

icbaker self-assigned this Feb 27, 2024

icbaker self-requested a review February 27, 2024 08:44

icbaker requested changes Feb 27, 2024

View reviewed changes

icbaker added the pending comments label Feb 27, 2024

stevemayhew force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from d835d9b to d245f71 Compare February 28, 2024 01:11

stevemayhew commented Feb 28, 2024

View reviewed changes

stevemayhew requested a review from icbaker February 29, 2024 15:59

icbaker requested changes Apr 16, 2024

View reviewed changes

stevemayhew commented Apr 19, 2024

View reviewed changes

stevemayhew and others added 5 commits April 22, 2024 09:48

Refactor rename KeyLoadInfo to KeyRequestInfo

2ae3798

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

Make lists Immuteable in KeyRequestInfo

ebec293

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

Format with google-java-format

180b20e

icbaker force-pushed the p-add-loadinfo-to-ondrmkeysloaded branch from 9d27d45 to 180b20e Compare April 22, 2024 08:50

icbaker reviewed Apr 22, 2024

View reviewed changes

Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134

Are you sure you want to change the base?

Adds load info to DrmSessionEventListeneronDrmKeysLoaded() #1134

Conversation

stevemayhew commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemayhew Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemayhew commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemayhew commented Apr 17, 2024 via email

stevemayhew commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds load info to `DrmSessionEventListeneronDrmKeysLoaded()` #1134

Adds load info to `DrmSessionEventListeneronDrmKeysLoaded()` #1134

stevemayhew Feb 27, 2024 •

edited