| Message ID | 20190405235929.27987-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Sat, Apr 06, 2019 at 01:59:28AM +0200, Niklas Söderlund wrote: > The completion handler needs to handle all buffers in the request. Solve > this by iterating over all buffers in the completed request. The streams > are named automatically streamX, where X is the order of how the stream s/how/which/ > was passed to configureStream(). > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index da05612b1c347b31..da5ca3402ce1a823 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -23,6 +23,7 @@ using namespace libcamera; > > OptionsParser::Options options; > std::shared_ptr<Camera> camera; > +std::map<Stream *, std::string> streamInfo; > EventLoop *loop; > BufferWriter *writer; Unrelated to this patch, we should really create a class to hold all this. > > @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config) > { > std::vector<StreamUsage> roles; > > + streamInfo.clear(); > + > /* If no configuration is provided assume a single video stream. */ > if (!options.isSet(OptStream)) { > *config = camera->streamConfiguration({ Stream::VideoRecording() }); > + streamInfo[config->front()] = "stream0"; > return 0; > } > > @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config) > (*config)[stream].pixelFormat = conf["pixelformat"]; > } > > + for (unsigned int i = 0; i < config->size(); i++) > + streamInfo[(*config)[i]] = "stream" + std::to_string(i); How about using iterators ? index = 0; for (Stream *stream : *config) { streamInfo[stream] = "stream" + std::to_string(index); index++; } > + > return 0; > } > > static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers) > { > static uint64_t last = 0; > + double fps = 0.0; > > if (request->status() == Request::RequestCancelled) > return; > > - Buffer *buffer = buffers.begin()->second; > + for (auto it = buffers.begin(); it != buffers.end(); ++it) { > + Stream *stream = it->first; > + Buffer *buffer = it->second; > + std::string name = streamInfo[stream]; > > - double fps = buffer->timestamp() - last; > - fps = last && fps ? 1000000000.0 / fps : 0.0; > - last = buffer->timestamp(); > + if (it == buffers.begin()) { > + fps = buffer->timestamp() - last; > + fps = last && fps ? 1000000000.0 / fps : 0.0; > + last = buffer->timestamp(); > + } > > - std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence() > - << " buf: " << buffer->index() > - << " bytesused: " << buffer->bytesused() > - << " timestamp: " << buffer->timestamp() > - << " fps: " << std::fixed << std::setprecision(2) << fps > - << std::endl; > + std::cout << name << " seq: " << std::setw(6) > + << std::setfill('0') << buffer->sequence() > + << " buf: " << buffer->index() > + << " bytesused: " << buffer->bytesused() > + << " timestamp: " << buffer->timestamp() > + << " fps: " << std::fixed << std::setprecision(2) << fps As the fps is per-request and not per-stream we should ideally print it once only. One option would be to print a single line per request that contains per-request information as well as per-stream information for each stream, but that may be difficult to read. Another option would be to first print per-request information, and then one line for each stream. This may be best, otherwise the log stream0 seq:... stream1 seq:... could be understood as either one request containing both streams, or two requests containing stream0 and stream1 respectively. I'm tempted to see if we could go for the single line per request option while keeping it readable enough. > + << std::endl; > > - if (writer) > - writer->write(buffer, "stream0"); > + if (writer) > + writer->write(buffer, name); > + } > > request = camera->createRequest(); > if (!request) {
Hi Laurent, Thanks for your feedback. On 2019-04-06 20:23:40 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sat, Apr 06, 2019 at 01:59:28AM +0200, Niklas Söderlund wrote: > > The completion handler needs to handle all buffers in the request. Solve > > this by iterating over all buffers in the completed request. The streams > > are named automatically streamX, where X is the order of how the stream > > s/how/which/ > > > was passed to configureStream(). > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------ > > 1 file changed, 27 insertions(+), 12 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index da05612b1c347b31..da5ca3402ce1a823 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -23,6 +23,7 @@ using namespace libcamera; > > > > OptionsParser::Options options; > > std::shared_ptr<Camera> camera; > > +std::map<Stream *, std::string> streamInfo; > > EventLoop *loop; > > BufferWriter *writer; > > Unrelated to this patch, we should really create a class to hold all > this. I agree, I feel bad when I have to add to this hack. I will try to find time to rework this in a follow up patch. > > > > > @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config) > > { > > std::vector<StreamUsage> roles; > > > > + streamInfo.clear(); > > + > > /* If no configuration is provided assume a single video stream. */ > > if (!options.isSet(OptStream)) { > > *config = camera->streamConfiguration({ Stream::VideoRecording() }); > > + streamInfo[config->front()] = "stream0"; > > return 0; > > } > > > > @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config) > > (*config)[stream].pixelFormat = conf["pixelformat"]; > > } > > > > + for (unsigned int i = 0; i < config->size(); i++) > > + streamInfo[(*config)[i]] = "stream" + std::to_string(i); > > How about using iterators ? > > index = 0; > for (Stream *stream : *config) { > streamInfo[stream] = "stream" + std::to_string(index); > index++; > } Sure. > > > + > > return 0; > > } > > > > static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers) > > { > > static uint64_t last = 0; > > + double fps = 0.0; > > > > if (request->status() == Request::RequestCancelled) > > return; > > > > - Buffer *buffer = buffers.begin()->second; > > + for (auto it = buffers.begin(); it != buffers.end(); ++it) { > > + Stream *stream = it->first; > > + Buffer *buffer = it->second; > > + std::string name = streamInfo[stream]; > > > > - double fps = buffer->timestamp() - last; > > - fps = last && fps ? 1000000000.0 / fps : 0.0; > > - last = buffer->timestamp(); > > + if (it == buffers.begin()) { > > + fps = buffer->timestamp() - last; > > + fps = last && fps ? 1000000000.0 / fps : 0.0; > > + last = buffer->timestamp(); > > + } > > > > - std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence() > > - << " buf: " << buffer->index() > > - << " bytesused: " << buffer->bytesused() > > - << " timestamp: " << buffer->timestamp() > > - << " fps: " << std::fixed << std::setprecision(2) << fps > > - << std::endl; > > + std::cout << name << " seq: " << std::setw(6) > > + << std::setfill('0') << buffer->sequence() > > + << " buf: " << buffer->index() > > + << " bytesused: " << buffer->bytesused() > > + << " timestamp: " << buffer->timestamp() > > + << " fps: " << std::fixed << std::setprecision(2) << fps > > As the fps is per-request and not per-stream we should ideally print it > once only. One option would be to print a single line per request that > contains per-request information as well as per-stream information for > each stream, but that may be difficult to read. Another option would be > to first print per-request information, and then one line for each > stream. This may be best, otherwise the log > > stream0 seq:... > stream1 seq:... > > could be understood as either one request containing both streams, or > two requests containing stream0 and stream1 respectively. > > I'm tempted to see if we could go for the single line per request > option while keeping it readable enough. I had an attempt of makig this a single line, lets see how it plays out. > > > + << std::endl; > > > > - if (writer) > > - writer->write(buffer, "stream0"); > > + if (writer) > > + writer->write(buffer, name); > > + } > > > > request = camera->createRequest(); > > if (!request) { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index da05612b1c347b31..da5ca3402ce1a823 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -23,6 +23,7 @@ using namespace libcamera; OptionsParser::Options options; std::shared_ptr<Camera> camera; +std::map<Stream *, std::string> streamInfo; EventLoop *loop; BufferWriter *writer; @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config) { std::vector<StreamUsage> roles; + streamInfo.clear(); + /* If no configuration is provided assume a single video stream. */ if (!options.isSet(OptStream)) { *config = camera->streamConfiguration({ Stream::VideoRecording() }); + streamInfo[config->front()] = "stream0"; return 0; } @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config) (*config)[stream].pixelFormat = conf["pixelformat"]; } + for (unsigned int i = 0; i < config->size(); i++) + streamInfo[(*config)[i]] = "stream" + std::to_string(i); + return 0; } static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers) { static uint64_t last = 0; + double fps = 0.0; if (request->status() == Request::RequestCancelled) return; - Buffer *buffer = buffers.begin()->second; + for (auto it = buffers.begin(); it != buffers.end(); ++it) { + Stream *stream = it->first; + Buffer *buffer = it->second; + std::string name = streamInfo[stream]; - double fps = buffer->timestamp() - last; - fps = last && fps ? 1000000000.0 / fps : 0.0; - last = buffer->timestamp(); + if (it == buffers.begin()) { + fps = buffer->timestamp() - last; + fps = last && fps ? 1000000000.0 / fps : 0.0; + last = buffer->timestamp(); + } - std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence() - << " buf: " << buffer->index() - << " bytesused: " << buffer->bytesused() - << " timestamp: " << buffer->timestamp() - << " fps: " << std::fixed << std::setprecision(2) << fps - << std::endl; + std::cout << name << " seq: " << std::setw(6) + << std::setfill('0') << buffer->sequence() + << " buf: " << buffer->index() + << " bytesused: " << buffer->bytesused() + << " timestamp: " << buffer->timestamp() + << " fps: " << std::fixed << std::setprecision(2) << fps + << std::endl; - if (writer) - writer->write(buffer, "stream0"); + if (writer) + writer->write(buffer, name); + } request = camera->createRequest(); if (!request) {
The completion handler needs to handle all buffers in the request. Solve this by iterating over all buffers in the completed request. The streams are named automatically streamX, where X is the order of how the stream was passed to configureStream(). Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)