search

geeksville / Micro-RTSP

A RTSP video server intended for very small CPUs (ESP32 etc)
MIT License
755 stars 199 forks source link

2nd try to do pull req: refactored client commands processing to be more compliant to the rfc standard. #39

Closed kadavris closed 10 months ago

kadavris commented 3 years ago

Also fixed some issues, combed some code to be more readable. tested on windows and esp32 ai thinker with 1-2 simultaneous clients (vlc via tcp)

brainrecall commented 3 years ago

This is looking good, and I started using your changes, but I believe the examples/ESP32-devcam.ino no longer compiles with the changes to some of the APIs. Also, the example doesn't appear to support multiple clients correctly.

kadavris commented 3 years ago

@brainrecall Glad to hear. I though this project is effectively dead already ;( . I did not tried one in examples as I have no lcd module it uses. And it is a bit outdated indeed, The one I did run is in the 'test' folder.

brainrecall commented 3 years ago

Actually, I think I discovered a problem. Seems like it can't parse the client sockets from the SETUP, the server will fail to PLAY, and the client will revert back to TCP which succeeds, but I found is significantly slower on the ESP32CAM so not desirable. I suggest you watch the connection on Wireshark and you'll see the port issue.

brainrecall commented 3 years ago

And, I think I got a fix. In CRtspSession.c, around line 321. if ( m_ClientRTPPort == -1 && 0 == strncmp( cur_pos, "client_port=", 12 ) ) This is being skipped, it never even tries to find the client_port. What I discovered is m_ClientRTPPort is a uint16. Apparently newer GCC compilers are treating a comparison between an unsigned short and -1 as always false, so it completely removes the if! I would suggest not bothering to check if m_ClientRTPPort was unset or explicitly set and check m_ClientRTPPort as 65535.

brainrecall commented 3 years ago

I've been working on this some more, and I've come to a few conclusions. Most of these aren't related directly to your work, @kadavris , which has been excellent in helping this code, but I'd thought I'd bring them up:

  1. Support for multiple sessions is broken, certainly for UDP. It might work somewhat for TCP, but I'm suspecting there are pointer issues after a client disconnects. I've been working on reverting the changes from that PR: https://github.com/geeksville/Micro-RTSP/pull/20/files
  2. The ESP32 simply doesn't have the horsepower to support multiple clients, further making me want to remove the extra code.
  3. Streaming over UDP is significantly faster compared to TCP, like double the framerate. VLC by default will try UDP first, and then revert to TCP if it fails, so using this branch's code is highly desirable since it fixes all the initial connection issues the parent has. That means VLC will connect successfully, first time, with UDP, getting faster framerates.
  4. There are a ton of redundant static buffers in the code that are eating RAM. Not only do static buffers break the entire point of C++ objects, they can't be reused between functions and they eat program space as well. I've been working on making them dynamic, maintained by the object, and sized more appropriately. Doing those simple things saves about 16KB of RAM. There is probably a lot more to save.
  5. The ESP32CAM gets hot, really hot. Both the ESP32 and the voltage regulator need some cooling on them to maintain stability (either a small fan or large heatsink).

I'm still working on some issues, but I think my plan is to branch from your code and put in my fixes (mainly the multi-session reversion).