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

[Hirokazu Honda]

On Thu, Mar 25, 2021 at 7:44 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Thu, Mar 25, 2021 at 02:19:29PM +0900, Hirokazu Honda wrote:
> > Libcamera doesn't handle |crop_rotate_scale_degrees| in
> > 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 | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 72a89258..1414bfef 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -256,6 +256,38 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
> >       unsortedConfigs = sortedConfigs;
> >  }
> >
> > +/*
> > + *  verifyCropRotateScaleDegrees returns where |crop_rotate_scale_degrees| in
>
> double space at beginning of the line
> > + * all camera3_stream in stream_list are valid.
>
> And usage of |variable| is a bit alien to libcamera :)
>
> > + */
> > +bool verifyCropRotateScaleDegrees(const camera3_stream_configuration_t &stream_list)
>
> just verifyCropRotate() ?
> Or better validateCropRotate() ?
>
> > +{
> > +     if (stream_list.num_streams == 0)
> > +             return true;
>
> Emtpy line ?
> Also, is this worth being checked in configureStreams() ?
>
> > +     const int crop_rotate_scale_degrees =
>
> We use camelCase for variables
>
> > +             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) {
>
> These are the possible values for crop_rotate_scale_degrees
>
> /**
>  * camera3_stream_rotation_t:
>  *
>  * The required counterclockwise rotation of camera stream.
>  */
> typedef enum camera3_stream_rotation {
>     /* No rotation */
>     CAMERA3_STREAM_ROTATION_0 = 0,
>
>     /* Rotate by 90 degree counterclockwise */
>     CAMERA3_STREAM_ROTATION_90 = 1,
>
>     /* Rotate by 180 degree counterclockwise */
>     CAMERA3_STREAM_ROTATION_180 = 2,
>
>     /* Rotate by 270 degree counterclockwise */
>     CAMERA3_STREAM_ROTATION_270 = 3
> } camera3_stream_rotation_t;
>
> The filed is documented as
>
>    /**
>      * This should be one of the camera3_stream_rotation_t values except for
>      * CAMERA3_STREAM_ROTATION_180.
>      * When setting to CAMERA3_STREAM_ROTATION_90 or CAMERA3_STREAM_ROTATION_270, HAL would crop,
>      * rotate the frame by the specified degrees clockwise and scale it up to original size.
>      * In Chrome OS, it's possible to have a portrait activity run in a landscape screen with
>      * landscape-mounted camera. The activity would show stretched or rotated preview because it
>      * does not expect to receive landscape preview frames. To solve this problem, we ask HAL to
>      * crop, rotate and scale the frames and modify CameraCharacteristics.SENSOR_ORIENTATION
>      * accordingly to imitate a portrait camera.
>      * Setting it to CAMERA3_STREAM_ROTATION_0 means no crop-rotate-scale would be performed.
>      * |cros_rotate_scale_degrees| in all camera3_stream_t of a configure_streams() call must be
>      * identical. The HAL should return -EINVAL if the degrees are not the same for all the streams.
>      */
>
> I think checking for != ROTATION_180 would be enough, but in
> camera3_stream_t the field as type int, not
> camera3_stream_rotation_t, so do we need to make sure it is 'valid'
> in the [0, 3] interval ? It doesn't hurt though
>

Yeah, since the type is int, it can be any value.
In fact, chromeos camera test has a test to set an invalid value and
checks if the configuration fails.
So we should keep this check.

> Handling rotation properly will be quite an headache as we'll have to
> compensate the properties::Rotation value from the Camera. I admit
> it's not yet clear in my mind how that will have to be done, but we'll
> see. I guess we cannot rely on the sensor's flips but this will have
> to go through the YUV post-processor (also to handle the crop-scale
> part)
>

My intuition is to flip (rotate) on a sensor, and cropping and scaling
is done in the YUV post-processor.
If a sensor is not reliable, all should be done in the YUV post processor.

Best Regards,
-Hiro

> > +                     LOG(HAL, Error) << "invalid crop_rotate_scale_degrees: "
> > +                                     << stream.crop_rotate_scale_degrees;
>
> Invalid
>
> > +                     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";
> > +                     return false;
> > +             }
> > +             if (crop_rotate_scale_degrees != stream.crop_rotate_scale_degrees) {
> > +                     LOG(HAL, Error) << "crop_rotate_scale_degrees in all "
> > +                                     << "streams are not identical";
> > +                     return false;
> > +             }
> > +     }
>
> Empty line ?
>
> > +     return true;
> > +}
> > +
> >  } /* namespace */
> >
> >  /*
> > @@ -1566,6 +1598,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >               running_ = false;
> >       }
> >
> > +     if (!verifyCropRotateScaleDegrees(*stream_list))
> > +             return -EINVAL;
> > +
> >       /*
> >        * Generate an empty configuration, and construct a StreamConfiguration
> >        * for each camera3_stream to add to it.
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel