[libcamera-devel,1/3] android: camera_stream: Create post porcessor in configure()
diff mbox series

Message ID 20210805134530.825065-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • android: Request one stream for identica stream requests
Related show

Commit Message

Hirokazu Honda Aug. 5, 2021, 1:45 p.m. UTC 
CameraStream creates PostProcessor and FrameBufferAllocator in
the constructor. CameraStream assumes that a used post processor
is JPEG post processor. Since we need to support various post
processors, we would rather move the creation to configure() so
as to return an error code if no proper post processor is found.
This also moves FrameBufferAllocator and Mutex creation for
consistency.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.h   |  2 ++
 src/android/camera_stream.cpp | 35 ++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 17 deletions(-)

Comments

Jacopo Mondi Aug. 18, 2021, 10:56 a.m. UTC | #1 
Hi Hiro,

On Thu, Aug 05, 2021 at 10:45:28PM +0900, Hirokazu Honda wrote:
> CameraStream creates PostProcessor and FrameBufferAllocator in
> the constructor. CameraStream assumes that a used post processor
> is JPEG post processor. Since we need to support various post
> processors, we would rather move the creation to configure() so
> as to return an error code if no proper post processor is found.
> This also moves FrameBufferAllocator and Mutex creation for
> consistency.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.h   |  2 ++
>  src/android/camera_stream.cpp | 35 ++++++++++++++++++-----------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 089a6204..cbc71be4 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -63,6 +63,8 @@ public:
>  	int processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request);
>
> +	libcamera::PixelFormat toPixelFormat(int format) const;
> +

Why I do see this function part of the CameraCapabilities class and
not of CameraDevice ? Is this patch missing the
CameraDevice::toPixelFormat() implementation ?

>  protected:
>  	std::string logPrefix() const override;
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index bf4a7b41..86263403 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -45,20 +45,6 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>  	: cameraDevice_(cameraDevice), config_(config), type_(type),
>  	  camera3Stream_(camera3Stream), index_(index)
>  {
> -	if (type_ == Type::Internal || type_ == Type::Mapped) {
> -		/*
> -		 * \todo There might be multiple post-processors. The logic
> -		 * which should be instantiated here, is deferred for the
> -		 * future. For now, we only have PostProcessorJpeg and that
> -		 * is what we instantiate here.
> -		 */
> -		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> -	}
> -
> -	if (type == Type::Internal) {
> -		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> -		mutex_ = std::make_unique<std::mutex>();
> -	}
>  }
>
>  const StreamConfiguration &CameraStream::configuration() const
> @@ -73,15 +59,30 @@ Stream *CameraStream::stream() const
>
>  int CameraStream::configure()
>  {
> -	if (postProcessor_) {
> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> +		const PixelFormat outFormat =
> +			cameraDevice_->toPixelFormat(camera3Stream_->format);
>  		StreamConfiguration output = configuration();
> -		output.pixelFormat = formats::MJPEG;
> +		output.pixelFormat = outFormat;
> +		switch (outFormat) {
> +		case formats::MJPEG:
> +			postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> +			break;
> +
> +		default:
> +			LOG(HAL, Error) << "Unsupported format: " << outFormat;
> +			return -EINVAL;
> +		}
> +
>  		int ret = postProcessor_->configure(configuration(), output);
>  		if (ret)
>  			return ret;
>  	}
>
> -	if (allocator_) {
> +	if (type_ == Type::Internal) {
> +		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> +		mutex_ = std::make_unique<std::mutex>();
> +

We should probably
        ASSERT(type_ == Type::Internal)
in CameraStream::getBuffer() and putBuffer() and remove the
if (!allocator) check, but that's not required for this series

Thanks
   j
>  		int ret = allocator_->allocate(stream());
>  		if (ret < 0)
>  			return ret;
> --
> 2.32.0.554.ge1b32706d8-goog
>
Hirokazu Honda Aug. 19, 2021, 5:40 p.m. UTC | #2 
Hi Jacopo,

On Wed, Aug 18, 2021 at 7:55 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
>
> On Thu, Aug 05, 2021 at 10:45:28PM +0900, Hirokazu Honda wrote:
> > CameraStream creates PostProcessor and FrameBufferAllocator in
> > the constructor. CameraStream assumes that a used post processor
> > is JPEG post processor. Since we need to support various post
> > processors, we would rather move the creation to configure() so
> > as to return an error code if no proper post processor is found.
> > This also moves FrameBufferAllocator and Mutex creation for
> > consistency.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.h   |  2 ++
> >  src/android/camera_stream.cpp | 35 ++++++++++++++++++-----------------
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 089a6204..cbc71be4 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -63,6 +63,8 @@ public:
> >       int processCaptureRequest(camera3_capture_request_t *request);
> >       void requestComplete(libcamera::Request *request);
> >
> > +     libcamera::PixelFormat toPixelFormat(int format) const;
> > +
>
> Why I do see this function part of the CameraCapabilities class and
> not of CameraDevice ? Is this patch missing the
> CameraDevice::toPixelFormat() implementation ?
>

Hm, I somehow reverted the change upon uploading unintendedly. Fixed.

> >  protected:
> >       std::string logPrefix() const override;
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index bf4a7b41..86263403 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -45,20 +45,6 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> >       : cameraDevice_(cameraDevice), config_(config), type_(type),
> >         camera3Stream_(camera3Stream), index_(index)
> >  {
> > -     if (type_ == Type::Internal || type_ == Type::Mapped) {
> > -             /*
> > -              * \todo There might be multiple post-processors. The logic
> > -              * which should be instantiated here, is deferred for the
> > -              * future. For now, we only have PostProcessorJpeg and that
> > -              * is what we instantiate here.
> > -              */
> > -             postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > -     }
> > -
> > -     if (type == Type::Internal) {
> > -             allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> > -             mutex_ = std::make_unique<std::mutex>();
> > -     }
> >  }
> >
> >  const StreamConfiguration &CameraStream::configuration() const
> > @@ -73,15 +59,30 @@ Stream *CameraStream::stream() const
> >
> >  int CameraStream::configure()
> >  {
> > -     if (postProcessor_) {
> > +     if (type_ == Type::Internal || type_ == Type::Mapped) {
> > +             const PixelFormat outFormat =
> > +                     cameraDevice_->toPixelFormat(camera3Stream_->format);
> >               StreamConfiguration output = configuration();
> > -             output.pixelFormat = formats::MJPEG;
> > +             output.pixelFormat = outFormat;
> > +             switch (outFormat) {
> > +             case formats::MJPEG:
> > +                     postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > +                     break;
> > +
> > +             default:
> > +                     LOG(HAL, Error) << "Unsupported format: " << outFormat;
> > +                     return -EINVAL;
> > +             }
> > +
> >               int ret = postProcessor_->configure(configuration(), output);
> >               if (ret)
> >                       return ret;
> >       }
> >
> > -     if (allocator_) {
> > +     if (type_ == Type::Internal) {
> > +             allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> > +             mutex_ = std::make_unique<std::mutex>();
> > +
>
> We should probably
>         ASSERT(type_ == Type::Internal)
> in CameraStream::getBuffer() and putBuffer() and remove the
> if (!allocator) check, but that's not required for this series
>

Thanks for the comment. I will do after this series are checked in.

Best,
-Hiro
> Thanks
>    j
> >               int ret = allocator_->allocate(stream());
> >               if (ret < 0)
> >                       return ret;
> > --
> > 2.32.0.554.ge1b32706d8-goog
> >

Patch
diff mbox series 

diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 089a6204..cbc71be4 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -63,6 +63,8 @@  public:
 	int processCaptureRequest(camera3_capture_request_t *request);
 	void requestComplete(libcamera::Request *request);
 
+	libcamera::PixelFormat toPixelFormat(int format) const;
+
 protected:
 	std::string logPrefix() const override;
 
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index bf4a7b41..86263403 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -45,20 +45,6 @@  CameraStream::CameraStream(CameraDevice *const cameraDevice,
 	: cameraDevice_(cameraDevice), config_(config), type_(type),
 	  camera3Stream_(camera3Stream), index_(index)
 {
-	if (type_ == Type::Internal || type_ == Type::Mapped) {
-		/*
-		 * \todo There might be multiple post-processors. The logic
-		 * which should be instantiated here, is deferred for the
-		 * future. For now, we only have PostProcessorJpeg and that
-		 * is what we instantiate here.
-		 */
-		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
-	}
-
-	if (type == Type::Internal) {
-		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
-		mutex_ = std::make_unique<std::mutex>();
-	}
 }
 
 const StreamConfiguration &CameraStream::configuration() const
@@ -73,15 +59,30 @@  Stream *CameraStream::stream() const
 
 int CameraStream::configure()
 {
-	if (postProcessor_) {
+	if (type_ == Type::Internal || type_ == Type::Mapped) {
+		const PixelFormat outFormat =
+			cameraDevice_->toPixelFormat(camera3Stream_->format);
 		StreamConfiguration output = configuration();
-		output.pixelFormat = formats::MJPEG;
+		output.pixelFormat = outFormat;
+		switch (outFormat) {
+		case formats::MJPEG:
+			postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
+			break;
+
+		default:
+			LOG(HAL, Error) << "Unsupported format: " << outFormat;
+			return -EINVAL;
+		}
+
 		int ret = postProcessor_->configure(configuration(), output);
 		if (ret)
 			return ret;
 	}
 
-	if (allocator_) {
+	if (type_ == Type::Internal) {
+		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
+		mutex_ = std::make_unique<std::mutex>();
+
 		int ret = allocator_->allocate(stream());
 		if (ret < 0)
 			return ret;