| Message ID | 20241018092716.295624-3-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-10-18 10:27:15) > Simple pipeline retrieves the requests to complete from the > conversionQueue_. This patch stores the requests in conversionQueue_ > explicitly. This explicit tracking is supposed to be preferred to > implicit retrieval and it simplifies the completion code a bit here and > in the followup patch that adds request cleanup on stop. > > The change as implemented assumes that all the buffers in each of the > conversionQueue_ elements point to the same request, the one specified. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 3ddce71d3..a1339d87c 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -282,7 +282,7 @@ public: > std::unique_ptr<DelayedControls> delayedCtrls_; > > std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; > - std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; > + std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_; > bool useConversion_; > > std::unique_ptr<Converter> converter_; > @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > if (conversionQueue_.empty()) > return; > > - Request *request = nullptr; > - for (auto &item : conversionQueue_.front()) { > - FrameBuffer *outputBuffer = item.second; > - request = outputBuffer->request(); > - pipe->completeBuffer(request, outputBuffer); > - } > + Request *request = conversionQueue_.front().first; > + for (auto &item : conversionQueue_.front().second) > + pipe->completeBuffer(request, item.second); I'll always have a personal dislike for std::pair<> because something.first and something.second just really doesn't tell me what it's supposed to be. Even more so with "auto &item : conversionQueue_.front().second" > + pipe->completeRequest(request); > conversionQueue_.pop(); > > - if (request) > - pipe->completeRequest(request); > return; > } > > @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > > if (useConversion_ && !conversionQueue_.empty()) { > const std::map<const Stream *, FrameBuffer *> &outputs = > - conversionQueue_.front(); > + conversionQueue_.front().second; > if (!outputs.empty()) { > FrameBuffer *outputBuffer = outputs.begin()->second; > if (outputBuffer) > @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > } > > if (converter_) > - converter_->queueBuffers(buffer, conversionQueue_.front()); > + converter_->queueBuffers(buffer, conversionQueue_.front().second); > else > /* > * request->sequence() cannot be retrieved from `buffer' inside > @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > * already here. > */ > swIsp_->queueBuffers(request->sequence(), buffer, > - conversionQueue_.front()); > + conversionQueue_.front().second); > > conversionQueue_.pop(); > return; > @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > } > > if (data->useConversion_) { > - data->conversionQueue_.push(std::move(buffers)); > + data->conversionQueue_.push(std::make_pair(request, std::move(buffers))); As far as I can tell all the above looks correct, even with my dislike of std::pair - and it's /working/ Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> If there were a reason to respin, and std::pair could be more readable I'd be happier - but I can still do this I think: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > if (data->swIsp_) > data->swIsp_->queueRequest(request->sequence(), request->controls()); > } > -- > 2.44.1 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-10-18 10:27:15) >> Simple pipeline retrieves the requests to complete from the >> conversionQueue_. This patch stores the requests in conversionQueue_ > >> explicitly. This explicit tracking is supposed to be preferred to >> implicit retrieval and it simplifies the completion code a bit here and >> in the followup patch that adds request cleanup on stop. >> >> The change as implemented assumes that all the buffers in each of the >> conversionQueue_ elements point to the same request, the one specified. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++------------- >> 1 file changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 3ddce71d3..a1339d87c 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -282,7 +282,7 @@ public: >> std::unique_ptr<DelayedControls> delayedCtrls_; >> >> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; >> - std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; >> + std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_; >> bool useConversion_; >> >> std::unique_ptr<Converter> converter_; >> @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> if (conversionQueue_.empty()) >> return; >> >> - Request *request = nullptr; >> - for (auto &item : conversionQueue_.front()) { >> - FrameBuffer *outputBuffer = item.second; >> - request = outputBuffer->request(); >> - pipe->completeBuffer(request, outputBuffer); >> - } >> + Request *request = conversionQueue_.front().first; >> + for (auto &item : conversionQueue_.front().second) >> + pipe->completeBuffer(request, item.second); > > I'll always have a personal dislike for std::pair<> because > something.first and something.second just really doesn't tell me what > it's supposed to be. > > Even more so with > "auto &item : conversionQueue_.front().second" > > > > >> + pipe->completeRequest(request); >> conversionQueue_.pop(); >> >> - if (request) >> - pipe->completeRequest(request); >> return; >> } >> >> @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> >> if (useConversion_ && !conversionQueue_.empty()) { >> const std::map<const Stream *, FrameBuffer *> &outputs = >> - conversionQueue_.front(); >> + conversionQueue_.front().second; >> if (!outputs.empty()) { >> FrameBuffer *outputBuffer = outputs.begin()->second; >> if (outputBuffer) >> @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> } >> >> if (converter_) >> - converter_->queueBuffers(buffer, conversionQueue_.front()); >> + converter_->queueBuffers(buffer, conversionQueue_.front().second); >> else >> /* >> * request->sequence() cannot be retrieved from `buffer' inside >> @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> * already here. >> */ >> swIsp_->queueBuffers(request->sequence(), buffer, >> - conversionQueue_.front()); >> + conversionQueue_.front().second); >> >> conversionQueue_.pop(); >> return; >> @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >> } >> >> if (data->useConversion_) { >> - data->conversionQueue_.push(std::move(buffers)); >> + data->conversionQueue_.push(std::make_pair(request, std::move(buffers))); > > As far as I can tell all the above looks correct, even with my dislike > of std::pair - and it's /working/ > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > If there were a reason to respin, and std::pair could be more readable > I'd be happier - but I can still do this I think: Let's make you happier, this is a safe and easy change, I'll post v5 with it. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> if (data->swIsp_) >> data->swIsp_->queueRequest(request->sequence(), request->controls()); >> } >> -- >> 2.44.1 >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 3ddce71d3..a1339d87c 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -282,7 +282,7 @@ public: std::unique_ptr<DelayedControls> delayedCtrls_; std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; - std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; + std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_; bool useConversion_; std::unique_ptr<Converter> converter_; @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) if (conversionQueue_.empty()) return; - Request *request = nullptr; - for (auto &item : conversionQueue_.front()) { - FrameBuffer *outputBuffer = item.second; - request = outputBuffer->request(); - pipe->completeBuffer(request, outputBuffer); - } + Request *request = conversionQueue_.front().first; + for (auto &item : conversionQueue_.front().second) + pipe->completeBuffer(request, item.second); + pipe->completeRequest(request); conversionQueue_.pop(); - if (request) - pipe->completeRequest(request); return; } @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) if (useConversion_ && !conversionQueue_.empty()) { const std::map<const Stream *, FrameBuffer *> &outputs = - conversionQueue_.front(); + conversionQueue_.front().second; if (!outputs.empty()) { FrameBuffer *outputBuffer = outputs.begin()->second; if (outputBuffer) @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) } if (converter_) - converter_->queueBuffers(buffer, conversionQueue_.front()); + converter_->queueBuffers(buffer, conversionQueue_.front().second); else /* * request->sequence() cannot be retrieved from `buffer' inside @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) * already here. */ swIsp_->queueBuffers(request->sequence(), buffer, - conversionQueue_.front()); + conversionQueue_.front().second); conversionQueue_.pop(); return; @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) } if (data->useConversion_) { - data->conversionQueue_.push(std::move(buffers)); + data->conversionQueue_.push(std::make_pair(request, std::move(buffers))); if (data->swIsp_) data->swIsp_->queueRequest(request->sequence(), request->controls()); }
Simple pipeline retrieves the requests to complete from the conversionQueue_. This patch stores the requests in conversionQueue_ explicitly. This explicit tracking is supposed to be preferred to implicit retrieval and it simplifies the completion code a bit here and in the followup patch that adds request cleanup on stop. The change as implemented assumes that all the buffers in each of the conversionQueue_ elements point to the same request, the one specified. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)