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

[Laurent Pinchart]

Hi Hiro,

On Mon, Mar 29, 2021 at 06:37:15PM +0900, Hirokazu Honda wrote:
> On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart wrote:
> > 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?

libcamera is already used on Android by at least one project, and I'd
like to avoid breaking bisection for them. We introduce enough
regression on the Android side already without noticing, it would be
better to avoid at least the regressions that we are aware of :-) Would
you mind adding the android_platform option check already ?

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

-- 
Regards,

Laurent Pinchart