[libcamera-devel] [PATCH v2 3/5] android: camera_device: Postpone mapped streams handling

[Umang Jain]

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..

>>
>>>> +			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
>>>>