Skip to content

add support for custom AMF object in streaming clients#1912

Merged
pedroSG94 merged 3 commits intopedroSG94:masterfrom
xiuminga:main
Sep 4, 2025
Merged

add support for custom AMF object in streaming clients#1912
pedroSG94 merged 3 commits intopedroSG94:masterfrom
xiuminga:main

Conversation

@xiuminga
Copy link
Copy Markdown
Contributor

@xiuminga xiuminga commented Sep 1, 2025

Summary
This PR adds an optional custom AMF object that is injected during the RTMP connect handshake. It’s designed for large CDN deployments to run targeted stream connectivity checks and diagnostics against specific IPs/nodes by tagging probe metadata in the handshake. Non-RTMP clients safely ignore this parameter.

Changes
• Add customAmfObject to StreamBase.startStream(...) and propagate to clients.
• RTMP: RtmpClient/CommandsManager merge the custom AMF into the connect object (supports optional tcUrl override).
• AmfObject supports dynamic properties.
• MultiStream updated; RTSP/SRT/UDP are no-ops.

Compatibility
• Backward-compatible: when customAmfObject is null, behavior is unchanged; non-RTMP paths ignore it.

@pedroSG94
Copy link
Copy Markdown
Owner

pedroSG94 commented Sep 2, 2025

Hello,

Thank you for the PR, but It will need a refactor because the way you did to insert new methods to RtmpClient is not correct.

  • Only rtmp module and RtmpStreamClient class should be affected by this PR. (create a method in RtmpStreamClient is enougth to get access to the new methods of RtmpClient class).
  • In rtmp module change the tcUrl variable is redundant because you already do it using the custom amfObject
  • Remove debug logs

Let me know if you have any question or even if you prefer let me do this modifications.

@xiuminga
Copy link
Copy Markdown
Contributor Author

xiuminga commented Sep 3, 2025

Thank you for the review. I’ve updated the PR according to your suggestions, though there may still be issues. If you consider the feature valuable and are willing to make further changes, I’d really appreciate it. Please feel free to modify or close the PR at your discretion.Maybe I should start with an issue🤔

@pedroSG94
Copy link
Copy Markdown
Owner

Hello,

Now, the code is close to be fine but I feel that the last commit is not necessary:
da5e4be

@pedroSG94
Copy link
Copy Markdown
Owner

pedroSG94 commented Sep 3, 2025

My code suggestion is this:
https://github.com/pedroSG94/RootEncoder/pull/1917/files

If you are fine with this, let me know it and I can do the merge

@xiuminga
Copy link
Copy Markdown
Contributor Author

xiuminga commented Sep 4, 2025

LGTM. Thanks for the patient guidance and fixes—feel free to merge.

@pedroSG94 pedroSG94 merged commit da5e4be into pedroSG94:master Sep 4, 2025
@pedroSG94
Copy link
Copy Markdown
Owner

Merged, thank you for the PR

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