[libcamera-devel] [PATCH v2 3/5] android: camera_device: Postpone mapped streams handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 2 10:35:30 CEST 2022
On Thu, Jun 02, 2022 at 09:32:13AM +0200, Umang Jain via libcamera-devel wrote:
> On 5/30/22 21:58, Umang Jain via libcamera-devel wrote:
> > On 5/30/22 16:27, Jacopo Mondi wrote:
> >> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:
> >>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:
> >>>> 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.
s/from where //
> >>>>>
> >>>>> 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?
> >
> > Would like your thoughts on this as well..
>
> Probably we should merge in with the ASSERT(cameraStream->sourceStream()
> != nullptr) as a safety check.
> That gives me a bit of mental peace:
It shouldn't be null, so an assert is enough if it helps you sleeping
:-)
> With that,
>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>
> >>>>> + 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.
I have trouble parsing this, what is "the right libcamera::Stream" ? Do
you mean something along the lines of
/*
* Process all the Direct and Internal streams first, they map directly
* to a 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 });
Let's keep a comment:
/* Mapped streams will be handled in the next loop. */
> >>>>> 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.
* Now handle the Mapped streams. If no buffer has been added for them,
* because their corresponding direct source stream is not part of this
* particular request, add one 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() << "]"
We can now drop the .toString() :-)
> >>>>> + << " (mapped)";
> >>>>> +
> >>>>> + MutexLocker lock(descriptor->streamsProcessMutex_);
> >>>>> + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });
> >>>>> +
> >>>>> + /*
> >>>>> + * Make sure the CameraStream this stream is mapped on has been
s/mapped on/mapped to/
> >>>>> + * 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
> >
> > Ah .. so silly of me thinking the comparator as != requestedStreams.end()
> > instead of == requestedStreams.end(). So code patterns do stick in the
> > brain :D
Part of the confusion may come from the variable name. It contains the
streams that haven't been added to the request yet. How about renaming
it to unhandledStreams ?
Thinking more about it, couldn't we simplify the code by inverting the
logic ? It seems we could drop the first loop that populates
requestedStreams, and instead add streams to requestedStreams in the
loop that handles the Direct and Internal streams. The check here would
then become != requestedStreams.end(), and below the erase() would be
turned into an insert().
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >> 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.
> >
> > Makes sense now
> >
> >> Have I missed something on you comment ?
> >
> > No. It looks good to me now!
> >
> >>>>> +
> >>>>> + 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.
> >>>>
> >>>>> /*
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list