[libcamera-devel,v3,1/2] android: cameraDevice: Factorize the code of validating camera3_capture_request

11823 diff mbox series

CameraDevice::processCaptureRequest() checks the validness of a
provided camera3_capture_request. This factorizes the code in
order to add more validation to the request later.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_device.cpp | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Hi Hiro,

Thank you for the patch.

On Fri, Apr 02, 2021 at 11:44:51AM +0900, Hirokazu Honda wrote:
> CameraDevice::processCaptureRequest() checks the validness of a
> provided camera3_capture_request. This factorizes the code in
> order to add more validation to the request later.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

I think we should go one step further on top of this though. Instead of
splitting request validation to a separate function, we could move it to
the Camera3RequestDescriptor class. Validation would happen in the
constructor, and a new isValid() function would then allow
CameraDevice::processControls() to check if the request is valid. I'd
then turn Camera3RequestDescriptor into a class (it's a struct), and
move it out of CameraDevice to a camera_request.cpp file.

> ---
>  src/android/camera_device.cpp | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index eb327978..988c1fd5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -256,6 +256,21 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
>  	unsortedConfigs = sortedConfigs;
>  }
>  
> +bool isValidRequest(camera3_capture_request_t *camera3Request)
> +{
> +	if (!camera3Request) {
> +		LOG(HAL, Error) << "No capture request provided";
> +		return false;
> +	}
> +
> +	if (!camera3Request->num_output_buffers) {
> +		LOG(HAL, Error) << "No output buffers provided";
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  } /* namespace */
>  
>  /*
> @@ -1785,15 +1800,8 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
> -	if (!camera3Request) {
> -		LOG(HAL, Error) << "No capture request provided";
> +	if (isValidRequest(camera3Request))
>  		return -EINVAL;
> -	}
> -
> -	if (!camera3Request->num_output_buffers) {
> -		LOG(HAL, Error) << "No output buffers provided";
> -		return -EINVAL;
> -	}
>  
>  	/* Start the camera if that's the first request we handle. */
>  	if (!running_) {

Hi Laurent,

On Sat, Apr 3, 2021 at 8:38 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 11:44:51AM +0900, Hirokazu Honda wrote:
> > CameraDevice::processCaptureRequest() checks the validness of a
> > provided camera3_capture_request. This factorizes the code in
> > order to add more validation to the request later.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I think we should go one step further on top of this though. Instead of
> splitting request validation to a separate function, we could move it to
> the Camera3RequestDescriptor class. Validation would happen in the
> constructor, and a new isValid() function would then allow
> CameraDevice::processControls() to check if the request is valid. I'd
> then turn Camera3RequestDescriptor into a class (it's a struct), and
> move it out of CameraDevice to a camera_request.cpp file.
>

That sounds good to me.
I am going to do another patch after this and "leakage" patch are landed.
-Hiro

> > ---
> >  src/android/camera_device.cpp | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index eb327978..988c1fd5 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -256,6 +256,21 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
> >       unsortedConfigs = sortedConfigs;
> >  }
> >
> > +bool isValidRequest(camera3_capture_request_t *camera3Request)
> > +{
> > +     if (!camera3Request) {
> > +             LOG(HAL, Error) << "No capture request provided";
> > +             return false;
> > +     }
> > +
> > +     if (!camera3Request->num_output_buffers) {
> > +             LOG(HAL, Error) << "No output buffers provided";
> > +             return false;
> > +     }
> > +
> > +     return true;
> > +}
> > +
> >  } /* namespace */
> >
> >  /*
> > @@ -1785,15 +1800,8 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> >  {
> > -     if (!camera3Request) {
> > -             LOG(HAL, Error) << "No capture request provided";
> > +     if (isValidRequest(camera3Request))
> >               return -EINVAL;
> > -     }
> > -
> > -     if (!camera3Request->num_output_buffers) {
> > -             LOG(HAL, Error) << "No output buffers provided";
> > -             return -EINVAL;
> > -     }
> >
> >       /* Start the camera if that's the first request we handle. */
> >       if (!running_) {
>
> --
> Regards,
>
> Laurent Pinchart