Chore(android): refactor drm props #3846

freeboub · 2024-05-28T09:27:59Z

Summary

Refactor DRM props into a dedicated class

Motivation

Keep code clean and modularized

Changes

Just move all DRM subProps to a dedicated class

Test plan

Can be tested with sample

…ided from app

…oplayerView code size

…ideo # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java # src/Video.tsx

…ideo

…fix/avoidVideoResizingFlickering # Conflicts: # .github/ISSUE_TEMPLATE/bug-report.yml

…ideo

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

…iew.java

…tive-video into chore/refactorDrmProps

YangJonghun · 2024-05-28T11:55:58Z

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

-            }
+        DRMProps drmProps = DRMProps.parse(drm);
+        if (drmProps != null) {
+            videoView.setDrm(drmProps);


nit; If the drm prop is inserted as null, the old drm prop will still be there. wouldn't it be a problem? (I think it's a problem from the old behavior, not from this change)

If purpose of this PR is to preserve the old behavior, it's fine.

No It was not the goal of this PR, So I remove the test.
It doesn't make sense to keep previous DRM config when user remove it !
Thank you for the catch !

KrzysztofMoch

LGTM

I agree with @YangJonghun that we should considerate if we need old behaviour

…ideo into chore/refactorDrmProps # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

YangJonghun · 2024-05-30T12:36:16Z

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

-        }
+        DRMProps drmProps = DRMProps.parse(drm);
+        videoView.setDrm(drmProps);
+        videoView.setUseTextureView(false);


if drm props is null, we have to follow injected prop (useSecureView or useTextureView)

Yes you are right, thank you. I am also wondering if it can make sense to use Surface View by default? What is your opinion?

It's hard question. because it depends on what user using this library for
I prefer SurfaceView from a user perspective. In most situations, this is beneficial.
However, it can cause to make issue threads being created by people who don't read the documentation or don't know much about Android.

Decision is yours🫡

I am working to remove this videoView.setUseTextureView(false);
It is a bit. The ugly solution is to add new variables recording the last calls to setUseTextureView and setUseSecureView and apply it in the else.

I would prefer implement it in another way:

move DRM & viewType inside the source prop.

It will decrease the bridge usage, avoid restarting playback if we receive DRM prop after source and make cleaner code.

I will try to implement it, but I think we can merge this part now, and the next fix will come soon !

I propose to merge this PR as the setUseTextureView is already present in the code !

@YangJonghun here is The patch I would to do for ensure we correctly reset the view state in the app:
#3867
initially I didn't plan to it right now, but you make me accelerate :)
Still in draft as I need to do the same thing on ios, but that's the idea.
Let me know you thought please !
And ping @KrzysztofMoch

I apologize for late response.
I left related comment in #3867 .

…ideo into chore/refactorDrmProps # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

YangJonghun · 2024-06-10T15:45:28Z

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

+        this.drmProps = drmProps;
+        if (drmProps != null && drmProps.getDrmType() != null) {
+            this.drmProps.setDrmUUID(Util.getDrmUuid(drmProps.getDrmType()));
+        }


if (drmProps != null && drmProps.getDrmType() != null) { this.drmProps.setDrmUUID(Util.getDrmUuid(drmProps.getDrmType())); }

I think above code could be moved inside the DrmProps.parse fn.

I agree, but my problem with that is that I will have to import Utils from media3 in drmProp.kt. I really want to avoid including player specific code in the utils / data management classes ...

I understand. but for that reason, it seems like parse function should be categorized in a different class. (for example, DRMUtils)
And I think removing dependencies from Utils seems hard to avoid because there's so much to prepare to implement as the SDK requires.🥲

If you worried deep dependencies, I recommend copying media3's implementation.

However, I'm not suggesting this because I think it's a serious issue, so you don't have to fix it.

freeboub and others added 15 commits May 5, 2024 21:06

perf: ensure we do not provide callback to native if no callback prov…

d3fc4fd

…ided from app

chore: rework bufferConfig to make it more generic and reduce ReactEx…

e1da32d

…oplayerView code size

Merge branch 'master' of github.com:react-native-video/react-native-v…

df3da43

…ideo # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java # src/Video.tsx

chore: improve issue template

0f33ad1

Merge branch 'TheWidlarzGroup:master' into master

17f2385

fix(android): avoid video view flickering at playback startup

1066898

Merge branch 'master' of github.com:react-native-video/react-native-v…

485f867

…ideo

