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

[Hirokazu Honda]

Thanks Laurent for the comment.

It sounds a good solution so far.
I also noticed Jacopo has merged a patch with your idea.
Thanks Jacopo.

Best Regards,
-Hiro

On Mon, Dec 6, 2021 at 8:09 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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