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

Expose TweaksConfig.forceReuseVideoCodecReasons from Android #561

Conversation

saravanans-github
Copy link
Contributor

@saravanans-github saravanans-github commented Nov 14, 2024

Description

Expose Android SDKs TweaksConfig.forceReuseVideoCodecReasons

Changes

Checklist

  • 🗒 CHANGELOG entry

@saravanans-github saravanans-github requested a review from a team as a code owner November 14, 2024 07:44
Copy link
Contributor

@krocard krocard left a comment

Choose a reason for hiding this comment

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

I didn't test the PR, just reviewed the code change.

Comment on lines 204 to 212
val mutableSet = mutableSetOf<ForceReuseVideoCodecReason>()
for (item in it) {
val reason = item?.forceReuseVideoCodecReason()
if (reason != null) {
mutableSet.add(reason)
}
}
forceReuseVideoCodecReasons = mutableSet
}
Copy link
Contributor

@krocard krocard Nov 14, 2024

Choose a reason for hiding this comment

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

Suggested change
val mutableSet = mutableSetOf<ForceReuseVideoCodecReason>()
for (item in it) {
val reason = item?.forceReuseVideoCodecReason()
if (reason != null) {
mutableSet.add(reason)
}
}
forceReuseVideoCodecReasons = mutableSet
}
forceReuseVideoCodecReasons = it.mapNotNull(String::toForceReuseVideoCodecReason).toSet()

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably also a toSet() needs to be added.

Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* When switching between video formats (eg: adapting between video qualities) the codec might be recreated due to several reasons.
* This behaviour can cause brief black screens when switching between video qualities as codec recreation can be slow.
*
Copy link
Contributor

@krocard krocard Nov 14, 2024

Choose a reason for hiding this comment

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

Documentation was incomplete.

Suggested change
*
*
* If a device is know to support video format changes and keep the current decoder without issues,
* this set can be filled with multiple `ForceReuseVideoCodecReason` and avoid the black screen.
*

Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* Converts any JS string into an `AdSourceType` enum value.
*/
private fun String.forceReuseVideoCodecReason(): ForceReuseVideoCodecReason? = when (this) {
Copy link
Contributor

@krocard krocard Nov 14, 2024

Choose a reason for hiding this comment

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

Conversion function are prefixed by to:

Suggested change
private fun String.forceReuseVideoCodecReason(): ForceReuseVideoCodecReason? = when (this) {
private fun String.toForceReuseVideoCodecReason(): ForceReuseVideoCodecReason? = when (this) {

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -167,6 +168,16 @@ fun ReadableMap.toStyleConfig(): StyleConfig = StyleConfig().apply {
withString("scalingMode") { scalingMode = ScalingMode.valueOf(it) }
}

/**
* Converts any JS string into an `AdSourceType` enum value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Converts any JS string into an `AdSourceType` enum value.
* Converts any JS string into an `ForceReuseVideoCodecReason` enum value.

Copy link
Contributor

Choose a reason for hiding this comment

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

CHANGELOG.md Outdated Show resolved Hide resolved
src/tweaksConfig.ts Outdated Show resolved Hide resolved
src/tweaksConfig.ts Outdated Show resolved Hide resolved
Comment on lines 204 to 212
val mutableSet = mutableSetOf<ForceReuseVideoCodecReason>()
for (item in it) {
val reason = item?.forceReuseVideoCodecReason()
if (reason != null) {
mutableSet.add(reason)
}
}
forceReuseVideoCodecReasons = mutableSet
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably also a toSet() needs to be added.

@matamegger matamegger changed the title Prn 147/expose tweaks config force reuse video codec reasons Expose TweaksConfig.forceReuseVideoCodecReasons from Android Nov 14, 2024
@strangesource strangesource self-assigned this Nov 14, 2024
@strangesource
Copy link
Contributor

Thanks for the reviews, I'll pick this up tomorrow.

Copy link
Contributor

@matamegger matamegger left a comment

Choose a reason for hiding this comment

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

@strangesource Please revisit the Changelog entry

Co-authored-by: Matthias Tamegger <[email protected]>
@strangesource strangesource merged commit bfde320 into development Nov 15, 2024
6 checks passed
@strangesource strangesource deleted the PRN-147/expose-tweaksConfig-forceReuseVideoCodecReasons branch November 15, 2024 14:14
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