[libcamera-devel] [PATCH v2 3/5] android: camera_device: Postpone mapped streams handling
Umang Jain
umang.jain at ideasonboard.com
Thu Jun 2 09:32:13 CEST 2022
Hi Jacopo,
Just to re-iterate on point,
On 5/30/22 21:58, Umang Jain via libcamera-devel wrote:
> Hi Jacopo,
>
> On 5/30/22 16:27, Jacopo Mondi wrote:
>> 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?
>
>
> 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:
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.
>>>>> + *
>>>>> + * 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
>
>
> Ah .. so silly of me thinking the comparator as != requestedStreams.end()
> instead of == requestedStreams.end(). So code patterns do stick in the
> brain :D
>
>> 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.
>>>>
>>>>> /*
>>>>> --
>>>>> 2.30.2
>>>>>
More information about the libcamera-devel
mailing list