[libcamera-devel,v4,16/21] libcamera: rkisp1: Fill stride and frameSize at config validation
diff mbox series

Message ID 20200708134417.67747-17-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Clean up formats in v4l2-compat and pipeline handlers
Related show

Commit Message

Paul Elder July 8, 2020, 1:44 p.m. UTC 
Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.
This allows applications to get the stride when trying a configuration
without modifying the active configuration of the camera.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v4:
- mention motivation in commit message
- get stride and frameSize from tryFormat on the capture video node

New in v3
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart July 8, 2020, 3:23 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Wed, Jul 08, 2020 at 10:44:12PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v4:
> - mention motivation in commit message
> - get stride and frameSize from tryFormat on the capture video node
> 
> New in v3
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c3f3f3..5ac2ecc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -115,9 +115,9 @@ public:
>  class RkISP1CameraData : public CameraData
>  {
>  public:
> -	RkISP1CameraData(PipelineHandler *pipe)
> +	RkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *video)
>  		: CameraData(pipe), sensor_(nullptr), frame_(0),
> -		  frameInfo_(pipe)
> +		  frameInfo_(pipe), video_(video)
>  	{
>  	}
>  
> @@ -135,6 +135,8 @@ public:
>  	RkISP1Frames frameInfo_;
>  	RkISP1Timeline timeline_;
>  
> +	V4L2VideoDevice *video_;
> +

I don't think this is necessary, you can already access the pipe with

	PipelineHandlerRkISP1 *pipe =
		static_cast<PipelineHandlerRkISP1 *>(data->pipe_);

and use pipe->video_ instead of data->video_.

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

>  private:
>  	void queueFrameAction(unsigned int frame,
>  			      const IPAOperationData &action);
> @@ -535,6 +537,17 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  
>  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>  
> +	V4L2DeviceFormat format = {};
> +	format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> +	format.size = cfg.size;
> +
> +	int ret = data_->video_->tryFormat(&format);
> +	if (ret)
> +		return Invalid;
> +
> +	cfg.stride = format.planes[0].bpl;
> +	cfg.frameSize = format.planes[0].size;
> +
>  	return status;
>  }
>  
> @@ -683,7 +696,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  
>  	cfg.setStream(&data->stream_);
> -	cfg.stride = outputFormat.planes[0].bpl;
>  
>  	return 0;
>  }
> @@ -934,7 +946,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	int ret;
>  
>  	std::unique_ptr<RkISP1CameraData> data =
> -		std::make_unique<RkISP1CameraData>(this);
> +		std::make_unique<RkISP1CameraData>(this, video_);
>  
>  	ControlInfoMap::Map ctrls;
>  	ctrls.emplace(std::piecewise_construct,
Paul Elder July 9, 2020, 8:18 a.m. UTC | #2 
Hi Laurent,

Thank you for the review.

On Wed, Jul 08, 2020 at 06:23:18PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Wed, Jul 08, 2020 at 10:44:12PM +0900, Paul Elder wrote:
> > Fill the stride and frameSize fields of the StreamConfiguration at
> > configuration validation time instead of at camera configuration time.
> > This allows applications to get the stride when trying a configuration
> > without modifying the active configuration of the camera.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v4:
> > - mention motivation in commit message
> > - get stride and frameSize from tryFormat on the capture video node
> > 
> > New in v3
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 3c3f3f3..5ac2ecc 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -115,9 +115,9 @@ public:
> >  class RkISP1CameraData : public CameraData
> >  {
> >  public:
> > -	RkISP1CameraData(PipelineHandler *pipe)
> > +	RkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *video)
> >  		: CameraData(pipe), sensor_(nullptr), frame_(0),
> > -		  frameInfo_(pipe)
> > +		  frameInfo_(pipe), video_(video)
> >  	{
> >  	}
> >  
> > @@ -135,6 +135,8 @@ public:
> >  	RkISP1Frames frameInfo_;
> >  	RkISP1Timeline timeline_;
> >  
> > +	V4L2VideoDevice *video_;
> > +
> 
> I don't think this is necessary, you can already access the pipe with
> 
> 	PipelineHandlerRkISP1 *pipe =
> 		static_cast<PipelineHandlerRkISP1 *>(data->pipe_);
> 
> and use pipe->video_ instead of data->video_.

video_ is private in PipelineHandlerRkISP1 :/

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  private:
> >  	void queueFrameAction(unsigned int frame,
> >  			      const IPAOperationData &action);
> > @@ -535,6 +537,17 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  
> >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >  
> > +	V4L2DeviceFormat format = {};
> > +	format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> > +	format.size = cfg.size;
> > +
> > +	int ret = data_->video_->tryFormat(&format);
> > +	if (ret)
> > +		return Invalid;
> > +
> > +	cfg.stride = format.planes[0].bpl;
> > +	cfg.frameSize = format.planes[0].size;
> > +
> >  	return status;
> >  }
> >  
> > @@ -683,7 +696,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  		return ret;
> >  
> >  	cfg.setStream(&data->stream_);
> > -	cfg.stride = outputFormat.planes[0].bpl;
> >  
> >  	return 0;
> >  }
> > @@ -934,7 +946,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >  	int ret;
> >  
> >  	std::unique_ptr<RkISP1CameraData> data =
> > -		std::make_unique<RkISP1CameraData>(this);
> > +		std::make_unique<RkISP1CameraData>(this, video_);
> >  
> >  	ControlInfoMap::Map ctrls;
> >  	ctrls.emplace(std::piecewise_construct,


Thanks,

Paul

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 3c3f3f3..5ac2ecc 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -115,9 +115,9 @@  public:
 class RkISP1CameraData : public CameraData
 {
 public:
-	RkISP1CameraData(PipelineHandler *pipe)
+	RkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *video)
 		: CameraData(pipe), sensor_(nullptr), frame_(0),
-		  frameInfo_(pipe)
+		  frameInfo_(pipe), video_(video)
 	{
 	}
 
@@ -135,6 +135,8 @@  public:
 	RkISP1Frames frameInfo_;
 	RkISP1Timeline timeline_;
 
+	V4L2VideoDevice *video_;
+
 private:
 	void queueFrameAction(unsigned int frame,
 			      const IPAOperationData &action);
@@ -535,6 +537,17 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 
 	cfg.bufferCount = RKISP1_BUFFER_COUNT;
 
+	V4L2DeviceFormat format = {};
+	format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
+	format.size = cfg.size;
+
+	int ret = data_->video_->tryFormat(&format);
+	if (ret)
+		return Invalid;
+
+	cfg.stride = format.planes[0].bpl;
+	cfg.frameSize = format.planes[0].size;
+
 	return status;
 }
 
@@ -683,7 +696,6 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 		return ret;
 
 	cfg.setStream(&data->stream_);
-	cfg.stride = outputFormat.planes[0].bpl;
 
 	return 0;
 }
@@ -934,7 +946,7 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	int ret;
 
 	std::unique_ptr<RkISP1CameraData> data =
-		std::make_unique<RkISP1CameraData>(this);
+		std::make_unique<RkISP1CameraData>(this, video_);
 
 	ControlInfoMap::Map ctrls;
 	ctrls.emplace(std::piecewise_construct,