[libcamera-devel] [PATCH v2 02/12] android: camera_device: Generate JPEG sizes

[Hirokazu Honda]

Hi,

On Sun, Sep 6, 2020 at 2:49 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> > When producing the list of image resolution to claim as supported by the
>
> s/resolution/resolutions/
>
> > 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.
>
> Down the line we'll likely need to scale down images for JPEG
> compression, but for now this looks fine to me.
>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ad0d7fd15d90..8a8072123961 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
> >               const std::vector<PixelFormat> &libcameraFormats =
> >                       camera3Format.libcameraFormats;
> >
> > +             /*
> > +              * Fixed format mapping for JPEG.
> > +              *
> > +              * The list of supported JPEG resolutions is generated
> > +              * from the list of resolutions supported by
> > +              * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.
>
> This doesn seem to correspond to the code below. Should it read
>
>                 /*
>                  * JPEG is always supported, either produced directly by the
>                  * camera, or encoded in the HAL.
>                  */
>
> ?
>
> > +              *
> > +              * \todo Wire the JPEG encoder interface to query the list
> > +              * of supported resolutions.
>
> Is this still valid ?
>
> > +              */
> > +             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 +426,22 @@ 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 });

This comment is probably the same as Laurent's one.
The change looks to allow using native jpeg output streams.

> > +
> > +                     /*
> > +                      * 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.
>
> We shouldn't do this if the device can provide JPEG natively. Is this
> something you'd like to address already ? Otherwise we should add a
> \todo item for this too.
>
> > +                      */
> > +                     if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> > +                             streamConfigurations_.push_back(
> > +                                     { res, HAL_PIXEL_FORMAT_BLOB });
> >               }
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Best Regards,
-Hiro
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel