[libcamera-devel] [PATCH v3 02/11] android: camera_device: Generate JPEG sizes

[Hirokazu Honda]

Thanks for the patch. LGTM.

On Thu, Sep 10, 2020 at 8:10 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Niklas,
>
> On Thu, Sep 10, 2020 at 12:33:06PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-09-08 15:41:33 +0200, Jacopo Mondi wrote:
> > > When producing the list of image resolutions to claim as supported by the
> > > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
> > > time, there was no JPEG support in place at all.
> > >
> > > With the introduction of support for JPEG compression, reporting
> > > non-valid sizes as supported obviously causes troubles.
> > >
> > > In order to avoid reporting non-supported resolutions as supported,
> > > produce the list of available JPEG sizes by using the ones supported
> > > by the YCbCr_420_888 format, from which the JPEG stream is encoded.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  src/android/camera_device.cpp | 40 ++++++++++++++++++++++-------------
> > >  1 file changed, 25 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 28fb3868c082..1b2e12d6d33c 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -363,17 +363,21 @@ int CameraDevice::initializeStreamConfigurations()
> > >             const std::vector<PixelFormat> &libcameraFormats =
> > >                     camera3Format.libcameraFormats;
> > >
> > > +           /*
> > > +            * JPEG is always supported, either produced directly by the
> > > +            * camera, or encoded in the HAL.
> > > +            */
> > > +           if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > > +                   formatsMap_[androidFormat] = formats::MJPEG;
> > > +                   continue;
> > > +           }
> > > +
> > >             /*
> > >              * Test the libcamera formats that can produce images
> > >              * compatible with the format defined by Android.
> > >              */
> > >             PixelFormat mappedFormat;
> > >             for (const PixelFormat &pixelFormat : libcameraFormats) {
> > > -                   /* \todo Fixed mapping for JPEG. */
> > > -                   if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > > -                           mappedFormat = formats::MJPEG;
> > > -                           break;
> > > -                   }
> > >
> > >                     /*
> > >                      * The stream configuration size can be adjusted,
> > > @@ -416,19 +420,25 @@ int CameraDevice::initializeStreamConfigurations()
> > >                     cfg.size = res;
> > >
> > >                     CameraConfiguration::Status status = cameraConfig->validate();
> > > -                   /*
> > > -                    * Unconditionally report we can produce JPEG.
> > > -                    *
> > > -                    * \todo The JPEG stream will be implemented as an
> > > -                    * HAL-only stream, but some cameras can produce it
> > > -                    * directly. As of now, claim support for JPEG without
> > > -                    * inspecting where the JPEG stream is produced.
> > > -                    */
> > > -                   if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> > > -                       status != CameraConfiguration::Valid)
> > > +                   if (status != CameraConfiguration::Valid)
> > >                             continue;
> > >
> > >                     streamConfigurations_.push_back({ res, androidFormat });
> > > +
> > > +                   /*
> > > +                    * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
> > > +                    * from which JPEG is produced, add an entry for
> > > +                    * the JPEG stream.
> > > +                    *
> > > +                    * \todo Wire the JPEG encoder to query the supported
> > > +                    * sizes provided a list of formats it can encode.
> >
> > nit: Is this needed? Are we expecting the JPEG encoder to reject some
> > resolutions?
>
> It's more about alignments and limitations in the supported
> resolutions.
>
> >
> > In any case,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > > +                    *
> > > +                    * \todo Support JPEG streams produced by the Camera
> > > +                    * natively.
> > > +                    */
> > > +                   if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > > +                           streamConfigurations_.push_back(
> > > +                                   { res, HAL_PIXEL_FORMAT_BLOB });
> > >             }
> > >     }
> > >
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel