Conversation
- RawNv12TcpSource reads fixed-size NV12 frames from TCP - CLI: --raw-nv12, --raw-width, --raw-height, --raw-fps - Publish via VideoSource::captureFrame (no decode)
|
|
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
thanks, it looks good, some comments.
Sorry for being late on reviewing, I was traveling international in the past week.
| socket_t connectTcp(const std::string &host, std::uint16_t port) { | ||
| #ifdef _WIN32 | ||
| WSADATA wsa; | ||
| if (WSAStartup(MAKEWORD(2, 2), &wsa) != 0) |
There was a problem hiding this comment.
I am not that familiar with the windows API. Do we need to clean up / release the resource explicitly in Stop() as we are calling WSAStartup() in connectTcp() function ?
| auto t0 = std::chrono::steady_clock::now(); | ||
|
|
||
| while (running_.load()) { | ||
| std::vector<std::uint8_t> frame(frame_size_); |
There was a problem hiding this comment.
We'd better not allocate a new vector of frame() for every run inside the loop.
Move it out, and reserve the frame_size_, like
std::vectorstd::uint8_t buf(frame_size_);
If the frame_size_ can change / increased over time, ideally we can allocate a big enough buffer, otherwise just double the vector if frame_size_ is bigger the size of frames
| std::size_t filled = 0; | ||
| while (filled < frame_size_ && running_.load()) { | ||
| #ifdef _WIN32 | ||
| int n = recv(fd, reinterpret_cast<char *>(frame.data() + filled), |
There was a problem hiding this comment.
will recv() block the application forever in some cases?
I wonder if we can add a recv timeout or allow the application to interrupt with ctrl + c ?
| #else | ||
| ssize_t n = recv(fd, frame.data() + filled, frame_size_ - filled, 0); | ||
| #endif | ||
| if (n <= 0) { |
There was a problem hiding this comment.
Log it ? I wonder what use case recv will return negative values
| YuvFrame out; | ||
| out.data = std::move(frame); | ||
| out.timestamp_us = ts_us; | ||
| callback_(std::move(out)); |
There was a problem hiding this comment.
nit, replace the whole block with
if (callback_) {
callback_(YuvFrame{std::move(frame), ts_us});
}
| void YuvSource::stop() { | ||
| running_.store(false); | ||
| if (thread_.joinable()) | ||
| thread_.join(); |
There was a problem hiding this comment.
I think .join() will hang the application if the recv() is blocking. Suggestion, set a recv timeout there
|
|
||
| YuvSource::~YuvSource() { stop(); } | ||
|
|
||
| void YuvSource::start() { |
There was a problem hiding this comment.
I know it is an example, but wonder if you want the users to call start() / stop() multiple times ?
or they are supposed to be called only once ?
btw, if start() / stop() can be called multiple times WSAStartup(MAKEWORD(2, 2), &wsa) will likely won't work well.
| : host_(host), port_(port), width_(width), height_(height), fps_(fps), | ||
| callback_(std::move(callback)) { | ||
| if (width_ > 0 && height_ > 0) { | ||
| frame_size_ = static_cast<std::size_t>(width_) * |
There was a problem hiding this comment.
nit, do we need to validate the width and height to match the expected format
| return 1; | ||
| } | ||
|
|
||
| auto yuvSource = std::make_unique<publish_yuv::YuvSource>( |
There was a problem hiding this comment.
so the YuvSource only support raw NV12 ? I wonder if it makes sense to rename it to RawNv12Source or Nv12Source
3 participants
A simple example to injest yuv frames and publish to livekit