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.

Sign up for GitHub

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

Jump to bottom

obs-ffmpeg: Fix SRT listener bug #10508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

obs-ffmpeg: Fix SRT listener bug #10508

wants to merge 1 commit into from

Conversation

pkviet
Copy link
Member

@pkviet pkviet commented Apr 10, 2024

Description

Fixes #10504.
When starting an SRT stream in listener mode, if no connection is made by a client, the socket was not closed when exiting obs. This fixes the issue so that SRT is closed properly.

Motivation and Context

Bugs are bad.

How Has This Been Tested?

Bug is fixed.
SRT in caller mode still works.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@pkviet pkviet mentioned this pull request Apr 10, 2024
Open
@pkviet pkviet added Bug Fix Non-breaking change which fixes an issue Seeking Testers Build artifacts on CI labels Apr 10, 2024
@pkviet pkviet added this to the OBS Studio (Next Version) milestone Apr 10, 2024
@RytoEX RytoEX requested a review from tt2468 April 10, 2024 18:29
RytoEX
RytoEX requested changes Apr 10, 2024
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Commit message nit:

  • obs -> OBS

RytoEX
RytoEX requested changes Apr 10, 2024
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Formatting nit.

plugins/obs-ffmpeg/obs-ffmpeg-mpegts.c Show resolved Hide resolved
@RytoEX
Copy link
Member

RytoEX commented Apr 11, 2024 •
edited

I have no more nits at this time.

The commit message body still says obs in lowercase at the end of the second sentence. Please capitalize it.

Will need someone to test this and make sure the code changes make sense. Nothing jumps out to me at a glance, but a second set of eyes wouldn't hurt.

@tt2468
Copy link
Member

tt2468 commented Apr 12, 2024

Unfortunately, this does not appear fully fixed. Here's what I'm seeing on Ubuntu 22.04:

  • Start stream with srt://0.0.0.0:6969?mode=listener
  • Don't connect a client
    • Stream will never fully "start"
    • OBS begins spamming the log with error: [obs-ffmpeg mpegts muxer / libsrt]: Connection rejected, Unknown or erroneous messages
  • Close OBS
  • OBS will unload fully, however the program will segfault on finalization:
info: Number of memory leaks: 0
Segmentation fault (core dumped)

It completely fails to shut down the socket if I connect a client, then disconnect the client, then try stopping stream or shutting down OBS. To be completely honest, I think we need to just remove rendezvous and listener modes from our SRT output.

@pkviet
Copy link
Member Author

pkviet commented Apr 12, 2024

@tt2468 i tested on win 11 and didn't get that segmentation fault. Unfortunately i don't have a ubuntu 22.04 distro to test with. I'll investigate more on my side with win 11 to see if there are errors i overlooked.

@pkviet
obs-ffmpeg: Fix SRT listener bug
645952b 
Fixes obsproject#10504.
There was a bug in FFmpeg implementation which was hidden by a bug in
libsrt; it was fixed in a recent commit [1].
When we ported FFmpeg libsrt.c to obs, we brought the said bug along.
When starting an SRT stream in listener mode, if no connection is made
by a client, there were two issues:
- 1) obs was stuck into a connecting loop,
- 2) the socket was not closed when exiting OBS.
This fixes the issue so that SRT is displaying that a stream started
when in listener mode even if NO client is connected.
This is the correct behaviour for a listener.
The stream now closes properly.

[1] https://git.videolan.org/?p=ffmpeg.git;a=commit;h=87677c2195e86b126c3438439a05d0a46ae5bb50
Signed-off-by: pkv <[email protected]>
@pkviet
Copy link
Member Author

pkviet commented May 4, 2024 •
edited

@tt2468 the FFmpeg bug for the listener was exactly our issue.
I checked that listener now works properly:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RytoEX RytoEX RytoEX requested changes

@tt2468 tt2468 Awaiting requested review from tt2468

Assignees
No one assigned
Labels
Bug Fix Non-breaking change which fixes an issue Seeking Testers Build artifacts on CI
Projects
None yet
Milestone
OBS Studio (Next Version)
3 participants
@pkviet @RytoEX @tt2468