[libcamera-devel] [PATCH v2 3/5] android: camera_device: Postpone mapped streams handling
Jacopo Mondi
jacopo at jmondi.org
Mon May 30 16:27:05 CEST 2022
Hi Umang,
On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:
> Hi Jacopo,
>
> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:
> > Hi Paul
> >
> > On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:
> > > From: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > Mapped streams are generated by post-processing and always require a
> > > source buffer from where to process image data from.
> > >
> > > In case a Mapped stream is requested but its source stream is not, it
> > > is required to allocate a buffer on the fly and add it to the
> > > libcamera::Request.
> > >
> > > Make sure a source stream is available for all mapped streams, and if
> > > that's not the case, add a dedicated buffer to the request for that
> > > purpose.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - cosmetic changes in code
> > > - fix typo in the commit message
> > > ---
> > > src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----
> > > 1 file changed, 72 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 20599665..95c14f60 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <algorithm>
> > > #include <fstream>
> > > +#include <set>
> > > #include <sys/mman.h>
> > > #include <unistd.h>
> > > #include <vector>
> > > @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> > > << " with " << descriptor->buffers_.size() << " streams";
> > >
> > > + /*
> > > + * Collect the CameraStream associated to each requested capture stream.
> > > + * Since requestedStreams is an std:set<>, no duplications can happen.
> > std::set<>
> >
> > > + */
> > > + std::set<CameraStream *> requestedStreams;
> > > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> > > + CameraStream *cameraStream = buffer.stream;
> > > +
> > > + switch (cameraStream->type()) {
> > > + case CameraStream::Type::Mapped:
> > > + requestedStreams.insert(cameraStream->sourceStream());
>
>
> Can sourceStream field for Mapped streams be nullptr here? Should we ensure
> it via an ASSERT?
>
> > > + break;
> > > +
> > > + case CameraStream::Type::Direct:
> > > + case CameraStream::Type::Internal:
> > > + requestedStreams.insert(cameraStream);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /*
> > > + * Process all the Direct and Internal streams, for which the CameraStream
> > > + * they refer to is the one that points to the right libcamera::Stream.
> > > + *
> > > + * Streams of type Mapped will be handled later.
> > > + */
> > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> > > CameraStream *cameraStream = buffer.stream;
> > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> > > @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >
> > > switch (cameraStream->type()) {
> > > case CameraStream::Type::Mapped:
> > > - /*
> > > - * Mapped streams don't need buffers added to the
> > > - * Request.
> > > - */
> > > - LOG(HAL, Debug) << ss.str() << " (mapped)";
> > > -
> > > - descriptor->pendingStreamsToProcess_.insert(
> > > - { cameraStream, &buffer });
> > > continue;
> > >
> > > case CameraStream::Type::Direct:
> > > @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > auto fence = std::make_unique<Fence>(std::move(acquireFence));
> > > descriptor->request_->addBuffer(cameraStream->stream(),
> > > frameBuffer, std::move(fence));
> > > +
> > > + requestedStreams.erase(cameraStream);
> > > + }
> > > +
> > > + /*
> > > + * Now handle the Mapped streams. If no buffer has been added for them
> > > + * as their corresponding direct source stream has not been requested,
> > > + * add it here.
>
>
> I am wondering a situation where a direct stream D, and a mapped stream M,
> is requested and M is mapped onto D so,
>
> M->sourceStream = D;
>
> Provided the requestedStreams is a std::set<> where no duplications can
> happen, and given the above scenario:
> I see the requestedStreams will consist of { D } while populating the set
> in the switch-case above, which then gets
> erased from the requestedStreams (above the comment block) so now, the
> requestedStreams become an empty set { } here . . .
>
> > > + */
> > > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> > > + CameraStream *cameraStream = buffer.stream;
> > > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> > > +
> > > + if (cameraStream->type() != CameraStream::Type::Mapped)
> > > + continue;
> > > +
> > > + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x"
> > > + << camera3Stream->height << ")"
> > > + << "[" << utils::hex(camera3Stream->format) << "] -> "
> > > + << "(" << cameraStream->configuration().size.toString() << ")["
> > > + << cameraStream->configuration().pixelFormat.toString() << "]"
> > > + << " (mapped)";
> > > +
> > > + MutexLocker lock(descriptor->streamsProcessMutex_);
> > > + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });
> > > +
> > > + /*
> > > + * Make sure the CameraStream this stream is mapped on has been
> > > + * added to the request.
> > > + */
> > > + CameraStream *sourceStream = cameraStream->sourceStream();
> > > + if (requestedStreams.find(sourceStream) == requestedStreams.end())
>
>
> and then while handling mapped streams, it will try to find { D } in
> requestedStreams (which is now empty)
>
> > > + continue;
So that the find() operation returns == requestedStreams.end() and we
continue here
> > > +
> > > + /*
> > > + * If that's not the case, we need to add a buffer to the request
> > > + * for this stream.
> > > + */
> > > + FrameBuffer *frameBuffer = cameraStream->getBuffer();
> > > + buffer.internalBuffer = frameBuffer;
> > > +
> > > + descriptor->request_->addBuffer(sourceStream->stream(),
> > > + frameBuffer, nullptr);
>
>
> ... and add a internal buffer for D
>
> so we have 2 buffers for D in the end ?
So we don't get here and won't add the buffer twice.
Have I missed something on you comment ?
>
> > > +
> > > + requestedStreams.erase(sourceStream);
> > > }
> > We could possibily make sure here that now requestedStreams is empty,
> > but that's just an additional safety check.
> >
> > The patch still looks ok to me.
> >
> > > /*
> > > --
> > > 2.30.2
> > >
More information about the libcamera-devel
mailing list