-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added support for setting video settings, moved to pnpm, fixed 2nd stream reconnection #39
base: master
Are you sure you want to change the base?
Added support for setting video settings, moved to pnpm, fixed 2nd stream reconnection #39
Conversation
created react-native-publisher, till we are waiting for approval. You can also use patch-package
|
Hello @Andriiklymiuk thanks for the PR and contribution. |
import { NativeModules, ViewStyle } from 'react-native'; | ||
import PublisherComponent, { | ||
type DisconnectType, | ||
type ConnectionFailedType, | ||
type ConnectionStartedType, | ||
type ConnectionSuccessType, | ||
type NewBitrateReceivedType, | ||
type StreamStateChangedType, | ||
type BluetoothDeviceStatusChangedType, | ||
DisconnectType, | ||
ConnectionFailedType, | ||
ConnectionStartedType, | ||
ConnectionSuccessType, | ||
NewBitrateReceivedType, | ||
StreamStateChangedType, | ||
BluetoothDeviceStatusChangedType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the type
aliases?
@@ -24,4 +27,4 @@ | |||
"strict": true, | |||
"target": "esnext" | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you've delete the newline on the bottom
@@ -96,6 +101,9 @@ const RTMPPublisher = forwardRef<RTMPPublisherRefProps, RTMPPublisherProps>( | |||
const setAudioInput = (audioInput: AudioInputType) => | |||
RTMPModule.setAudioInput(audioInput); | |||
|
|||
const setVideoSettings = async (videoSettings: VideoSettingsType) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async
keyword is unnecessary on here.
@@ -27,9 +31,11 @@ | |||
"lint": "eslint \"**/*.{js,ts,tsx}\"", | |||
"prepare": "bob build", | |||
"release": "dotenv release-it --", | |||
"example": "yarn --cwd example", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you delete example
script?
"build": "pnpm install && pnpm pack", | ||
"unpack": "for file in react-native-rtmp-publisher*.tgz; do tar -xzf \"$file\"; done", | ||
"deletePackage": "rm -r package && rm -r react-native-rtmp-publisher*.tgz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think sticking on yarn is much more good idea for right now. We can consider pnpm on the future. Thanks for the suggestion 👍🏼
Can you revert this for now?
@@ -209,6 +216,7 @@ For live stream, Youtube gives you stream url and stream key, you can place the | |||
| `isVideoPrepared` | `Promise<boolean>` | Returns video prepare state | ✅ | ✅ | | |||
| `isCameraOnPreview` | `Promise<boolean>` | Returns camera is on | ✅ | ❌ | | |||
| `setAudioInput` | `Promise<AudioInputType>`| Sets microphone input | ✅ | ✅ | | |||
|`setVideoSettings` | `Promise<VideoSettingsType>`| Sets camera quality settings| ❌ | ✅ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not setting the camera quality, it's setting the video scale values. Can you update it for avoiding misunderstanding?
In the MR I added:
p.s. will publish separate library till these fixes are implemented (don't know if this library still supported)