Repository: bluenviron/gortsplib
Pull request #568 (2024-05-09 03:53:21)

avoid returning partial RTP-Info header, omit seq/rtptime if needed
A recent behavioral change introduced by [PR #2244](https://github.com/bluenviron/mediamtx/pull/2244) has inadvertently caused a bug affecting the initialization timing of the RTSP server, which now waits until the first playback connection attempt. This change has resulted in playback failures in some video players due to the timing of the server's start-up sequence. The root of the issue lies in the `OnSetup` handler in [server_session.go](https://github.com/bluenviron/gortsplib/blob/877771e4c547546db3c93cec4381636648cf9efc/server_session.go#L747), which activates the RTSP server processor found in [stream.go](https://github.com/bluenviron/mediamtx/blob/3e7cc0470f69b90c0d6dec3340514b1fd7d99701/internal/stream/stream.go#L102).

The server's setup process allows `gortsplib` to asynchronously process packets and extract stream data such as video and audio tracks. However, the immediate return of `OnSetup` means that the `generateRTPInfo` function might try to construct an `RTP-Info` header before all tracks have been detected by `gortsplib`, leading to incomplete or incorrect headers.

Currently, the server prematurely constructs `RTP-Info` headers based on a list from `setuppedMediasOrdered`. This results in headers that may lack complete track data if `gortsplib` has not yet processed all packets required to detect the tracks, as seen in the conditional check in [server_stream.go](https://github.com/bluenviron/gortsplib/blob/877771e4c547546db3c93cec4381636648cf9efc/server_stream.go#L124-L127).

This PR resolves the issue by ensuring that even if the packet data for a track isn't fully available at the time of constructing the `RTP-Info` header, a partial but valid `RTP-Info` URL entry will still be included. This modification prevents the generation of headers that could cause strict RTSP implementations to fail during playback.

**Examples of RTP-Info Headers:**

*Valid header formats:*
- `RTP-Info: url=rtsp://127.0.0.1:8554/foo/trackID=0;seq=55350;rtptime=712469876, url=rtsp://127.0.0.1:8554/foo/trackID=1;seq=17411;rtptime=3933183075`

*Invalid header formats (due to race condition, one of the tracks is missing):*
- Completely absent `RTP-Info`
- `RTP-Info: url=rtsp://127.0.0.1:8554/foo/trackID=1;seq=17411;rtptime=3933183075`
- `RTP-Info: url=rtsp://127.0.0.1:8554/foo/trackID=0;seq=55350;rtptime=712469876`

*Proposed fix example (for missing data cases):*
- `RTP-Info: url=rtsp://127.0.0.1:8554/foo/trackID=0, url=rtsp://127.0.0.1:8554/foo/trackID=1;seq=17411;rtptime=3933183075`

**Summary of the Fix:**
- Modify `generateRTPInfo` to ensure each track at least has a valid URL entry in the `RTP-Info` header, even if sequence and RTP time data are not yet available.
- This change helps prevent playback issues in clients strictly parsing RTP headers, ensuring better reliability and user experience.

aler9: Hello, you're perfectly right about the fact that the `RTP-Info` header is often sent with one or more missing media streams (tracks), since the server must have at least a RTP packet for each media stream in order to generate it. Sometimes this packet will never be available, since a RTSP server/client is free to declare a media and never use it, therefore waiting for it would be useless, and this is why the mechanism is totally asynchronous.

The question here is whether it is better to send a partial RTP-Info entry or to omit it, when no packets are available yet.

Can you write the name of the client that prefers partial RTP-info entries with respect to no entries? unfortunately in the specifications there is no trace of such partial RTP-infos, therefore if we want to merge this, we need a reference. Thanks
Fusl: Windows Media Foundation is the problematic application here:

![Screenshot_20240509-092924~2.png](https://github.com/bluenviron/gortsplib/assets/2349496/eee47049-8e37-4a76-b64c-f0c5205ca197)

This is being used through NSPlayer by [AVPro Video](https://renderheads.com/products/avpro-video/) which in turn is being used by [VRChat](https://vrchat.com/).

MediaMTX is a common software used by many VRChat users to stream a screen share with OBS or other media in low-latency into user-made VRChat worlds.

The problem here is that all of it relies on the stream being fully established when it's time to call back into the application to set up the player at which point most of the time we only see an audio track in the stream.
aler9: Perfect, fix the tests and this will be merged.
Fusl: There you go, this should make the test pass. For what it's worth, my initial thought was to revert the initialization procedure to occur during the creation of the struct rather than lazily initializing when the first RTSP playback client connects. However, that approach would have rolled back some of the performance improvements we've achieved, so I opted for this solution instead. I’ll continue to explore this issue and if I find a more efficient way to address it, I’ll submit another PR.

Patch:
diff --git a/server_session.go b/server_session.go
index 506b5c9e..affc7433 100644
--- a/server_session.go
+++ b/server_session.go
@@ -128,15 +128,16 @@ func generateRTPInfo(
 
 	for _, sm := range setuppedMediasOrdered {
 		entry := setuppedStream.rtpInfoEntry(sm.media, now)
-		if entry != nil {
-			entry.URL = (&base.URL{
-				Scheme: u.Scheme,
-				Host:   u.Host,
-				Path: setuppedPath + "/trackID=" +
-					strconv.FormatInt(int64(setuppedStream.streamMedias[sm.media].trackID), 10),
-			}).String()
-			ri = append(ri, entry)
-		}
+		if entry == nil {
+			entry = &headers.RTPInfoEntry{}
+		}
+		entry.URL = (&base.URL{
+			Scheme: u.Scheme,
+			Host:   u.Host,
+			Path: setuppedPath + "/trackID=" +
+				strconv.FormatInt(int64(setuppedStream.streamMedias[sm.media].trackID), 10),
+		}).String()
+		ri = append(ri, entry)
 	}
 
 	if len(ri) == 0 {


Test patch:
diff --git a/server_play_test.go b/server_play_test.go
index 29175625..aa1d6552 100644
--- a/server_play_test.go
+++ b/server_play_test.go
@@ -1901,6 +1901,13 @@ func TestServerPlayAdditionalInfos(t *testing.T) {
 			SequenceNumber: uint16Ptr(557),
 			Timestamp:      (*rtpInfo)[0].Timestamp,
 		},
+		&headers.RTPInfoEntry{
+			URL: (&base.URL{
+				Scheme: "rtsp",
+				Host:   "localhost:8554",
+				Path:   mustParseURL((*rtpInfo)[1].URL).Path,
+			}).String(),
+		},
 	}, rtpInfo)
 	require.Equal(t, []*uint32{
 		uint32Ptr(96342362),