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

Problem with the way DefaultDrmSessionManager releases its references to DrmSession-s #8576

Closed
sruditsky opened this issue Feb 10, 2021 · 3 comments

Comments

@sruditsky
Copy link

There are several situations in which it would be desirable to re-prepare ExoPlayer with a different media source while keeping the DrmSession from the previous prepare.
For example, there may be several different media sources which have the same DRM info or it is required to retry a failed playback.
In such cases not keeping the DrmSession would mean an extra license request, which takes time and consumes resources

We had a solution for this which in essence does the following:

defaultDrmSessionManager.prepare()
session = defaultDrmSessionManager.acquireSession() 

// acquire session so we can keep it
session.acquire(null) 

session.release(null)

defaultDrmSessionManager.release()
defaultDrmSessionManager.prepare()
defaultDrmSessionManager.release()
defaultDrmSessionManager.prepare()

// release the session when we do not need it anymore
session.release(null)   

defaultDrmSessionManager.release()

This worked as we expected it to in ExoPlayer 2.9.4, however, we recently upgraded to ExoPlayer 2.12.2 and this mechanism does not work anymore.

Two reasons why it is no longer working are both in 2.12.2's implementation of DefaultDrmSessionManager.release():

a) when a DefaultDrmSessionManager is constructed with sessionKeepaliveMs == C.TIME_UNSET
calling DefaultDrmSessionManager.release() releases DrmSessions which it did not acquire.
b) DefaultDrmSessionManager.release() releases DrmSessions, however it does not forget them -- they are still kept in the
DefaultDrmSessionManager.sessions list.

The sequence of

defaultDrmSessionManager.prepare()
defaultDrmSessionManager.release()

always releases the DrmSession objects which are stored in the sessions list, while it does not acquire them (in the same sequence) and this eventually causes a license request (or IllegalStateException).

@icbaker
Copy link
Collaborator

icbaker commented Feb 11, 2021

a) when a DefaultDrmSessionManager is constructed with sessionKeepaliveMs == C.TIME_UNSET
calling DefaultDrmSessionManager.release() releases DrmSessions which it did not acquire.

This is definitely a bug (introduced by me in 316f8a8 - sorry!). By complete coincidence I actually spotted this on Tuesday when doing other work in DefaultDrmSessionManager, so I already have a fix in review.

However, assuming there's no other prepare() or release() calls on the manager that aren't shown in your code snippet, I'm not sure the sequence of calls you're making is valid because it seems to me that you frequently allow the internal reference count of the manager to reach zero (which makes the bug have a much bigger impact than it "should"):

// manager ref-count = 0
defaultDrmSessionManager.prepare()
// manager ref-count = 1
session = defaultDrmSessionManager.acquireSession() 

// acquire session so we can keep it
session.acquire(null) 

session.release(null)

defaultDrmSessionManager.release()
// manager ref-count = 0
defaultDrmSessionManager.prepare()
// manager ref-count = 1
defaultDrmSessionManager.release()
// manager ref-count = 0
defaultDrmSessionManager.prepare()
// manager ref-count = 1

// release the session when we do not need it anymore
session.release(null)   

defaultDrmSessionManager.release()
// manager ref-count = 0

When the manager reference count reaches zero, it releases it's underlying ExoMediaDrm object:


In the 'default' case, this ExoMediaDrm will be an ExoPlayer FrameworkMediaDrm wrapping an Android MediaDrm. This same object is used to back all the DrmSession instances acquired from the manager. FrameworkMediaDrm also has an internal reference count. If that reaches zero it also releases its underlying MediaDrm:
if (--referenceCount == 0) {
mediaDrm.release();
}

When (re-)preparing and incrementing the reference count from 0 to 1, the manager reacquires the ExoMediaDrm, which may well reinstantiate a new MediaDrm object (if the last one was released):

exoMediaDrm = exoMediaDrmProvider.acquireExoMediaDrm(uuid);

This means that after releasing & re-preparing the manager, the underlying MediaDrm object might be different (with the previous one having been released). This means any DrmSession instances acquired from the previous 'version' of the manager will no longer be functional (because their underlying MediaDrm object will have been released).

So I don't think it's valid to keep a DrmSession alive longer than the manager it came from. This isn't documented on either DrmSession or DrmSessionManager, but it should be.

In your case, I think you can fix this by preparing the manager manually at the start of your code snippet, then finally releasing it when you're completely done. This means the reference count will oscillate between 1 and 2 (instead of 0 and 1). So it would be something like:

defaultDrmSessionManager.prepare()
defaultDrmSessionManager.prepare()
session = defaultDrmSessionManager.acquireSession() 

// acquire session so we can keep it
session.acquire(null) 

session.release(null)

defaultDrmSessionManager.release()
defaultDrmSessionManager.prepare()
defaultDrmSessionManager.release()
defaultDrmSessionManager.prepare()

// release the session when we do not need it anymore
session.release(null)   

defaultDrmSessionManager.release()
defaultDrmSessionManager.release()

If you do this you need to be careful the prepare() and release() calls on the manager are only made from the playback thread (available from ExoPlayer#getPlaybackLooper()). This also isn't clearly documented, and should be.


There are several situations in which it would be desirable to re-prepare ExoPlayer with a different media source while keeping the DrmSession from the previous prepare.

If you keep the same manager instance around (with ref-count > 0), could you leverage the keepalive support for this and avoid needing to manually acquire the session? When ExoPlayer calls acquireSession in the new MediaSource, it should find the previous DrmSession instance (assuming the Format.drmInitData matches). This will avoid the extra license request you mention.


There's a recent (unreleased) related DRM change that may be relevant to you. We now try and re-use DefaultDrmSessionManager instances when moving through items in a playlist: a062075

@sruditsky
Copy link
Author

Thank you @icbaker.
Yes, we did implement a solution similar to one you have suggested and it seems to solve the problem.

As for the mechanism of re-using DefaultDrmSessionManager instances, is there any approximate date when it will be released?

ojw28 pushed a commit that referenced this issue Feb 12, 2021
If keepalive is disabled the existing code over-eagerly releases
DrmSession instances. This is arguably OK since a (Default)DrmSession
should be released before its (Default)Manager is released
(since the underlying MediaDrm instance might be released when the
manager is released). And if all sessions are released before the
manager is released then `sessions` is empty, so the loop is a no-op.

Issue: #8576
#minor-release
PiperOrigin-RevId: 356955308
ojw28 pushed a commit that referenced this issue Feb 12, 2021
If keepalive is disabled the existing code over-eagerly releases
DrmSession instances. This is arguably OK since a (Default)DrmSession
should be released before its (Default)Manager is released
(since the underlying MediaDrm instance might be released when the
manager is released). And if all sessions are released before the
manager is released then `sessions` is empty, so the loop is a no-op.

Issue: #8576
PiperOrigin-RevId: 356955308
@ojw28 ojw28 closed this as completed Feb 12, 2021
@ojw28
Copy link
Contributor

ojw28 commented Feb 12, 2021

It will be released in 2.13.1, which should happen very soon. Thanks!

@google google locked and limited conversation to collaborators Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants