[libcamera-devel,v3,3/5] android: camera_device: Postpone mapped streams handling

16152 diff mbox series

Mapped streams are generated by post-processing and always require a
source buffer 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@jmondi.org>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 63 ++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 8 deletions(-)

--
2.35.1

Hi Umang,

On Sun, Jun 05, 2022 at 09:25:36PM +0200, Umang Jain wrote:
> Hi Jacopo
>
> The cover letter mentions you added an assertion for
> mappedStream->sourceStream != nullptr
> but I cannot see here (or in any other patch), did you miss it by any
> chance?

You're right. With the rework of the requestedStreams handling, the
following loop where I had the assertion has gone

+       /*
+        * Collect the CameraStream associated to each requested capture stream.
+        * Since requestedStreams is an std:set<>, no duplications can happen.
+        */
+       std::set<CameraStream *> requestedStreams;
+       for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
+               CameraStream *cameraStream = buffer.stream;
+
+               switch (cameraStream->type()) {
+               case CameraStream::Type::Mapped:
+                       ASSERT(cameraStream->sourceStream() != nullptr);
+                       requestedStreams.insert(cameraStream->sourceStream());
+                       break;
+
+               case CameraStream::Type::Direct:
+               case CameraStream::Type::Internal:
+                       requestedStreams.insert(cameraStream);
+                       break;
+               }
+       }

As right now requestedStreams is not pre-populated.

>
> On 6/4/22 11:30, Jacopo Mondi wrote:
> > Mapped streams are generated by post-processing and always require a
> > source buffer 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@jmondi.org>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   src/android/camera_device.cpp | 63 ++++++++++++++++++++++++++++++-----
> >   1 file changed, 55 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 773cb3b66d48..bfe61cf715c8 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>
> > @@ -930,6 +931,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >   	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> >   			<< " with " << descriptor->buffers_.size() << " streams";
> >
> > +	/*
> > +	 * Process all the Direct and Internal streams first, they map directly
> > +	 * to a libcamera stream. Streams of type Mapped will be handled later.
> > +	 *
> > +	 * Collect the CameraStream associated to each requested capture stream.
> > +	 * Since requestedStreams is an std:set<>, no duplications can happen.
> > +	 */
> > +	std::set<CameraStream *> requestedStreams;
> >   	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> >   		CameraStream *cameraStream = buffer.stream;
> >   		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> > @@ -952,14 +961,7 @@ 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 });
> > +			/* Mapped streams will be handled in the next loop. */
> >   			continue;
> >
> >   		case CameraStream::Type::Direct:
> > @@ -1003,6 +1005,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.insert(cameraStream);
> > +	}
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	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 << ")["
> > +				<< cameraStream->configuration().pixelFormat << "]"
> > +				<< " (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();

But maybe we now want an assertion here ?


> > +		if (requestedStreams.find(sourceStream) != requestedStreams.end())
> > +			continue;
>
> Now this is looking quite straight forwards, thanks!
>
> > +
> > +		/*
> > +		 * 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);
> > +
> > +		requestedStreams.erase(sourceStream);
> >   	}
> >
> >   	/*
> > --
> > 2.35.1
> >

Hi Jacopo,

On 6/6/22 07:36, Jacopo Mondi wrote:
> Hi Umang,
>
> On Sun, Jun 05, 2022 at 09:25:36PM +0200, Umang Jain wrote:
>> Hi Jacopo
>>
>> The cover letter mentions you added an assertion for
>> mappedStream->sourceStream != nullptr
>> but I cannot see here (or in any other patch), did you miss it by any
>> chance?
> You're right. With the rework of the requestedStreams handling, the
> following loop where I had the assertion has gone
>
> +       /*
> +        * Collect the CameraStream associated to each requested capture stream.
> +        * Since requestedStreams is an std:set<>, no duplications can happen.
> +        */
> +       std::set<CameraStream *> requestedStreams;
> +       for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> +               CameraStream *cameraStream = buffer.stream;
> +
> +               switch (cameraStream->type()) {
> +               case CameraStream::Type::Mapped:
> +                       ASSERT(cameraStream->sourceStream() != nullptr);
> +                       requestedStreams.insert(cameraStream->sourceStream());
> +                       break;
> +
> +               case CameraStream::Type::Direct:
> +               case CameraStream::Type::Internal:
> +                       requestedStreams.insert(cameraStream);
> +                       break;
> +               }
> +       }
>
> As right now requestedStreams is not pre-populated.
>
>> On 6/4/22 11:30, Jacopo Mondi wrote:
>>> Mapped streams are generated by post-processing and always require a
>>> source buffer 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@jmondi.org>
>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    src/android/camera_device.cpp | 63 ++++++++++++++++++++++++++++++-----
>>>    1 file changed, 55 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 773cb3b66d48..bfe61cf715c8 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>
>>> @@ -930,6 +931,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>    	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
>>>    			<< " with " << descriptor->buffers_.size() << " streams";
>>>
>>> +	/*
>>> +	 * Process all the Direct and Internal streams first, they map directly
>>> +	 * to a libcamera stream. Streams of type Mapped will be handled later.
>>> +	 *
>>> +	 * Collect the CameraStream associated to each requested capture stream.
>>> +	 * Since requestedStreams is an std:set<>, no duplications can happen.
>>> +	 */
>>> +	std::set<CameraStream *> requestedStreams;
>>>    	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
>>>    		CameraStream *cameraStream = buffer.stream;
>>>    		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
>>> @@ -952,14 +961,7 @@ 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 });
>>> +			/* Mapped streams will be handled in the next loop. */
>>>    			continue;
>>>
>>>    		case CameraStream::Type::Direct:
>>> @@ -1003,6 +1005,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.insert(cameraStream);
>>> +	}
>>> +
>>> +	/*
>>> +	 * 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.
>>> +	 */
>>> +	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 << ")["
>>> +				<< cameraStream->configuration().pixelFormat << "]"
>>> +				<< " (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();
> But maybe we now want an assertion here ?


Yes indeed. Looks alright to me to do it here.

>
>
>>> +		if (requestedStreams.find(sourceStream) != requestedStreams.end())
>>> +			continue;
>> Now this is looking quite straight forwards, thanks!
>>
>>> +
>>> +		/*
>>> +		 * 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);
>>> +
>>> +		requestedStreams.erase(sourceStream);
>>>    	}
>>>
>>>    	/*
>>> --
>>> 2.35.1
>>>