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

FCE-423, FCE-298: Propose updated way of configuring video quality in SDKs; Re-think default activeEncodings settings in SDK; FCE-1032: Fix stopCapture #281

Merged
merged 11 commits into from
Jan 15, 2025

Conversation

MiloszFilimowski
Copy link
Collaborator

@MiloszFilimowski MiloszFilimowski commented Dec 20, 2024

Description

  • Removed configuration from RN Client
  • Fixed stopCapture crash

Motivation and Context

  • We agreed that for now simulcast configuration will remain hidden from the user.

How has this been tested?

Tested on iOS and Android

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@MiloszFilimowski MiloszFilimowski changed the base branch from main to mfilimowski/add-simulcast December 20, 2024 14:45
Base automatically changed from mfilimowski/add-simulcast to main January 10, 2025 14:53
@MiloszFilimowski MiloszFilimowski force-pushed the mfilimowski/FCE-423-update-simulcast-config branch from cfdb717 to 0fc8b05 Compare January 13, 2025 12:17
@MiloszFilimowski MiloszFilimowski force-pushed the mfilimowski/FCE-423-update-simulcast-config branch from be186b7 to 662f2a0 Compare January 14, 2025 10:21
@MiloszFilimowski MiloszFilimowski force-pushed the mfilimowski/FCE-423-update-simulcast-config branch from 662f2a0 to ee10165 Compare January 14, 2025 11:06
@MiloszFilimowski MiloszFilimowski marked this pull request as ready for review January 14, 2025 17:25
@MiloszFilimowski MiloszFilimowski changed the title FCE-423, FCE-298: Propose updated way of configuring video quality in SDKs; Re-think default activeEncodings settings in SDK FCE-423, FCE-298: Propose updated way of configuring video quality in SDKs; Re-think default activeEncodings settings in SDK; FCE-1032: Fix stopCapture Jan 14, 2025
Copy link

linear bot commented Jan 14, 2025

FCE-1032 Fix stopCapture

Copy link
Member

@mironiasty mironiasty left a comment

Choose a reason for hiding this comment

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

Looks good to go 👍

Comment on lines +66 to +67
// iOS has a limit of 3 hardware encoders
// 3 simulcast layers + 1 screen share layer = 4, which is too much
Copy link
Member

Choose a reason for hiding this comment

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

I was just thinking - do we still know that this limit applies on latest iOS version? If not, maybe we should investigate it again 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +10 to +13
const defaultSimulcastConfig = () =>
({
enabled: false,
}) satisfies SimulcastConfig;
Copy link
Member

Choose a reason for hiding this comment

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

There is philosophical question: should satisfies be used, or just type variable as () => SimulcastConfig 🤔

(in the end, it does not really matter here)

Copy link
Collaborator Author

@MiloszFilimowski MiloszFilimowski Jan 15, 2025

Choose a reason for hiding this comment

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

In this case, it doesn't really matter. The advantage of using satisfies instead of a type is that you preserve the actual value.
So if you type is

type Something = { something: string}

and you create an object

const test = { something: 'test'} satisfies Something

You still retail the autocompletion when using test.something as "test", whereas with type annotation it's just string.

So in our case the user would now that the default value is always false.

@MiloszFilimowski MiloszFilimowski merged commit cdcc580 into main Jan 15, 2025
4 checks passed
@MiloszFilimowski MiloszFilimowski deleted the mfilimowski/FCE-423-update-simulcast-config branch January 15, 2025 10:10
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.

2 participants