Merge branch 'master' of github.com:freeboub/react-native-video into …

a5e10a3

…fix/avoidVideoResizingFlickering # Conflicts: # .github/ISSUE_TEMPLATE/bug-report.yml

Merge branch 'master' of github.com:react-native-video/react-native-v…

9ce1d95

…ideo

Merge branch 'master' of github.com:react-native-video/react-native-v…

fa27db7

…ideo

Merge branch 'master' of github.com:react-native-video/react-native-v…

be681d3

…ideo

Merge branch 'master' of github.com:react-native-video/react-native-v…

2fadb26

…ideo

Merge branch 'master' of github.com:react-native-video/react-native-v…

92df10d

…ideo

Merge branch 'master' of github.com:react-native-video/react-native-v…

aa61388

…ideo

chore(android): refactor DRM props into a dedicated class

41e2577

freeboub requested review from YangJonghun and KrzysztofMoch May 28, 2024 09:28

freeboub commented May 28, 2024

View reviewed changes

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java Outdated Show resolved Hide resolved

freeboub and others added 3 commits May 28, 2024 11:28

Update android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerV…

755b834

…iew.java

chore: fix linter

dc12ff8

Merge branch 'chore/refactorDrmProps' of github.com:freeboub/react-na…

cdb77e5

…tive-video into chore/refactorDrmProps

YangJonghun reviewed May 28, 2024

View reviewed changes

KrzysztofMoch approved these changes May 28, 2024

View reviewed changes

freeboub added 2 commits May 30, 2024 10:40

Merge branch 'master' of github.com:react-native-video/react-native-v…

25eaedb

…ideo into chore/refactorDrmProps # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

fix: ensure drm prop is correctly cleaned

1e155fc

freeboub commented May 30, 2024

View reviewed changes

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java Outdated Show resolved Hide resolved

YangJonghun reviewed May 30, 2024

View reviewed changes

Merge branch 'master' of github.com:react-native-video/react-native-v…

8b0e2a6

…ideo into chore/refactorDrmProps # Conflicts: # android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

freeboub force-pushed the chore/refactorDrmProps branch from bb17e1b to 8b0e2a6 Compare June 7, 2024 13:08

chore: revert unnecessary change

3f37e6d

YangJonghun reviewed Jun 10, 2024

View reviewed changes

YangJonghun approved these changes Jun 10, 2024

View reviewed changes

freeboub merged commit cfb5b1c into TheWidlarzGroup:master Jun 10, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore(android): refactor drm props #3846

Chore(android): refactor drm props #3846

freeboub commented May 28, 2024

YangJonghun May 28, 2024

YangJonghun May 28, 2024

freeboub May 30, 2024

KrzysztofMoch left a comment

YangJonghun May 30, 2024

freeboub May 30, 2024

YangJonghun May 30, 2024 •

edited

Loading

freeboub May 31, 2024 •

edited

Loading

freeboub May 31, 2024 •

edited

Loading

YangJonghun Jun 1, 2024 •

edited

Loading

YangJonghun Jun 10, 2024

freeboub Jun 10, 2024

YangJonghun Jun 10, 2024 •

edited

Loading

Chore(android): refactor drm props #3846

Chore(android): refactor drm props #3846

Conversation

freeboub commented May 28, 2024

Summary

Motivation

Changes

Test plan

YangJonghun May 28, 2024

Choose a reason for hiding this comment

YangJonghun May 28, 2024

Choose a reason for hiding this comment

freeboub May 30, 2024

Choose a reason for hiding this comment

KrzysztofMoch left a comment

Choose a reason for hiding this comment

YangJonghun May 30, 2024

Choose a reason for hiding this comment

freeboub May 30, 2024

Choose a reason for hiding this comment

YangJonghun May 30, 2024 • edited Loading

Choose a reason for hiding this comment

freeboub May 31, 2024 • edited Loading

Choose a reason for hiding this comment

freeboub May 31, 2024 • edited Loading

Choose a reason for hiding this comment

YangJonghun Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

YangJonghun Jun 10, 2024

Choose a reason for hiding this comment

freeboub Jun 10, 2024

Choose a reason for hiding this comment

YangJonghun Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

YangJonghun May 30, 2024 •

edited

Loading

freeboub May 31, 2024 •

edited

Loading

freeboub May 31, 2024 •

edited

Loading

YangJonghun Jun 1, 2024 •

edited

Loading

YangJonghun Jun 10, 2024 •

edited

Loading