| Message ID | 20220207151214.887140-2-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Mon, Feb 07, 2022 at 03:12:13PM +0000, Naushir Patuck wrote: > Simplify the image and embedded buffer matching logic by removing the assumption > that we require a buffer match between the two streams. Instead, if an image > buffer does not match with an embedded data buffer, simply use the ControlList > provided by DelayedControls for the sensor parameters. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 141 ++++-------------- > 1 file changed, 31 insertions(+), 110 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 0755de84c70c..af234bd18c5b 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -2081,122 +2081,43 @@ void RPiCameraData::tryRunPipeline() > > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer) > { > - unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0; > - > - /* Loop until we find a matching bayer and embedded data buffer. */ > - while (!bayerQueue_.empty()) { > - /* Start with the front of the bayer queue. */ > - FrameBuffer *bayerBuffer = bayerQueue_.front().buffer; > - > - /* > - * Find the embedded data buffer with a matching timestamp to pass to > - * the IPA. Any embedded buffers with a timestamp lower than the > - * current bayer buffer will be removed and re-queued to the driver. > - */ > - uint64_t ts = bayerBuffer->metadata().timestamp; > - embeddedBuffer = nullptr; > - while (!embeddedQueue_.empty()) { > - FrameBuffer *b = embeddedQueue_.front(); > - if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) { > - embeddedQueue_.pop(); > - unicam_[Unicam::Embedded].returnBuffer(b); > - embeddedRequeueCount++; > - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " > - << unicam_[Unicam::Embedded].name(); > - } else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) { > - /* We pop the item from the queue lower down. */ > - embeddedBuffer = b; > - break; > - } else { > - break; /* Only higher timestamps from here. */ > - } > - } > - > - if (!embeddedBuffer) { > - bool flushedBuffers = false; > - > - LOG(RPI, Debug) << "Could not find matching embedded buffer"; > - > - if (!sensorMetadata_) { > - /* > - * If there is no sensor metadata, simply return the > - * first bayer frame in the queue. > - */ > - LOG(RPI, Debug) << "Returning bayer frame without a match"; > - bayerFrame = std::move(bayerQueue_.front()); > - bayerQueue_.pop(); > - embeddedBuffer = nullptr; > - return true; > - } > - > - if (!embeddedQueue_.empty()) { > - /* > - * Not found a matching embedded buffer for the bayer buffer in > - * the front of the queue. This buffer is now orphaned, so requeue > - * it back to the device. > - */ > - unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); > - bayerQueue_.pop(); > - bayerRequeueCount++; > - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " > - << unicam_[Unicam::Image].name(); > - } > - > - /* > - * If we have requeued all available embedded data buffers in this loop, > - * then we are fully out of sync, so might as well requeue all the pending > - * bayer buffers. > - */ > - if (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) { > - /* The embedded queue must be empty at this point! */ > - ASSERT(embeddedQueue_.empty()); > - > - LOG(RPI, Warning) << "Flushing bayer stream!"; > - while (!bayerQueue_.empty()) { > - unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); > - bayerQueue_.pop(); > - } > - flushedBuffers = true; > - } > + if (bayerQueue_.empty()) > + return false; > > - /* > - * Similar to the above, if we have requeued all available bayer buffers in > - * the loop, then we are fully out of sync, so might as well requeue all the > - * pending embedded data buffers. > - */ > - if (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) { > - /* The bayer queue must be empty at this point! */ > - ASSERT(bayerQueue_.empty()); > - > - LOG(RPI, Warning) << "Flushing embedded data stream!"; > - while (!embeddedQueue_.empty()) { > - unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front()); > - embeddedQueue_.pop(); > - } > - flushedBuffers = true; > - } > + /* Start with the front of the bayer queue. */ > + bayerFrame = std::move(bayerQueue_.front()); > + bayerQueue_.pop(); > > - /* > - * If the embedded queue has become empty, we cannot do any more. > - * Similarly, if we have flushed any one of our queues, we cannot do > - * any more. Return from here without a buffer pair. > - */ > - if (embeddedQueue_.empty() || flushedBuffers) > - return false; > - } else { > - /* > - * We have found a matching bayer and embedded data buffer, so > - * nothing more to do apart from assigning the bayer frame and > - * popping the buffers from the queue. > - */ > - bayerFrame = std::move(bayerQueue_.front()); > - bayerQueue_.pop(); > + /* > + * Find the embedded data buffer with a matching timestamp to pass to > + * the IPA. Any embedded buffers with a timestamp lower than the > + * current bayer buffer will be removed and re-queued to the driver. > + */ > + uint64_t ts = bayerFrame.buffer->metadata().timestamp; > + embeddedBuffer = nullptr; > + while (!embeddedQueue_.empty()) { > + FrameBuffer *b = embeddedQueue_.front(); > + if (b->metadata().timestamp < ts) { > + embeddedQueue_.pop(); > + unicam_[Unicam::Embedded].returnBuffer(b); > + LOG(RPI, Debug) << "Dropping unmatched input frame in stream " > + << unicam_[Unicam::Embedded].name(); > + } else if (b->metadata().timestamp == ts) { It would be very nice if we could switch to using sequence numbers instead of timestamps, comparing timestamps for equality always looks dangerous to me (even if I know Unicam handles this fine). This looks like a nice simplification, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + /* Found a match! */ > + embeddedBuffer = b; > embeddedQueue_.pop(); > - return true; > + break; > + } else { > + break; /* Only higher timestamps from here. */ > } > } > > - return false; > + if (!embeddedBuffer && sensorMetadata_) { > + /* Log if there is no matching embedded data buffer found. */ > + LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer."; > + } > + > + return true; > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)
Hi Laurent, On Mon, 7 Feb 2022 at 23:54, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Mon, Feb 07, 2022 at 03:12:13PM +0000, Naushir Patuck wrote: > > Simplify the image and embedded buffer matching logic by removing the > assumption > > that we require a buffer match between the two streams. Instead, if an > image > > buffer does not match with an embedded data buffer, simply use the > ControlList > > provided by DelayedControls for the sensor parameters. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 141 ++++-------------- > > 1 file changed, 31 insertions(+), 110 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 0755de84c70c..af234bd18c5b 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -2081,122 +2081,43 @@ void RPiCameraData::tryRunPipeline() > > > > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, > FrameBuffer *&embeddedBuffer) > > { > > - unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0; > > - > > - /* Loop until we find a matching bayer and embedded data buffer. */ > > - while (!bayerQueue_.empty()) { > > - /* Start with the front of the bayer queue. */ > > - FrameBuffer *bayerBuffer = bayerQueue_.front().buffer; > > - > > - /* > > - * Find the embedded data buffer with a matching timestamp > to pass to > > - * the IPA. Any embedded buffers with a timestamp lower > than the > > - * current bayer buffer will be removed and re-queued to > the driver. > > - */ > > - uint64_t ts = bayerBuffer->metadata().timestamp; > > - embeddedBuffer = nullptr; > > - while (!embeddedQueue_.empty()) { > > - FrameBuffer *b = embeddedQueue_.front(); > > - if (!unicam_[Unicam::Embedded].isExternal() && > b->metadata().timestamp < ts) { > > - embeddedQueue_.pop(); > > - unicam_[Unicam::Embedded].returnBuffer(b); > > - embeddedRequeueCount++; > > - LOG(RPI, Warning) << "Dropping unmatched > input frame in stream " > > - << > unicam_[Unicam::Embedded].name(); > > - } else if (unicam_[Unicam::Embedded].isExternal() > || b->metadata().timestamp == ts) { > > - /* We pop the item from the queue lower > down. */ > > - embeddedBuffer = b; > > - break; > > - } else { > > - break; /* Only higher timestamps from > here. */ > > - } > > - } > > - > > - if (!embeddedBuffer) { > > - bool flushedBuffers = false; > > - > > - LOG(RPI, Debug) << "Could not find matching > embedded buffer"; > > - > > - if (!sensorMetadata_) { > > - /* > > - * If there is no sensor metadata, simply > return the > > - * first bayer frame in the queue. > > - */ > > - LOG(RPI, Debug) << "Returning bayer frame > without a match"; > > - bayerFrame = > std::move(bayerQueue_.front()); > > - bayerQueue_.pop(); > > - embeddedBuffer = nullptr; > > - return true; > > - } > > - > > - if (!embeddedQueue_.empty()) { > > - /* > > - * Not found a matching embedded buffer > for the bayer buffer in > > - * the front of the queue. This buffer is > now orphaned, so requeue > > - * it back to the device. > > - */ > > - > unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); > > - bayerQueue_.pop(); > > - bayerRequeueCount++; > > - LOG(RPI, Warning) << "Dropping unmatched > input frame in stream " > > - << > unicam_[Unicam::Image].name(); > > - } > > - > > - /* > > - * If we have requeued all available embedded data > buffers in this loop, > > - * then we are fully out of sync, so might as well > requeue all the pending > > - * bayer buffers. > > - */ > > - if (embeddedRequeueCount == > unicam_[Unicam::Embedded].getBuffers().size()) { > > - /* The embedded queue must be empty at > this point! */ > > - ASSERT(embeddedQueue_.empty()); > > - > > - LOG(RPI, Warning) << "Flushing bayer > stream!"; > > - while (!bayerQueue_.empty()) { > > - > unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); > > - bayerQueue_.pop(); > > - } > > - flushedBuffers = true; > > - } > > + if (bayerQueue_.empty()) > > + return false; > > > > - /* > > - * Similar to the above, if we have requeued all > available bayer buffers in > > - * the loop, then we are fully out of sync, so > might as well requeue all the > > - * pending embedded data buffers. > > - */ > > - if (bayerRequeueCount == > unicam_[Unicam::Image].getBuffers().size()) { > > - /* The bayer queue must be empty at this > point! */ > > - ASSERT(bayerQueue_.empty()); > > - > > - LOG(RPI, Warning) << "Flushing embedded > data stream!"; > > - while (!embeddedQueue_.empty()) { > > - > unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front()); > > - embeddedQueue_.pop(); > > - } > > - flushedBuffers = true; > > - } > > + /* Start with the front of the bayer queue. */ > > + bayerFrame = std::move(bayerQueue_.front()); > > + bayerQueue_.pop(); > > > > - /* > > - * If the embedded queue has become empty, we > cannot do any more. > > - * Similarly, if we have flushed any one of our > queues, we cannot do > > - * any more. Return from here without a buffer > pair. > > - */ > > - if (embeddedQueue_.empty() || flushedBuffers) > > - return false; > > - } else { > > - /* > > - * We have found a matching bayer and embedded > data buffer, so > > - * nothing more to do apart from assigning the > bayer frame and > > - * popping the buffers from the queue. > > - */ > > - bayerFrame = std::move(bayerQueue_.front()); > > - bayerQueue_.pop(); > > + /* > > + * Find the embedded data buffer with a matching timestamp to pass > to > > + * the IPA. Any embedded buffers with a timestamp lower than the > > + * current bayer buffer will be removed and re-queued to the > driver. > > + */ > > + uint64_t ts = bayerFrame.buffer->metadata().timestamp; > > + embeddedBuffer = nullptr; > > + while (!embeddedQueue_.empty()) { > > + FrameBuffer *b = embeddedQueue_.front(); > > + if (b->metadata().timestamp < ts) { > > + embeddedQueue_.pop(); > > + unicam_[Unicam::Embedded].returnBuffer(b); > > + LOG(RPI, Debug) << "Dropping unmatched input frame > in stream " > > + << > unicam_[Unicam::Embedded].name(); > > + } else if (b->metadata().timestamp == ts) { > > It would be very nice if we could switch to using sequence numbers > instead of timestamps, comparing timestamps for equality always looks > dangerous to me (even if I know Unicam handles this fine). > I see no reason why we cannot switch to sequence numbers here. I'll test it out when I get a chance, and provide a patch on top of this one to make the change when I can. Regards, Naush > > This looks like a nice simplification, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + /* Found a match! */ > > + embeddedBuffer = b; > > embeddedQueue_.pop(); > > - return true; > > + break; > > + } else { > > + break; /* Only higher timestamps from here. */ > > } > > } > > > > - return false; > > + if (!embeddedBuffer && sensorMetadata_) { > > + /* Log if there is no matching embedded data buffer found. > */ > > + LOG(RPI, Debug) << "Returning bayer frame without a > matching embedded buffer."; > > + } > > + > > + return true; > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi) > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 0755de84c70c..af234bd18c5b 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -2081,122 +2081,43 @@ void RPiCameraData::tryRunPipeline() bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer) { - unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0; - - /* Loop until we find a matching bayer and embedded data buffer. */ - while (!bayerQueue_.empty()) { - /* Start with the front of the bayer queue. */ - FrameBuffer *bayerBuffer = bayerQueue_.front().buffer; - - /* - * Find the embedded data buffer with a matching timestamp to pass to - * the IPA. Any embedded buffers with a timestamp lower than the - * current bayer buffer will be removed and re-queued to the driver. - */ - uint64_t ts = bayerBuffer->metadata().timestamp; - embeddedBuffer = nullptr; - while (!embeddedQueue_.empty()) { - FrameBuffer *b = embeddedQueue_.front(); - if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) { - embeddedQueue_.pop(); - unicam_[Unicam::Embedded].returnBuffer(b); - embeddedRequeueCount++; - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " - << unicam_[Unicam::Embedded].name(); - } else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) { - /* We pop the item from the queue lower down. */ - embeddedBuffer = b; - break; - } else { - break; /* Only higher timestamps from here. */ - } - } - - if (!embeddedBuffer) { - bool flushedBuffers = false; - - LOG(RPI, Debug) << "Could not find matching embedded buffer"; - - if (!sensorMetadata_) { - /* - * If there is no sensor metadata, simply return the - * first bayer frame in the queue. - */ - LOG(RPI, Debug) << "Returning bayer frame without a match"; - bayerFrame = std::move(bayerQueue_.front()); - bayerQueue_.pop(); - embeddedBuffer = nullptr; - return true; - } - - if (!embeddedQueue_.empty()) { - /* - * Not found a matching embedded buffer for the bayer buffer in - * the front of the queue. This buffer is now orphaned, so requeue - * it back to the device. - */ - unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); - bayerQueue_.pop(); - bayerRequeueCount++; - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " - << unicam_[Unicam::Image].name(); - } - - /* - * If we have requeued all available embedded data buffers in this loop, - * then we are fully out of sync, so might as well requeue all the pending - * bayer buffers. - */ - if (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) { - /* The embedded queue must be empty at this point! */ - ASSERT(embeddedQueue_.empty()); - - LOG(RPI, Warning) << "Flushing bayer stream!"; - while (!bayerQueue_.empty()) { - unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer); - bayerQueue_.pop(); - } - flushedBuffers = true; - } + if (bayerQueue_.empty()) + return false; - /* - * Similar to the above, if we have requeued all available bayer buffers in - * the loop, then we are fully out of sync, so might as well requeue all the - * pending embedded data buffers. - */ - if (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) { - /* The bayer queue must be empty at this point! */ - ASSERT(bayerQueue_.empty()); - - LOG(RPI, Warning) << "Flushing embedded data stream!"; - while (!embeddedQueue_.empty()) { - unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front()); - embeddedQueue_.pop(); - } - flushedBuffers = true; - } + /* Start with the front of the bayer queue. */ + bayerFrame = std::move(bayerQueue_.front()); + bayerQueue_.pop(); - /* - * If the embedded queue has become empty, we cannot do any more. - * Similarly, if we have flushed any one of our queues, we cannot do - * any more. Return from here without a buffer pair. - */ - if (embeddedQueue_.empty() || flushedBuffers) - return false; - } else { - /* - * We have found a matching bayer and embedded data buffer, so - * nothing more to do apart from assigning the bayer frame and - * popping the buffers from the queue. - */ - bayerFrame = std::move(bayerQueue_.front()); - bayerQueue_.pop(); + /* + * Find the embedded data buffer with a matching timestamp to pass to + * the IPA. Any embedded buffers with a timestamp lower than the + * current bayer buffer will be removed and re-queued to the driver. + */ + uint64_t ts = bayerFrame.buffer->metadata().timestamp; + embeddedBuffer = nullptr; + while (!embeddedQueue_.empty()) { + FrameBuffer *b = embeddedQueue_.front(); + if (b->metadata().timestamp < ts) { + embeddedQueue_.pop(); + unicam_[Unicam::Embedded].returnBuffer(b); + LOG(RPI, Debug) << "Dropping unmatched input frame in stream " + << unicam_[Unicam::Embedded].name(); + } else if (b->metadata().timestamp == ts) { + /* Found a match! */ + embeddedBuffer = b; embeddedQueue_.pop(); - return true; + break; + } else { + break; /* Only higher timestamps from here. */ } } - return false; + if (!embeddedBuffer && sensorMetadata_) { + /* Log if there is no matching embedded data buffer found. */ + LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer."; + } + + return true; } REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)
Simplify the image and embedded buffer matching logic by removing the assumption that we require a buffer match between the two streams. Instead, if an image buffer does not match with an embedded data buffer, simply use the ControlList provided by DelayedControls for the sensor parameters. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 141 ++++-------------- 1 file changed, 31 insertions(+), 110 deletions(-)