Expose RtmpClientSettings on RtmpEndpoint and RtmpEndpointFactory#286
Expose RtmpClientSettings on RtmpEndpoint and RtmpEndpointFactory#286alexmck wants to merge 4 commits intoThibaultBee:dev_v3_2from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes komuxer’s RtmpClientSettings through StreamPack’s RTMP endpoint API so callers can customize the RTMP connect handshake (e.g., flashVer) instead of always using komuxer defaults.
Changes:
- Add an optional
RtmpClientSettingsparameter toRtmpEndpoint. - Thread
clientSettingsintoRtmpConnectionBuilder.connect(...)duringopen(). - Add an optional
clientSettingsparameter toRtmpEndpointFactoryand forward it to created endpoints.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| rtmpClient = connectionBuilder.connect(descriptor.uri.toString()).apply { | ||
| rtmpClient = connectionBuilder.connect(descriptor.uri.toString(), settings = clientSettings).apply { |
There was a problem hiding this comment.
connect(..., settings = clientSettings) uses a named argument for an API coming from an external dependency (io.github.komedia.komuxer.rtmp.connect). In Kotlin, named arguments are source-level and can break consumers if the dependency renames the parameter (even without changing behavior). Prefer using positional arguments (or an overload) here to avoid coupling to the parameter name.
| rtmpClient = connectionBuilder.connect(descriptor.uri.toString(), settings = clientSettings).apply { | |
| rtmpClient = connectionBuilder.connect(descriptor.uri.toString(), clientSettings).apply { |
|
Hi, I do understand the reason for this but this is a bad usage of |
|
Hey ThibaultBee. This was a genuine pull request. I have a similar change that was merged into RootEncoder. While I realise I got a bit carried away with the scope and this turned into more than just setting a custom I was genuinely quite excited by this approach which gives the developer direct access to connect command properties and RTMP client settings in the If you're really not interested, I can absolutely respect that decision and I can either keep using my own fork, or go back to my original implementation. Alternatively if you would prefer I take a different approach, I'm more than happy to spend time revising this pull request. |
|
Could you explain why you need to identify devices? IMHO, as you are not going to give the same RTMP URL (path and stream key) to multiple devices, the RTMP URL allows you to identify the user. Oh, nice you are using your own factory and endpoint. I have got questions now (but they are out of this scope). Your PR is missing the implementation in the |
|
That's correct. Each user has their own unique stream key, and authentication happens server side. Setting the From a support point of view, it's often very helpful to be able to troubleshoot and tell a customer that they are using an old version of the app, or triage an issue with a newer version of the app. The number of users that have auto updates turned off is also staggering! But user authentication via If I can provide anymore assistance or guidance please let me know. |
|
You are trying to use a streaming protocol to pass debug info. It feels lazy. |
|
It's no different to using the User Agent String from a browser to identify the browser and version number. flashVer is in some ways synonymous to this, and this was to my knowledge it's intended use case. Sending it with the RTMP connection is one way to ensure that the correct client meta data is sent when the stream is negotiating it's connection, rather than relying on extra HTTP overhead and trying to line the two up later. This becomes particularly important for slow and high latency connections, such as poor 4G connectivity. |
|
Yeah but HTTP is not a streaming protocol. About the implementation, have you consider using |
|
Could you move your PR to |
|
I had not considered using Would you like me to rework this to use the |
What do you mean? |
|
Never mind. I was under the impression that settings like |
|
I am surprised. Do you plan to use But yeah, I would rather use an implementation with the |
|
Me personally, no, but there may be others that would like access to that setting. Would you like me to put a new implementation together? |
|
I am wondering if you should:
|
The system can clear the view model for performance reason and the application won't recover from this.
36f0e22 to
2faa47a
Compare
|
Hey ThibaultBee. I've updated this PR and used the |
ThibaultBee
left a comment
There was a problem hiding this comment.
Ok, that's better. You get the main idea.
Could you avoid creating default RtmpClientSettings everywhere? It seems the be undebuggable.
I would rather use the internal rtmp default client settings instead.
Could you only pass the ConnectObjectBuilder instead of the RtmpClientSettings.
The reason is that there are sensible parameters in RtmpClientSettings (ie. parameters that can break the RTMP conection). If needed later, I would make the changes myself.
|
Hey ThibaultBee. Sorry for the delay in reviewing your comments and making the requested changes. I believe these changes help narrow the scope as requested. I've added a |
Summary
This change adds an optional
RtmpClientSettingsparameter toRtmpEndpointandRtmpEndpointFactory, allowing developers to configure the RTMP connect command sent during the handshake with the server.The underlying komuxer RTMP library already supports full configuration of the connect command via
RtmpClientSettingsandConnectObjectBuilder, but StreamPack'sRtmpEndpointwas hard coded to useRtmpClientSettings()defaults. This change threads that configuration through to the public API.Motivation
A common requirement for RTMP streaming applications is to identify themselves to the server during the connect handshake — typically by setting a custom
flashVerstring. This is useful for:Without this change, all StreamPack clients identify as the komuxer default:
FMLE/3.0 (compatible; FMSc/1.0).What changed
A single file was modified —
RtmpEndpoint.kt:RtmpEndpointconstructor — addedclientSettings: RtmpClientSettings = RtmpClientSettings()parameterRtmpEndpoint.open()— passesclientSettingstoconnectionBuilder.connect()instead of using the implicit defaultRtmpEndpointFactoryconstructor — added matchingclientSettingsparameter, forwarded to every endpoint it createsAll parameters have defaults, so this is fully backwards-compatible. Existing code that constructs
RtmpEndpointFactory()with no arguments behaves identically to before.Usage
The
connectInfolambda receives aConnectObjectBuilderpre-populated with defaults. Only the fields you set are overridden — everything else retains its default value.Exposed settings
As a bonus, this change allows customisation of all RTMP options.
RtmpClientSettings(connection-level)writeWindowAcknowledgementSizeIntInt.MAX_VALUEamfVersionAmfVersionAMF0AMF0orAMF3)clockRtmpClockRtmpClock.Default()tooLateFrameDropTimeoutInMsLong?null(never drop)connectInfoConnectObjectBuilder.() -> Unit{}ConnectObjectBuilder(connect command fields, viaconnectInfo)flashVerString"FMLE/3.0 (compatible; FMSc/1.0)"swfUrlString?nullpageUrlString?nullfpadBooleanfalsecapabilitiesInt239audioCodecsList<AudioMediaType>?[AAC, G711_ALAW, G711_MLAW]videoCodecsList<VideoMediaType>?[SORENSON_H263, AVC]videoFunctionList<VideoFunction>[]objectEncodingObjectEncodingAMF0capsExList<CapsEx>?nullaudioTrackIdInfoMapMap<AudioMediaType, List<FourCCInfo>>?nullvideoFourCcInfoMapMap<VideoMediaType, List<FourCCInfo>>?nullVideoFunction(Enhanced RTMP v1)CLIENT_SEEKCLIENT_HDRCLIENT_PACKET_TYPE_METADATACLIENT_LARGE_SCALE_TILECapsEx(Enhanced RTMP v2)RECONNECTMULTITRACKMODEXTIMESTAMP_NANO_OFFSETFourCCInfo(per-codec capability flags, Enhanced RTMP v2)CAN_DECODECAN_ENCODECAN_FORWARD