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

[Hirokazu Honda]

Hi Laurent,

On Tue, Mar 30, 2021 at 12:56 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
>

Fair enough. I will do in the next patch series.

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