Interface for DRM Visibility #39

kabliz · 2024-08-21T22:52:19Z

Adds DrmState to ArmadilloState, giving full visibility into DRM status to the client, including the expiration date for content.
Splits SocketTimeout from HttpResponseCodeException, into ConnectivityException to better differentiate connectivity difficulties from developer error.
Ensures that ArmadilloState is updated from the main thread consistently.

- Adds DrmState to ArmadilloState, giving full visibility into DRM status to the client, including the expiration date for content. - Splits SocketTimeout from HttpResponseCodeException, into ConnectivityException to better differentiate connectivity difficulties from developer error. - Ensures that ArmadilloState is updated from the main thread consistently.

Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt

kschults · 2024-08-22T20:18:00Z

Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt

@@ -25,7 +27,7 @@ internal interface StateStore {
    }
 }

-internal class ArmadilloStateStore(private val reducer: Reducer) :
+internal class ArmadilloStateStore(private val reducer: Reducer, private val appContext: Context) :


Do we have to worry about this leaking the context now that it's holding a reference to it?

Would it help if we held a reference to the looper instead? ArmadilloStateStore(..., appContext: Context) ... private val looper = appContext.mainLooper

This is the app context rather than the activity context, so it shouldn't be a danger for leaks the way Activity is

Resolved by passing in Handler

kschults · 2024-08-22T20:20:33Z

Armadillo/src/main/java/com/scribd/armadillo/di/AppModule.kt

@@ -33,7 +33,7 @@ internal class AppModule(private val context: Context) {
    @Singleton
    @PrivateModule
    @Provides
-    fun stateStore(reducer: Reducer): ArmadilloStateStore = ArmadilloStateStore(reducer)
+    fun stateStore(reducer: Reducer): ArmadilloStateStore = ArmadilloStateStore(reducer, context)


Actually, you could even pass the looper from here, since that's what you actually need the context for

That reads better yea

kschults · 2024-08-22T20:26:44Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+        val dataSourceFactory = this.drmHttpDataSourceFactory
+            ?: DefaultHttpDataSource.Factory().setUserAgent(this.userAgent)
+        val httpDrmCallback = HttpMediaDrmCallback(if (drmConfiguration.licenseUri == null) null else drmConfiguration.licenseUri.toString(), drmConfiguration.forceDefaultLicenseUri, (dataSourceFactory)!!)
+        val var4: UnmodifiableIterator<*> = drmConfiguration.licenseRequestHeaders.entries.iterator()


This seems like it got eaten a little bit by the class converter. Can you clean this up so it's a little more readable?

Original java code

for (Map.Entry<String, String> entry : drmConfiguration.licenseRequestHeaders.entrySet()) { httpDrmCallback.setKeyRequestProperty(entry.getKey(), entry.getValue()); }

thank you that helps to see the intended behavior

kschults · 2024-08-22T20:27:57Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+    }
+
+    /** New original provider for this class */
+    class ArmadilloDrmProvider(private val stateStore: StateStore.Modifier) : ExoMediaDrm.Provider {


Probably makes more sense for this to be a separate class in its own file than an inner class

I felt that the large amount of single use classes in separate files made everything very hard to follow. It was very hard to figure out how to build this feature.

I felt that the large amount of single use classes in separate files made everything very hard to follow. It was very hard to figure out how to build this feature.

This x1000. This is what I meant by my audio player being a more simplified interpretation of armadillo and could be a good reference of what exactly is being done

Yeah if i were to refactor this project, i would use some Facades, layers or something to help indicate hierarchy and order. Though I love, love the Reducer, as it helps so much to wrangle the spaghetti coming from Exoplayer.

kschults · 2024-08-22T20:29:54Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+        return drmSessionManager
+    }
+
+    /** New original provider for this class */


The only thing this does differently is emit events to the dispatcher, right? Can you make that explicit in the docstring

kschults · 2024-08-22T20:35:14Z

Armadillo/src/main/java/com/scribd/armadillo/playback/mediasource/DashMediaSourceGenerator.kt

-            }
-        }
+        val download = downloadTracker.getDownload(request.url.toUri())
+        val isDownloaded = download != null && download.state != Download.STATE_FAILED


I'm worried about isDownloaded here because it's also possible for it to be in progress, which would be misleading. But I don't have a better suggestion. It could be either in progress or finished. Maybe forDownload?

ah nice catch, this line was there before but i didn't think much of it. Im changing it to check for Completion, because in progress might not contain the audio that is resuming.

Armadillo/src/main/java/com/scribd/armadillo/playback/mediasource/DashMediaSourceGenerator.kt

SeanoAndroid · 2024-08-22T20:21:28Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+        val dataSourceFactory = this.drmHttpDataSourceFactory
+            ?: DefaultHttpDataSource.Factory().setUserAgent(this.userAgent)
+        val httpDrmCallback = HttpMediaDrmCallback(if (drmConfiguration.licenseUri == null) null else drmConfiguration.licenseUri.toString(), drmConfiguration.forceDefaultLicenseUri, (dataSourceFactory)!!)
+        val var4: UnmodifiableIterator<*> = drmConfiguration.licenseRequestHeaders.entries.iterator()


