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

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

Commit Message

Hirokazu Honda Aug. 31, 2021, 9:34 a.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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.h   |  1 +
 src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 17 deletions(-)

Comments

Laurent Pinchart Aug. 31, 2021, 8:33 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Tue, Aug 31, 2021 at 06:34:37PM +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>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.h   |  1 +
>  src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a5576927..296c2f18 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,6 +48,7 @@ public:
>  
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> +	const CameraCapabilities *capabilities() const { return &capabilities_; }
>  	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
>  
>  	const std::string &maker() const { return maker_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 01909ec7..fb10bf06 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -10,6 +10,7 @@
>  #include <sys/mman.h>
>  
>  #include "camera_buffer.h"
> +#include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
>  #include "jpeg/post_processor_jpeg.h"
> @@ -47,20 +48,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
> @@ -75,15 +62,30 @@ Stream *CameraStream::stream() const
>  
>  int CameraStream::configure()
>  {
> -	if (postProcessor_) {
> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> +		const PixelFormat outFormat =
> +			cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
>  		StreamConfiguration output = configuration();
> -		output.pixelFormat = formats::MJPEG;
> +		output.pixelFormat = outFormat;

I'd add a blank line here. Please let me know what you prefer, and I'll
handle it when pushing.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		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;
Hirokazu Honda Aug. 31, 2021, 8:45 p.m. UTC | #2 
Hi Laurent,

On Wed, Sep 1, 2021 at 5:33 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Aug 31, 2021 at 06:34:37PM +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>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.h   |  1 +
> >  src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a5576927..296c2f18 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -48,6 +48,7 @@ public:
> >
> >       unsigned int id() const { return id_; }
> >       camera3_device_t *camera3Device() { return &camera3Device_; }
> > +     const CameraCapabilities *capabilities() const { return &capabilities_; }
> >       const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> >
> >       const std::string &maker() const { return maker_; }
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 01909ec7..fb10bf06 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -10,6 +10,7 @@
> >  #include <sys/mman.h>
> >
> >  #include "camera_buffer.h"
> > +#include "camera_capabilities.h"
> >  #include "camera_device.h"
> >  #include "camera_metadata.h"
> >  #include "jpeg/post_processor_jpeg.h"
> > @@ -47,20 +48,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
> > @@ -75,15 +62,30 @@ Stream *CameraStream::stream() const
> >
> >  int CameraStream::configure()
> >  {
> > -     if (postProcessor_) {
> > +     if (type_ == Type::Internal || type_ == Type::Mapped) {
> > +             const PixelFormat outFormat =
> > +                     cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
> >               StreamConfiguration output = configuration();
> > -             output.pixelFormat = formats::MJPEG;
> > +             output.pixelFormat = outFormat;
>
> I'd add a blank line here. Please let me know what you prefer, and I'll
> handle it when pushing.
>

Fair enough.

Thanks,
-Hiro
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +             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;
>
> --
> Regards,
>
> Laurent Pinchart
Umang Jain Sept. 3, 2021, 6:46 a.m. UTC | #3 
Hi Hiro

On 8/31/21 3:04 PM, 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>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/android/camera_device.h   |  1 +
>   src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
>   2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a5576927..296c2f18 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,6 +48,7 @@ public:
>   
>   	unsigned int id() const { return id_; }
>   	camera3_device_t *camera3Device() { return &camera3Device_; }
> +	const CameraCapabilities *capabilities() const { return &capabilities_; }
>   	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
>   
>   	const std::string &maker() const { return maker_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 01909ec7..fb10bf06 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -10,6 +10,7 @@
>   #include <sys/mman.h>
>   
>   #include "camera_buffer.h"
> +#include "camera_capabilities.h"
>   #include "camera_device.h"
>   #include "camera_metadata.h"
>   #include "jpeg/post_processor_jpeg.h"
> @@ -47,20 +48,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
> @@ -75,15 +62,30 @@ Stream *CameraStream::stream() const
>   
>   int CameraStream::configure()
>   {
> -	if (postProcessor_) {
> +	if (type_ == Type::Internal || type_ == Type::Mapped) {
> +		const PixelFormat outFormat =
> +			cameraDevice_->capabilities()->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;

Patch
diff mbox series 

diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index a5576927..296c2f18 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -48,6 +48,7 @@  public:
 
 	unsigned int id() const { return id_; }
 	camera3_device_t *camera3Device() { return &camera3Device_; }
+	const CameraCapabilities *capabilities() const { return &capabilities_; }
 	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
 
 	const std::string &maker() const { return maker_; }
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 01909ec7..fb10bf06 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -10,6 +10,7 @@ 
 #include <sys/mman.h>
 
 #include "camera_buffer.h"
+#include "camera_capabilities.h"
 #include "camera_device.h"
 #include "camera_metadata.h"
 #include "jpeg/post_processor_jpeg.h"
@@ -47,20 +48,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
@@ -75,15 +62,30 @@  Stream *CameraStream::stream() const
 
 int CameraStream::configure()
 {
-	if (postProcessor_) {
+	if (type_ == Type::Internal || type_ == Type::Mapped) {
+		const PixelFormat outFormat =
+			cameraDevice_->capabilities()->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;