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

Support standard streamid syntax with unit tests #43

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

gpmidi
Copy link

@gpmidi gpmidi commented Nov 26, 2024

An updated version of #39 with unit tests. Also added support for passing on the username to the upstream AAA. Plus a few random non-bug fixes I ran across while I was at it.

@gpmidi
Copy link
Author

gpmidi commented Nov 26, 2024

@iSchluff I'm working on unit tests for this and noticed that the TestServerImpl_GetStatistics test fails. The tl;dr is that it's old vs new format. I can either update the test to use the SRT defined spec or figure out a way to base the answer on what was sent to the server in the first place. Thoughts?

For now I've just reverted the output to the pre-change version just so the behavior doesn't change on dependent apps.

=== RUN   TestServerImpl_GetStatistics
    server_test.go:52: Invalid stream statistics: got {s1 srt://testserver.de:1337?streamid=#!::m=request,r=s1 2 2024-11-25 14:48:26.374969306 -0500 EST m=+0.000745398}, expected {s1 srt://testserver.de:1337?streamid=play/s1 2 2024-11-25 14:48:26.374969306 -0500 EST m=+0.000745398}
--- FAIL: TestServerImpl_GetStatistics (0.00s)

@iSchluff
Copy link
Member

I'm not super attached to the url returned in GetStatistics. The main goal is having something that ffmpeg understands. So if the new format still works with ffplay I am fine with changing the result.

@gpmidi
Copy link
Author

gpmidi commented Nov 26, 2024

@iSchluff Thanks! I'll test it locally with ffmpeg to double check and report back.

@gpmidi
Copy link
Author

gpmidi commented Nov 28, 2024

ffplay works fine with the new url format

@gpmidi gpmidi marked this pull request as ready for review November 28, 2024 15:04
Using new format for get stats
@gpmidi
Copy link
Author

gpmidi commented Nov 28, 2024

@iSchluff Ready for review. Can close out #39 when this is merged :)

@gpmidi
Copy link
Author

gpmidi commented Dec 11, 2024

@iSchluff Any chance on a PR review?

@gpmidi
Copy link
Author

gpmidi commented Dec 17, 2024

@iSchluff Any word on a review? Plz? ;)

@danimo
Copy link
Contributor

danimo commented Dec 24, 2024

Quick heads-up: I have rebased your branch on top of our ongoing work to update the ecosystem. Unfortunately, the changes you are proposing are above my skill level wrt the SRT format. @iSchluff seems busy at the moment, please be patient.

@gpmidi
Copy link
Author

gpmidi commented Dec 24, 2024

@danimo Ah cool, thanks for the update :)

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.

3 participants