[DATA-751] Add RTSP model #1653
Conversation
bhaney
added
the
safe to test
viambot
removed
the
safe to test
go.mod
Outdated
| github.com/aws/aws-sdk-go v1.41.14 // indirect | ||
| github.com/bamiaux/iobit v0.0.0-20170418073505-498159a04883 // indirect | ||
| github.com/benbjohnson/clock v1.3.0 // indirect | ||
| github.com/beorn7/perks v1.0.1 // indirect | ||
| github.com/bhaney/rtsp-simple-server v0.0.0-20221207231808-7a1402c0b840 // indirect |
There was a problem hiding this comment.
This is literally just used for the test. Needed to fork the repo because it did not expose the server as a package, only as a binary.
bhaney
added
the
safe to test
viambot
removed
the
safe to test
bhaney
added
the
safe to test
viambot
removed
the
safe to test
bhaney
added
the
safe to test
|
Thanks for sharing this! I hadn't seen an example of using a codec in go before. |
| // #include <libswscale/swscale.h> | ||
| import "C" | ||
|
|
||
| import ( |
There was a problem hiding this comment.
Gotta still look through this but I'm curious why we are using this over mediadevices h264 codec
There was a problem hiding this comment.
I thought it was only encoding in mediadevices? Let me check again...
There was a problem hiding this comment.
from what I can tell, https://github.com/pion/mediadevices/blob/master/pkg/codec/x264/x264.go is just the encoder, but this is h264 -> image.RGBA decoding.
There was a problem hiding this comment.
ah. can we not use the x264 c code though so that we have just one dependency?
There was a problem hiding this comment.
Rather than changing the library to just the x264 one, what if I changed this PR to allow any codec available from libavcodec (which is from ffmpeg).It will make it more general for many different formats (Prob could have just done that from the beginning)
the available ones are
- H264
- HEVC (i.e. H265)
- MPEG4
- MPEG2video
- Theora
- VC-1 (i.e. SMPTE 421)
- RAW
There was a problem hiding this comment.
It almost seems better as a modular resource
There was a problem hiding this comment.
So the footprint is pretty significant. Installing JUST those three libraries on a bare-bones debian system brings in 380MB of required other packages/files. Adding them to our AppImage doubles the compressed size from 64MB to 128MB.
As to the two followup questions... I think supporting all the codecs would make a LOT of sense rather than just h264.
Lastly, yeah, due to the size... I think this would be a good candidate as a module. Or we acquire enough new things capable of making use of libavcodec that it becomes worthwhile to put it in the main RDK.
OH, and yeah, hoping that module stuff is done any day now. I'm hoping to be ready for final reviews later today, but i've been saying that since thursday.
There was a problem hiding this comment.
Wait, but since these libraries are already included in ffmpeg, which we already have in RDK, would it still add to the overall size of RDK? I don't think this would require adding anything new to RDK.
Or does explictly using cgo (rather than just pointing to the ffmpeg binary like we do in the ffmpeg model) mean having to copy those libraries and repeat them?
This PR passed CI without me having to download/add anything to the canon image.
There was a problem hiding this comment.
What if we removed the ffmpeg model, and just included these libraries. Would it decrease the size of RDK? Not that we can remove ffmpeg... but if people are only using ffmpeg for the RTSP capability, we maybe could...
There was a problem hiding this comment.
We don't include ffmpeg in the actual distribution/packaging, just libx264 and libopus. As far as I know, the ffmpeg camera model only calls an external binary. It never actually links to shared cgo code/libraries. But because we have that model, ffmpeg is needed for testing, and so is in the canon docker images.
The actual list of what gets bundled as true dependencies is here: https://github.com/viamrobotics/rdk/blob/main/etc/packaging/appimages/viam-server-x86_64.yml#L36-L41 (though those few packages pull in a couple more of their own dependencies.)
|
Are they available yet (modular resources?) Is there one I could use as a template? I can also just delay merging this model into RDK for another time |
|
Okay following up on this and understanding a bit more about what is actually needed, we want h264 decoding. We can't just pull in Instead, we need to find a way to lightly include h264 decoding. There does exist https://github.com/ausocean/h264decode but it looks pretty WIP so far. Alternatively, if that proves too cumbersome, mp4 decoding can be found here https://pkg.go.dev/github.com/jfbus/mp4. I would try some quick tests to see if either works more easily and go with that one since ultimately RTSP should provide either h264 or mp4 commonly. |
This sounds good - I'll work on trying to include both mp4 and h264 decoding (without using avcodec). Talked with Eliot and will hold off on creating a modular resource of RTSP for now |
|
To clarify, I'd pick one of the decodings, but sounds good. |
|
So, libx264 does not have a decoder. It is only an encoder. Everywhere I look, the recommendation for decoding x264 is "just use ffmpeg", which we've already decided is a no-go. I looked at using MP4, but this would require not using https://github.com/aler9/gortsplib, because it only support MP4Audio, not full MP4 currently. Since the whole RTSP model is built on aler9/gortsplib, one thing I could do is try to create a PR that adds full MP4 support to that library. I think my course of action right now will be to use/modify https://github.com/ausocean/h264decode/ which is trying to natively decode H264 in golang. |
|
Did you check out https://pkg.go.dev/github.com/jfbus/mp4#section-readme? |
It's the RTSP client that doesn't support MP4. The problem is the conversion from RTP -> MP4, not decoding the MP4 -> image.Image. The Go RTSP client supports MPEG4Audio only right now. So an option would be to add the RTP->MP4 functions to https://github.com/aler9/gortsplib/, but it would need to be a PR on that repo. |
|
We can also full fork (non go mod replace) it if it's an easy path while waiting on a PR. |
OK - I think I was confused, I thought MP4 and H264 were different things. James helpfully explained that MP4 is a container for video, and it can be used to contain the encoded video in H264. So I looked more closely at the mp4 package you sent me, and it looks like it doesn't decode the data? It just stores the packets... it says in the comments "Status: No decoded". https://github.com/jfbus/mp4/blob/master/mdat.go It defines the "Box" to store the data in, but it doesn't decode the data. |
|
So is the RTSP we're working with sending only h264? is there mjpeg? |
I wrote it for only h264 because I thought that would be the most common encoding, but mjpeg is supported! |
bhaney
force-pushed
the
DATA-751-add-rtsp-model
branch
from
143e429 to
6276a45
Compare
viambot
added
safe to test
bhaney
force-pushed
the
DATA-751-add-rtsp-model
branch
from
b21186e to
363e0df
Compare
viambot
added
safe to test
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest ac0a9c0~1=(9bce7dc) instead
|
| case <-cancelCtx.Done(): | ||
| return nil, nil, cancelCtx.Err() | ||
| case <-ctx.Done(): | ||
| return nil, nil, ctx.Err() | ||
| default: | ||
| <-rtspCam.gotFirstFrame // block until you get the first frame | ||
| } |
There was a problem hiding this comment.
Ah so this would've had to be (no default)
select {
case <-cancelCtx.Done():
return nil, nil, cancelCtx.Err()
case <-ctx.Done():
return nil, nil, ctx.Err()
case <-rtspCam.gotFirstFrame: // block until you get the first frame
}
to get rid of the issue. That way the select could block until one of those conditions are true. As of now it could still not see a Done(), fall into the default case, then block indefinitely on <-rtspCam.gotFirstFrame if the goroutine's parent function tried to kill it before sending a frame. Not a big deal though. Just wanted to call it out in the very unlikely chance this gets triggered.
There was a problem hiding this comment.
we should probably fix this. unlikely is possible
There was a problem hiding this comment.
I can change it to a (Wait for Chan or 10s) and then continue after 10s to look for the Done() again
There was a problem hiding this comment.
Eric mentioned that the block you propose has its own problems, so I need to create a way to always be listening on the contexts. Essentially the easiest way will actually just putting in the select statement twice - once without the blocking gotFrame, and one with.
Uses gortsplib to create an RTSP camera client in RDK
rtsprtsp_addressH264MJPEG streams currently!Uses a fork of simple-rtsp-server to do testing (original package hides server functions in an internal package)