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

Message ID 20210401153123.1217170-1-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,1/2] android: cameraDevice: Factorize the code of validating camera3_capture_request
Related show

Commit Message

Hirokazu Honda April 1, 2021, 3:31 p.m. UTC 
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>
---
 src/android/camera_device.cpp | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi April 1, 2021, 4:05 p.m. UTC | #1 
Hi Hiro,
On Fri, Apr 02, 2021 at 12:31:22AM +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>
> ---
>  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..48eb471d 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 -EINVAL;

return false ?

With this fixed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +	}
> +
> +	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";
> -		return -EINVAL;
> -	}
> -
> -	if (!camera3Request->num_output_buffers) {
> -		LOG(HAL, Error) << "No output buffers provided";
> +	if (isValidRequest(camera3Request))
>  		return -EINVAL;
> -	}
>
>  	/* Start the camera if that's the first request we handle. */
>  	if (!running_) {
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda April 1, 2021, 4:11 p.m. UTC | #2 
On Fri, Apr 2, 2021 at 1:04 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
> On Fri, Apr 02, 2021 at 12:31:22AM +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>
> > ---
> >  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..48eb471d 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 -EINVAL;
>
> return false ?
>

definitely, yes.
Thanks for catching!

> With this fixed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
> > +     }
> > +
> > +     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";
> > -             return -EINVAL;
> > -     }
> > -
> > -     if (!camera3Request->num_output_buffers) {
> > -             LOG(HAL, Error) << "No output buffers provided";
> > +     if (isValidRequest(camera3Request))
> >               return -EINVAL;
> > -     }
> >
> >       /* Start the camera if that's the first request we handle. */
> >       if (!running_) {
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham April 1, 2021, 10:28 p.m. UTC | #3 
On 01/04/2021 17:05, Jacopo Mondi wrote:
> Hi Hiro,
> On Fri, Apr 02, 2021 at 12:31:22AM +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>
>> ---
>>  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..48eb471d 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 -EINVAL;
> 
> return false ?
> 
> With this fixed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 

With that,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Thanks
>    j
> 
>> +	}
>> +
>> +	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";
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (!camera3Request->num_output_buffers) {
>> -		LOG(HAL, Error) << "No output buffers provided";
>> +	if (isValidRequest(camera3Request))
>>  		return -EINVAL;
>> -	}
>>
>>  	/* Start the camera if that's the first request we handle. */
>>  	if (!running_) {
>> --
>> 2.31.0.291.g576ba9dcdaf-goog
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index eb327978..48eb471d 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 -EINVAL;
+	}
+
+	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";
-		return -EINVAL;
-	}
-
-	if (!camera3Request->num_output_buffers) {
-		LOG(HAL, Error) << "No output buffers provided";
+	if (isValidRequest(camera3Request))
 		return -EINVAL;
-	}
 
 	/* Start the camera if that's the first request we handle. */
 	if (!running_) {