Maybe clean up the DefaultDrmSessionManagerProvider code a bit, or leave it to better convey what it's copied from? idk

rubeus90 · 2024-08-22T21:33:29Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+
+            //ExoMediaDrm.OnEventListener doesn't do anything at all, so its not used
+            if (Util.SDK_INT >= 23) {
+                instance.setOnKeyStatusChangeListener { exoMediaDrm, sessionId, keyStatuses, b ->


If we're gonna keep all the params names (are we using them later?), the last boolean param is actually hasNewUsableKey 😁

rubeus90 · 2024-08-22T21:42:09Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

+        val dataSourceFactory = this.drmHttpDataSourceFactory
+            ?: DefaultHttpDataSource.Factory().setUserAgent(this.userAgent)
+        val httpDrmCallback = HttpMediaDrmCallback(if (drmConfiguration.licenseUri == null) null else drmConfiguration.licenseUri.toString(), drmConfiguration.forceDefaultLicenseUri, (dataSourceFactory)!!)
+        val var4: UnmodifiableIterator<*> = drmConfiguration.licenseRequestHeaders.entries.iterator()


Original java code

for (Map.Entry<String, String> entry : drmConfiguration.licenseRequestHeaders.entrySet()) { httpDrmCallback.setKeyRequestProperty(entry.getKey(), entry.getValue()); }

- Adds DrmState to ArmadilloState, giving full visibility into DRM status to the client, including the expiration date for content. - Splits SocketTimeout from HttpResponseCodeException, into ConnectivityException to better differentiate connectivity difficulties from developer error. - Ensures that ArmadilloState is updated from the main thread consistently.

SeanoAndroid · 2024-08-23T03:07:07Z

Armadillo/src/main/java/com/scribd/armadillo/download/drm/ArmadilloDrmSessionManagerProvider.kt

@@ -73,11 +71,10 @@ internal class ArmadilloDrmSessionManagerProvider @Inject constructor(private va
    private fun createManager(drmConfiguration: DrmConfiguration): DrmSessionManager {
        val dataSourceFactory = this.drmHttpDataSourceFactory
            ?: DefaultHttpDataSource.Factory().setUserAgent(this.userAgent)
-        val httpDrmCallback = HttpMediaDrmCallback(if (drmConfiguration.licenseUri == null) null else drmConfiguration.licenseUri.toString(), drmConfiguration.forceDefaultLicenseUri, (dataSourceFactory)!!)
-        val var4: UnmodifiableIterator<*> = drmConfiguration.licenseRequestHeaders.entries.iterator()
+        val license = if (drmConfiguration.licenseUri == null) null else  drmConfiguration.licenseUri.toString()


Any reason not to just use drmConfiguration.licenseUri?.toString()?

kabliz added the High High complexity/size label Aug 21, 2024

SeanoAndroid self-assigned this Aug 22, 2024

rubeus90 self-assigned this Aug 22, 2024

kschults reviewed Aug 22, 2024

View reviewed changes

SeanoAndroid reviewed Aug 22, 2024

View reviewed changes

rubeus90 reviewed Aug 22, 2024

View reviewed changes

rubeus90 approved these changes Aug 22, 2024

View reviewed changes

SeanoAndroid approved these changes Aug 23, 2024

View reviewed changes

kschults approved these changes Aug 23, 2024

View reviewed changes

SeanoAndroid approved these changes Aug 26, 2024

View reviewed changes

rubeus90 approved these changes Aug 26, 2024

View reviewed changes

kabliz merged commit d8dbb91 into main Aug 26, 2024
6 checks passed

kabliz deleted the kabliz/APT-9823-drm-interface branch August 26, 2024 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface for DRM Visibility #39

Interface for DRM Visibility #39

kabliz commented Aug 21, 2024

kschults Aug 22, 2024

kschults Aug 22, 2024

kabliz Aug 22, 2024

kabliz Aug 23, 2024

kschults Aug 22, 2024

kabliz Aug 23, 2024

kschults Aug 22, 2024

rubeus90 Aug 22, 2024

kabliz Aug 23, 2024

kschults Aug 22, 2024

kabliz Aug 22, 2024

SeanoAndroid Aug 22, 2024

kabliz Aug 22, 2024 •

edited

Loading

kschults Aug 22, 2024

kschults Aug 22, 2024

kabliz Aug 23, 2024

SeanoAndroid Aug 22, 2024

rubeus90 Aug 22, 2024

rubeus90 Aug 22, 2024

SeanoAndroid Aug 23, 2024

Interface for DRM Visibility #39

Interface for DRM Visibility #39

Conversation

kabliz commented Aug 21, 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

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

kabliz Aug 22, 2024 • edited Loading

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

kabliz Aug 22, 2024 •

edited

Loading