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

Add DecoderConfig to Player #622

Open
wants to merge 23 commits into
base: development
Choose a base branch
from
Open

Conversation

testcenter
Copy link
Contributor

@testcenter testcenter commented Mar 21, 2025

Description

With the player-android 3.106 release we will introduce a new DecoderConfig to the PlaybackConfig.
With this PR we introduce the DecoderConfig API on react-native.

In addition, the 3.106 release will remove some Tweaks. The affected tweak will also be removed in this react-native project in favor of the DecoderConfig (Removed tweak: preferSoftwareDecodingForAds)

Changes

  • Android: Add DecoderConfigModule
  • RN: Add decoderConfig (+ usage in PlaybackConfig
  • RN: Remove TweaksConfig.preferSoftwareDecodingForAds

Checklist

  • [s] 🗒 CHANGELOG entry

Android:
- Remove unnecessary `resolveOnUiThread` call
- Remove unnecessary `DecoderConfigBridge`
- Add `decoderConfigModule` extension val to `ReactApplicationContext` and `RejectPromiseOnExceptionBlock`
- Add additional parameter to `PlayerModule` `init` functions (add `decoderNativeId`, similar to `networkNativeId`)
- Add extension function `ReadableArray.toMediaCodecInfo` to parse the decoder priority result from rn

RN
- Fix `DecoderContextMediaType` enum values
- `DecoderConfig` extends `NativeInstanceConfig` now
- Refactor and fix `decoder/index.ts`
- add `export` statement to export the decoderConfig
- Add missing `destroy` to the decoderConfig on player destroy
@testcenter testcenter self-assigned this Mar 21, 2025
@rolandkakonyi rolandkakonyi removed their request for review March 21, 2025 13:15
src/player.ts Outdated
Comment on lines 525 to 527
console.warn(
`[Player ${this.nativeId}] config: PlaybackConfig.DecoderConfig is not available for iOS. Only Android devices.`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be called on iOS as this results in a visible warning in the application!
I think keeping just the return should be enough.
Otherwise you need the same check on the caller side of initDecoderConfig

@testcenter testcenter force-pushed the PA-2888/decoder-config branch from e8d56eb to c3529bf Compare March 25, 2025 07:03
@testcenter testcenter force-pushed the PA-2888/decoder-config branch from c3529bf to c38b514 Compare March 25, 2025 07:54
@testcenter testcenter force-pushed the PA-2888/decoder-config branch from c38b514 to e2a2f68 Compare March 25, 2025 09:49
@testcenter testcenter marked this pull request as ready for review March 25, 2025 10:32
@testcenter testcenter requested review from a team as code owners March 25, 2025 10:32
@testcenter testcenter requested review from zigavehovec, strangesource and rolandkakonyi and removed request for a team March 25, 2025 10:32
Copy link
Contributor

@zigavehovec zigavehovec left a comment

Choose a reason for hiding this comment

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

Would be nice to have a minimal PR description.
I performed some basic tests and it's looking good on Android. I'm having some issues building on iOS, but still looking into it.

Please feel free to ping me if you need any help working through the comments I added 🙂

@@ -105,6 +105,6 @@ dependencies {
// Bitmovin
implementation 'com.google.ads.interactivemedia.v3:interactivemedia:3.35.1'
implementation 'com.google.android.gms:play-services-ads-identifier:18.0.1'
implementation 'com.bitmovin.player:player:3.104.2+jason'
implementation 'com.bitmovin.player:player-media-session:3.104.2+jason'
implementation 'com.bitmovin.player:player:3.106.0-a.1-decoderconfig+jason'
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder comment to update this when the final release is out 🙂

@stonko1994 stonko1994 removed the request for review from a team March 26, 2025 14:31
@testcenter testcenter requested a review from zigavehovec March 27, 2025 07:52
Copy link
Contributor

@strangesource strangesource left a comment

Choose a reason for hiding this comment

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

Nothing to add on top of @zigavehovec's thorough review.

Copy link
Contributor

@zigavehovec zigavehovec left a comment

Choose a reason for hiding this comment

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

Please make sure to take a look at #622 (comment)

After that we are ready to go IMO 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants