[v2,3/9] android: Set StreamBuffer::srcBuffer in CameraDevice::processCaptureRequest
diff mbox series

Message ID 20241127092632.3145984-4-chenghaoyang@chromium.org
State New
Headers show
Series
  • Signal metadataAvailable and Android partial result
Related show

Commit Message

Cheng-Hao Yang Nov. 27, 2024, 9:25 a.m. UTC 
StreamBuffer::srcBuffer was set right before being processed, while it
was already determined when being constructed. This patch sets the value
in CameraDevice::processCaptureRequest.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/camera_device.cpp | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi Nov. 28, 2024, 1:13 p.m. UTC | #1 
Hi Harvey

On Wed, Nov 27, 2024 at 09:25:53AM +0000, Harvey Yang wrote:
> StreamBuffer::srcBuffer was set right before being processed, while it
> was already determined when being constructed. This patch sets the value
> in CameraDevice::processCaptureRequest.

Could you explain in the commit message why is needed/better to set
srcBuffer ealier ? And also mention why making requestedStreams is
required ?

>
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/android/camera_device.cpp | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4e3bdc9cc..11613fac9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -967,9 +967,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * 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.
> +	 * Since requestedStreams is an std:map<>, no duplications can happen.

So you can now remove #include <set> ?

>  	 */
> -	std::set<CameraStream *> requestedStreams;
> +	std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
>  	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
>  		CameraStream *cameraStream = buffer.stream;
>  		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> @@ -1008,6 +1008,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  						  cameraStream->configuration().size);
>  			frameBuffer = buffer.frameBuffer.get();
>  			acquireFence = std::move(buffer.fence);
> +
> +			requestedBuffers[cameraStream] = frameBuffer;
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>
> @@ -1021,6 +1023,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 */
>  			frameBuffer = cameraStream->getBuffer();
>  			buffer.internalBuffer = frameBuffer;
> +			buffer.srcBuffer = frameBuffer;
> +
> +			requestedBuffers[cameraStream] = frameBuffer;
>  			LOG(HAL, Debug) << ss.str() << " (internal)";
>
>  			descriptor->pendingStreamsToProcess_.insert(
> @@ -1036,8 +1041,6 @@ 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);

Why can't you
                requestedBuffers[cameraStream] = frameBuffer;

here ?

I feel like it should be done after

		if (!frameBuffer) {
			LOG(HAL, Error) << "Failed to create frame buffer";
			return -ENOMEM;
		}
>  	}
>
>  	/*
> @@ -1068,8 +1071,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		 */
>  		CameraStream *sourceStream = cameraStream->sourceStream();
>  		ASSERT(sourceStream);
> -		if (requestedStreams.find(sourceStream) != requestedStreams.end())
> +		ASSERT(sourceStream->type() == CameraStream::Type::Direct);

If you want to add this assertion, you can do it in one line

		ASSERT(sourceStream && sourceStream->type() == CameraStream::Type::Direct);

> +
> +		/*
> +		 * If the buffer for the source stream has been requested as
> +		 * Direct, use its framebuffer as the source buffer for
> +		 * post-processing. No need to recycle the buffer since it's
> +		 * owned by Android.

What do you mean by "recycle the buffer" ?

> +		 */
> +		auto iterDirectBuffer = requestedBuffers.find(sourceStream);
> +		if (iterDirectBuffer != requestedBuffers.end()) {
> +			buffer.srcBuffer = iterDirectBuffer->second;
>  			continue;
> +		}
>
>  		/*
>  		 * If that's not the case, we need to add a buffer to the request
> @@ -1077,11 +1091,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		 */
>  		FrameBuffer *frameBuffer = cameraStream->getBuffer();
>  		buffer.internalBuffer = frameBuffer;
> +		buffer.srcBuffer = frameBuffer;
>
>  		descriptor->request_->addBuffer(sourceStream->stream(),
>  						frameBuffer, nullptr);
>
> -		requestedStreams.insert(sourceStream);
> +		requestedBuffers[sourceStream] = frameBuffer;
>  	}
>
>  	/*
> @@ -1236,8 +1251,6 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>
> -		buffer->srcBuffer = src;
> -
>  		++iter;
>  		int ret = stream->process(buffer);
>  		if (ret) {
> --
> 2.47.0.338.g60cca15819-goog
>
Cheng-Hao Yang Nov. 29, 2024, 8:46 a.m. UTC | #2 
Hi Jacopo,

Will upload another version later.

On Thu, Nov 28, 2024 at 9:14 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Wed, Nov 27, 2024 at 09:25:53AM +0000, Harvey Yang wrote:
> > StreamBuffer::srcBuffer was set right before being processed, while it
> > was already determined when being constructed. This patch sets the value
> > in CameraDevice::processCaptureRequest.
>
> Could you explain in the commit message why is needed/better to set
> srcBuffer ealier ? And also mention why making requestedStreams is
> required ?

Updated. Please take another look.

>
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 4e3bdc9cc..11613fac9 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -967,9 +967,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >        * 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.
> > +      * Since requestedStreams is an std:map<>, no duplications can happen.
>
> So you can now remove #include <set> ?

Done

>
> >        */
> > -     std::set<CameraStream *> requestedStreams;
> > +     std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
> >       for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> >               CameraStream *cameraStream = buffer.stream;
> >               camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> > @@ -1008,6 +1008,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                                                 cameraStream->configuration().size);
> >                       frameBuffer = buffer.frameBuffer.get();
> >                       acquireFence = std::move(buffer.fence);
> > +
> > +                     requestedBuffers[cameraStream] = frameBuffer;
> >                       LOG(HAL, Debug) << ss.str() << " (direct)";
> >                       break;
> >
> > @@ -1021,6 +1023,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                        */
> >                       frameBuffer = cameraStream->getBuffer();
> >                       buffer.internalBuffer = frameBuffer;
> > +                     buffer.srcBuffer = frameBuffer;
> > +
> > +                     requestedBuffers[cameraStream] = frameBuffer;
> >                       LOG(HAL, Debug) << ss.str() << " (internal)";
> >
> >                       descriptor->pendingStreamsToProcess_.insert(
> > @@ -1036,8 +1041,6 @@ 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);
>
> Why can't you
>                 requestedBuffers[cameraStream] = frameBuffer;
>
> here ?
>
> I feel like it should be done after
>
>                 if (!frameBuffer) {
>                         LOG(HAL, Error) << "Failed to create frame buffer";
>                         return -ENOMEM;
>                 }

True, done.

> >       }
> >
> >       /*
> > @@ -1068,8 +1071,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                */
> >               CameraStream *sourceStream = cameraStream->sourceStream();
> >               ASSERT(sourceStream);
> > -             if (requestedStreams.find(sourceStream) != requestedStreams.end())
> > +             ASSERT(sourceStream->type() == CameraStream::Type::Direct);
>
> If you want to add this assertion, you can do it in one line
>
>                 ASSERT(sourceStream && sourceStream->type() == CameraStream::Type::Direct);
>

Done

> > +
> > +             /*
> > +              * If the buffer for the source stream has been requested as
> > +              * Direct, use its framebuffer as the source buffer for
> > +              * post-processing. No need to recycle the buffer since it's
> > +              * owned by Android.
>
> What do you mean by "recycle the buffer" ?

It's a comment originally written by Han-lin. I think he meant
that it's not an internal buffer that we need to set and recycle
to CameraStream.

Might be a bit redundant though. Do you think we should
remove it?

BR,
Harvey

>
> > +              */
> > +             auto iterDirectBuffer = requestedBuffers.find(sourceStream);
> > +             if (iterDirectBuffer != requestedBuffers.end()) {
> > +                     buffer.srcBuffer = iterDirectBuffer->second;
> >                       continue;
> > +             }
> >
> >               /*
> >                * If that's not the case, we need to add a buffer to the request
> > @@ -1077,11 +1091,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                */
> >               FrameBuffer *frameBuffer = cameraStream->getBuffer();
> >               buffer.internalBuffer = frameBuffer;
> > +             buffer.srcBuffer = frameBuffer;
> >
> >               descriptor->request_->addBuffer(sourceStream->stream(),
> >                                               frameBuffer, nullptr);
> >
> > -             requestedStreams.insert(sourceStream);
> > +             requestedBuffers[sourceStream] = frameBuffer;
> >       }
> >
> >       /*
> > @@ -1236,8 +1251,6 @@ void CameraDevice::requestComplete(Request *request)
> >                       continue;
> >               }
> >
> > -             buffer->srcBuffer = src;
> > -
> >               ++iter;
> >               int ret = stream->process(buffer);
> >               if (ret) {
> > --
> > 2.47.0.338.g60cca15819-goog
> >

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 4e3bdc9cc..11613fac9 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -967,9 +967,9 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	 * 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.
+	 * Since requestedStreams is an std:map<>, no duplications can happen.
 	 */
-	std::set<CameraStream *> requestedStreams;
+	std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
 	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
 		CameraStream *cameraStream = buffer.stream;
 		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
@@ -1008,6 +1008,8 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 						  cameraStream->configuration().size);
 			frameBuffer = buffer.frameBuffer.get();
 			acquireFence = std::move(buffer.fence);
+
+			requestedBuffers[cameraStream] = frameBuffer;
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
@@ -1021,6 +1023,9 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			 */
 			frameBuffer = cameraStream->getBuffer();
 			buffer.internalBuffer = frameBuffer;
+			buffer.srcBuffer = frameBuffer;
+
+			requestedBuffers[cameraStream] = frameBuffer;
 			LOG(HAL, Debug) << ss.str() << " (internal)";
 
 			descriptor->pendingStreamsToProcess_.insert(
@@ -1036,8 +1041,6 @@  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);
 	}
 
 	/*
@@ -1068,8 +1071,19 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		 */
 		CameraStream *sourceStream = cameraStream->sourceStream();
 		ASSERT(sourceStream);
-		if (requestedStreams.find(sourceStream) != requestedStreams.end())
+		ASSERT(sourceStream->type() == CameraStream::Type::Direct);
+
+		/*
+		 * If the buffer for the source stream has been requested as
+		 * Direct, use its framebuffer as the source buffer for
+		 * post-processing. No need to recycle the buffer since it's
+		 * owned by Android.
+		 */
+		auto iterDirectBuffer = requestedBuffers.find(sourceStream);
+		if (iterDirectBuffer != requestedBuffers.end()) {
+			buffer.srcBuffer = iterDirectBuffer->second;
 			continue;
+		}
 
 		/*
 		 * If that's not the case, we need to add a buffer to the request
@@ -1077,11 +1091,12 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		 */
 		FrameBuffer *frameBuffer = cameraStream->getBuffer();
 		buffer.internalBuffer = frameBuffer;
+		buffer.srcBuffer = frameBuffer;
 
 		descriptor->request_->addBuffer(sourceStream->stream(),
 						frameBuffer, nullptr);
 
-		requestedStreams.insert(sourceStream);
+		requestedBuffers[sourceStream] = frameBuffer;
 	}
 
 	/*
@@ -1236,8 +1251,6 @@  void CameraDevice::requestComplete(Request *request)
 			continue;
 		}
 
-		buffer->srcBuffer = src;
-
 		++iter;
 		int ret = stream->process(buffer);
 		if (ret) {