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

OME allows multiple incoming RTMP streams for the same stream key #1749

Open
hernanrz opened this issue Dec 13, 2024 · 11 comments
Open

OME allows multiple incoming RTMP streams for the same stream key #1749

hernanrz opened this issue Dec 13, 2024 · 11 comments
Assignees
Labels
bug Confirmed as bug patched Patch applied

Comments

@hernanrz
Copy link

Describe the bug

The streaming server allows multiple incoming RTMP streams for the same stream key, leading to unexpected playback behavior. This manifests as:

  • Intermittent switching between the two streams during playback.
  • Rapid and frequent transitions between "live" and "offline" states, as indicated by incoming/closing RTMP admission webhooks.

To Reproduce

See error:

  • Observe the unexpected playback behavior described above.
  • Monitor the server logs and webhooks for evidence of multiple incoming streams and the corresponding live/offline transitions.

Expected behavior

  • The server should only accept one incoming RTMP stream for a given stream key. Subsequent attempts to stream to the same key should either be rejected or handled gracefully (e.g., by replacing the existing stream).

Logs

These logs are from our live instance of OME, we believe network issues on users' end cause OBS to reconnect, or attempt to reconnect, while the stream is already live in OME.

imagen

Server

  • OvenMediaEngine Version: latest docker image
@bchah
Copy link
Collaborator

bchah commented Dec 13, 2024

There is a setting just for this scenario. VirtualHost -> Application -> Providers -> RTMP -> BlockDuplicateStreamName = true

...
<Application>
<Providers>
<RTMP>
  <BlockDuplicateStreamName>true</BlockDuplicateStreamName>
</RTMP>
<SRT>
  <BlockDuplicateStreamName>true</BlockDuplicateStreamName>
</SRT>

Or you could use AdmissionWebhooks and check if the stream exists before allowing it.

@naanlizard
Copy link
Contributor

It looks like BlockDuplicateStreamName defaults to true already - so perhaps it isn't working?

https://airensoft.gitbook.io/ovenmediaengine/live-source/rtmp#rtmp-live-stream

@bchah
Copy link
Collaborator

bchah commented Dec 14, 2024

That would be a regression. Haven't tested for that in a bit... Got logs?

@naanlizard
Copy link
Contributor

naanlizard commented Dec 14, 2024 via email

@getroot getroot added the investigating Being checked label Dec 16, 2024
@getroot
Copy link
Member

getroot commented Dec 16, 2024

This might be an issue that occurs when streams with the same name are attempted to be created almost simultaneously.
Is my assumption correct? If so, I think I know where the problem is.

@hernanrz
Copy link
Author

This might be an issue that occurs when streams with the same name are attempted to be created almost simultaneously. Is my assumption correct? If so, I think I know where the problem is.

Yes. We've temporarily added a check in our admission webhook, to reject an rtmp connection if it's already live, which has solved the flickering the player sees and flickering between offline and online states, but now we see that OME triggers the "offline" webhook after the "publish" rtmp webhook is rejected, even though the stream is live, so our player shows a "stream is offline" message even though OBS will remain connected, hope that was clear, it's a bit hard to describe this issue, please let me know if it isn't clear or if you need more details!

@getroot
Copy link
Member

getroot commented Dec 16, 2024

I think I know what the problem's cause is.
I will solve this problem. Thank you for reporting it.

@getroot getroot assigned getroot and unassigned dimiden Dec 16, 2024
@getroot getroot added bug Confirmed as bug and removed investigating Being checked labels Dec 16, 2024
@getroot
Copy link
Member

getroot commented Dec 17, 2024

@hernanrz @naanlizard @bchah

There’s one policy we need to discuss.

AdmissionWebhooks sends an opening event when stream creation starts and a closing event when the stream ends.

Currently, a closing event is sent even if the control server rejects the stream. A closing event is also triggered if the stream name is duplicated or if other errors occur (e.g., an encoder-related error).

Regarding your recent report — "OME triggers 'offline' when the control server rejects it" — this is likely because a closing event was sent for a stream with the same name.

So the question is:

  • Should a closing event not be sent when the server rejects the stream?
  • Or should a closing event always be sent regardless, and the server re-check the offline status via the API?

I recall someone mentioning that the number of opening and closing events must always match.

@hernanrz
Copy link
Author

I noticed that behavior, I'm not sure if it makes sense to call the "close" event if the "open" was rejected, but if that's a behavior that must be kept to keep the number of "open" and "close" events equal, I think a new field in the "close" webhook payload could be added, to indicate the reason it's closing (for example: "client disconnected" or "admissionwebhook rejected")

@bchah
Copy link
Collaborator

bchah commented Dec 17, 2024

Should a closing event not be sent when the server rejects the stream?
Or should a closing event always be sent regardless, and the server re-check the offline status via the API?

I believe that what is never opened cannot be closed. However as suggested, a close with a status like "AdmissionWebhook Rejected" would be OK. It would nice to see responses as an enum rather than matching strings, but that's more of a feature request than a bug discussion 🥸

@getroot
Copy link
Member

getroot commented Dec 18, 2024

We will improve this so that close is not sent for rejected streams.
And we have fixed the problem of creating a new stream even if there is already a stream with the same name. However, in this case, the closing event is sent. Therefore, the Control Server should handle it appropriately to reduce confusion.

@getroot getroot added the patched Patch applied label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed as bug patched Patch applied
Projects
None yet
Development

No branches or pull requests

5 participants