[libcamera-devel] [PATCH v2 1/3] android: CameraDevice: Validate crop_rotate_scale_degrees in configuration

[Hirokazu Honda]

Hi Laurent, thanks for reviewing.

On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:
> > Libcamera doesn't handle |crop_rotate_scale_degrees| in
>
> s/Libcamera/libcamera/ (the name is always written in lowercase)
>
> > camera3_stream at all. This adds the validation of the requested
> > |crop_rotate_scale_degrees| in configuration, but still not
> > handle the specified values.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ae693664..c5e55a18 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
> >       unsortedConfigs = sortedConfigs;
> >  }
> >
> > +/*
> > + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list
>
> s/where/whether/ ?
>
> > + * are valid.
>
> I'd expand this a little bit to document that we're not validating the
> values against what libcamera support, but against what the Chrome OS
> API defines.
>
>  * Check whether the crop_rotate_scale_degrees values for all streams in
>  * the list are valid according to the Chrome OS camera HAL API.
>
> > + */
> > +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)
>
> s/stream_list/streamList/
>
> > +{
> > +     ASSERT(stream_list.num_streams > 0);
> > +
> > +     const int cropRotateScaleDegrees =
> > +             stream_list.streams[0]->crop_rotate_scale_degrees;
> > +     for (unsigned int i = 0; i < stream_list.num_streams; ++i) {
> > +             const camera3_stream_t &stream = *stream_list.streams[i];
> > +             if (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||
> > +                 CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {
> > +                     LOG(HAL, Error) << "Invalid crop_rotate_scale_degrees: "
> > +                                     << stream.crop_rotate_scale_degrees;
> > +                     return false;
> > +             }
> > +             if (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {
> > +                     LOG(HAL, Error) << "crop_rotate_scale_degrees should "
> > +                                     << "not be CAMERA3_STREAM_ROTATION_180";
>
> A comment to explain why this is the case would be useful.
>
> > +                     return false;
> > +             }
>
> How about simplifying this to
>
>                 switch (stream.crop_rotate_scale_degrees) {
>                 /* 180° rotation is specified by Chrome OS as invalid. */
>                 case CAMERA3_STREAM_ROTATION_180:
>                 default:
>                         LOG(HAL, Error) << "Invalid crop_rotate_scale_degrees: "
>                                         << stream.crop_rotate_scale_degrees;
>                         return false;
>
>                 case CAMERA3_STREAM_ROTATION_0:
>                 case CAMERA3_STREAM_ROTATION_90:
>                 case CAMERA3_STREAM_ROTATION_270:
>                         break;
>                 }
>
> > +             if (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {
> > +                     LOG(HAL, Error) << "crop_rotate_scale_degrees in all "
> > +                                     << "streams are not identical";
> > +                     return false;
> > +             }
> > +     }
> > +
> > +     return true;
> > +}
> > +
> >  } /* namespace */
> >
> >  /*
> > @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >               running_ = false;
> >       }
> >
> > +     if (stream_list->num_streams == 0)
> > +             return -EINVAL;
> > +
> > +     if (!validateCropRotate(*stream_list))
> > +             return -EINVAL;
>
> This should only be enabled when the 'android_platform' meson option is
> to to 'cros', as it's not available on Android. In the Android camera
> HAL v3.4, the first reserved field has been used for physical_camera_id,
> so there's a risk the value wouldn't be zero.
>

Right. If you don't mind, can I submit this patch as-is and quickly
submit a patch of limiting this with android_platform='cros' later?

Regards,
-Hiro
> > +
> >       /*
> >        * Generate an empty configuration, and construct a StreamConfiguration
> >        * for each camera3_stream to add to it.
>
> --
> Regards,
>
> Laurent Pinchart