[libcamera-devel] [PATCH 1/2] android: camera_capabilities: Add function to convert PixelFormat to android format

[Laurent Pinchart]

Hi Hiro,

On Wed, Dec 01, 2021 at 04:28:07PM +0900, Hirokazu Honda wrote:
> On Wed, Dec 1, 2021 at 11:26 AM Laurent Pinchart wrote:
> > On Tue, Nov 30, 2021 at 09:44:27PM +0900, Hirokazu Honda wrote:
> > > This adds a function to CameraCapabilities of converting PixelFormat
> > > to android format.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  src/android/camera_capabilities.cpp | 18 ++++++++++++++++++
> > >  src/android/camera_capabilities.h   |  2 ++
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index f357902e..4c1dc87c 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -545,6 +545,7 @@ int CameraCapabilities::initializeStreamConfigurations()
> > >                */
> > >               if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > >                       formatsMap_[androidFormat] = formats::MJPEG;
> > > +                     revFormatsMap_[formats::MJPEG] = androidFormat;
> > >                       LOG(HAL, Debug) << "Mapped Android format "
> > >                                       << camera3Format.name << " to "
> > >                                       << formats::MJPEG.toString()
> > > @@ -596,6 +597,7 @@ int CameraCapabilities::initializeStreamConfigurations()
> > >                * stream configurations map, by testing the image resolutions.
> > >                */
> > >               formatsMap_[androidFormat] = mappedFormat;
> > > +             revFormatsMap_[mappedFormat] = androidFormat;
> > >               LOG(HAL, Debug) << "Mapped Android format "
> > >                               << camera3Format.name << " to "
> > >                               << mappedFormat.toString();
> > > @@ -1422,6 +1424,22 @@ PixelFormat CameraCapabilities::toPixelFormat(int format) const
> > >       return it->second;
> > >  }
> > >
> > > +/*
> > > + * Translate libcamera pixel format to Android format code. -1 if no mapped
> > > + * android format is found.
> > > + */
> > > +int CameraCapabilities::toAndroidFormat(PixelFormat format) const
> > > +{
> > > +     auto it = revFormatsMap_.find(format);
> > > +     if (it == revFormatsMap_.end()) {
> > > +             LOG(HAL, Error) << "Requested format " << format.toString()
> > > +                             << " not supported";
> > > +             return -1;
> > > +     }
> > > +
> > > +     return it->second;
> >
> > I think there's a conceptual problem here. While a given Android format
> > can always be translated to a PixelFormat, the mapping isn't 1:1. We
> > already map different Android formats to NV21 for instance, and I expect
> > this to continue as we support more formats. A reverse map is thus
> > ill-defined, there's no guarantee that
> >
> >         toAndroidFormat(toPixelFormat(format)) == format
> >
> > which I think can lead to subtle bugs. Can't we instead, in the next
> > patch, get the Android format from camera3Stream_->format ?
> 
> camera3Stream_->format is what Android HAL client requests.
> From camera3Stream_->format, we decide to request configuration to camera.
> Since the created buffer is the input to camera, its format must be
> configuration().pixelFormat.
> Both PlatformFrameBufferAllocator (gralloc and
> cros::CameraBufferManager) demands android format.
> Further, pixelFormat resolved from android format is dependent on the
> backend implementation.
> Therefore it is probably correct to lookup a proper android format
> from pixelFormat.
> But there is no such API in gralloc API and cros::CameraBufferManager.

I understand that the gralloc and cros::CameraBufferManager APIs are
based on Android pixel formats and not on 4CCs. My concern is that the
CameraCapabilities::toAndroidFormat() function is meant to be generic,
but doesn't actually guarantee that

	toAndroidFormat(toPixelFormat(format)) == format

It's probably fine as-is for the specific use case of patch 2/2, but I
think it could cause issues if we start using it elsewhere.

If my understanding is correct, we can't really use
camera3Stream_->format in a generic way for internal streams. Because
internal streams are internal, they're not created directly from an
Android pixel format. At this time, we want to use post-processing to
produce

- JPEG streams, which we hardcode being produced from an internal NV12
  stream in case no compatible non-compressed streams is also requested.

- YUV streams, produced by scalign through libyuv, but without format
  conversion.

There may be other use cases in the future that would involve format
conversion, but I wonder if it wouldn't be best at this point to use the
Android pixel format directly in both cases instead of converting from a
libcamera pixel format.

> > > +}
> > > +
> > >  std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() const
> > >  {
> > >       if (!capabilities_.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {
> > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h
> > > index 2cf97ae8..df6361f1 100644
> > > --- a/src/android/camera_capabilities.h
> > > +++ b/src/android/camera_capabilities.h
> > > @@ -30,6 +30,7 @@ public:
> > >
> > >       CameraMetadata *staticMetadata() const { return staticMetadata_.get(); }
> > >       libcamera::PixelFormat toPixelFormat(int format) const;
> > > +     int toAndroidFormat(libcamera::PixelFormat format) const;
> > >       unsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }
> > >
> > >       std::unique_ptr<CameraMetadata> requestTemplateManual() const;
> > > @@ -77,6 +78,7 @@ private:
> > >
> > >       std::vector<Camera3StreamConfiguration> streamConfigurations_;
> > >       std::map<int, libcamera::PixelFormat> formatsMap_;
> > > +     std::map<libcamera::PixelFormat, int> revFormatsMap_;
> > >       std::unique_ptr<CameraMetadata> staticMetadata_;
> > >       unsigned int maxJpegBufferSize_;
> > >

-- 
Regards,

Laurent Pinchart