[RSDK-9846] - Add broader mpeg4 support #89

hexbabe · 2025-02-07T16:49:10Z

Hinges on upstream PR here to allow for dynamic clock rate configuration.

Other big change is massaging the Generic codec format into mpeg4, when it is secretly an mpeg4 track.

Edit: tested that setting the clock rate dynamically is not needed for a healthy MPEG4 stream with mediamtx and the office physical camera. Closing upstream change for the time being.

Image of manual test with physical cam in a relevant Slack thread

hexbabe · 2025-02-11T17:43:00Z

I can take @nicksanford off this review if priority is an issue

randhid · 2025-02-11T18:29:18Z

rtsp.go


 	// List of formats/codecs in priority order
 	codecFormats := []codecFormat{
 		{&h264, H264},
 		{&h265, H265},
 		{&mjpeg, MJPEG},
-		{&mpeg4, MPEG4},
+		{nil, MPEG4}, // ambiguous codec, MPEG4 can be in a generic format


Can you add more context here?

randhid · 2025-02-11T18:29:19Z

rtsp.go

@@ -976,22 +1043,33 @@ func modelToCodec(model resource.Model) (videoCodec, error) {

 // getAvailableCodec determines the first supported codec from a session's SDP data
 // returning Unknown if none are found.
-func getAvailableCodec(session *description.Session) videoCodec {
+func (rc *rtspCamera) getAvailableCodec(session *description.Session) videoCodec {


This function does not need to be on the struct. I don't see it accessing any of the struct's elements,

Ah I see. But why not just return the error there rather than swallow it?

If we return the error, we may skip over the other codecs if we ever change the priority order / add another codec

Add a comment on why we swallow the error there.

randhid · 2025-02-11T18:31:37Z

rtsp.go

+		if codecFormat.codec == MPEG4 {
+			mpeg4, _, err := findMPEG4InSession(session)
+			switch {
+			case err != nil:
+				rc.logger.Debugf("error checking for MPEG4 format: %v", err)
+			case mpeg4 != nil:
+				return MPEG4
+			}
+			continue
+		}
+
+		// For all other codecs, check if format exists directly
+		if media := session.FindFormat(codecFormat.formatPointer); media != nil {


Let's make this entire thing a case structure.

I swapped the order s.t. the special case handling happens after the direct checking of the session. Should we still use case structure? It would cause duplicate code.

Nah this is okay, my goal was readability and this is more readable.

randhid · 2025-02-11T18:32:39Z

rtsp.go

+			switch {
+			case err != nil:
+				rc.logger.Debugf("error checking for MPEG4 format: %v", err)
+			case mpeg4 != nil:
+				return MPEG4
+			}
+			continue


So... We check from mpeg4 to special case it because it's format is sufficiently generic, and when we have an error in this case, return. What is the continue for? Should this be default case to return the other codecs?

The continue was to avoid redundant checking of media := session.FindFormat(codecFormat.formatPointer) since findMPEG4InSession already does that. I refactored it so that we don't continue and decoupled the special generic handling from the direct check.

rtsp.go

randhid · 2025-02-11T18:33:56Z

rtsp.go

+	if !strings.HasPrefix(strings.ToUpper(generic.RTPMap()), "MP4V-ES/") {
+		return nil, fmt.Errorf("generic format is not MPEG4: %s", forma.Codec())


Does blueenviron have a checker for this logic?

After looking for a bit, I could not find one

randhid · 2025-02-11T18:35:25Z

rtsp.go

+		if profileID, ok := fmtp["profile-level-id"]; ok {
+			id, err := strconv.Atoi(profileID)


Let's return after !ok and err rather than nesting if statements.

Using early return now

rtsp.go

…n session from special handling (handling consequent refactor)

randhid · 2025-02-11T19:36:43Z

Do we have a plan for testing?

hexbabe · 2025-02-11T21:07:13Z

Do we have a plan for testing?

So far I've tested

ffmpeg + mediamtx with various stream configurations e.g.

ffmpeg -re -f lavfi -i testsrc=size=640x480:rate=30 \
-c:v mpeg4 -pix_fmt yuv420p -r 30 -g 60 -bf 2 \
-f rtsp -rtsp_transport tcp rtsp://127.0.0.1:8554/live.stream

Physical camera in the office

hexbabe added 4 commits February 7, 2025 11:47

WIP with local replacement to upstream gortsplib

921893b

Remove manual setting of sample rate

2e5ff81

Remove local replacement

c238b66

Revert mediacommon ver

6521eef

hexbabe marked this pull request as ready for review February 11, 2025 17:11

hexbabe requested review from randhid and nicksanford February 11, 2025 17:42

randhid requested changes Feb 11, 2025

View reviewed changes

hexbabe added 2 commits February 11, 2025 14:08

Improve comment about mpeg4 special handling; Decouple direct check i…

d13e079

…n session from special handling (handling consequent refactor)

forma -> f; Return early instead of nesting ifs

49a872a

Add comment about error swallow

3312eb8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-9846] - Add broader mpeg4 support #89

[RSDK-9846] - Add broader mpeg4 support #89

hexbabe commented Feb 7, 2025 •

edited

Loading

hexbabe commented Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid Feb 11, 2025

hexbabe Feb 11, 2025

randhid commented Feb 11, 2025

hexbabe commented Feb 11, 2025

		if !strings.HasPrefix(strings.ToUpper(generic.RTPMap()), "MP4V-ES/") {
		return nil, fmt.Errorf("generic format is not MPEG4: %s", forma.Codec())

		if profileID, ok := fmtp["profile-level-id"]; ok {
		id, err := strconv.Atoi(profileID)

[RSDK-9846] - Add broader mpeg4 support #89

Are you sure you want to change the base?

[RSDK-9846] - Add broader mpeg4 support #89

Conversation

hexbabe commented Feb 7, 2025 • edited Loading

hexbabe commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randhid commented Feb 11, 2025

hexbabe commented Feb 11, 2025

hexbabe commented Feb 7, 2025 •

edited

Loading