| Message ID | 20200629153518.3734304-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Jun 29, 2020 at 05:35:17PM +0200, Niklas Söderlund wrote: > It's confusing to read the code and understand that a request is only > completed before being processed by the ImgU if it only contains a > single RAW buffer. Add a boolean variable with a explanatory name to > make this clearer, no functional change. Be careful, if you think the existing code lacks clarity here, I'll make you document every single function of the pipeline handler classes :-) > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1bdad209de6e47fb..4e75bd40c66f821a 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -874,10 +874,13 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > * If the request contains a buffer for the RAW stream only, complete it > * now as there's no need for ImgU processing. > */ > - if (request->findBuffer(&rawStream_) && > - pipe_->completeBuffer(camera_, request, buffer)) { > - pipe_->completeRequest(camera_, request); > - return; > + if (request->findBuffer(&rawStream_)) { > + bool requestComplete = > + pipe_->completeBuffer(camera_, request, buffer); I'd have named the variable complete or isComplete just to avoid the line break :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + if (requestComplete) { > + pipe_->completeRequest(camera_, request); > + return; > + } > } > > imgu_->input_->queueBuffer(buffer);
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 1bdad209de6e47fb..4e75bd40c66f821a 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -874,10 +874,13 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) * If the request contains a buffer for the RAW stream only, complete it * now as there's no need for ImgU processing. */ - if (request->findBuffer(&rawStream_) && - pipe_->completeBuffer(camera_, request, buffer)) { - pipe_->completeRequest(camera_, request); - return; + if (request->findBuffer(&rawStream_)) { + bool requestComplete = + pipe_->completeBuffer(camera_, request, buffer); + if (requestComplete) { + pipe_->completeRequest(camera_, request); + return; + } } imgu_->input_->queueBuffer(buffer);
It's confusing to read the code and understand that a request is only completed before being processed by the ImgU if it only contains a single RAW buffer. Add a boolean variable with a explanatory name to make this clearer, no functional change. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)