[libcamera-devel] [PATCH v2 1/4] android: camera_device: Build stream configuration
Jacopo Mondi
jacopo at jmondi.org
Fri Jun 5 10:39:18 CEST 2020
Hi Kieran
On Fri, Jun 05, 2020 at 09:28:07AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 04/06/2020 14:35, Jacopo Mondi wrote:
> > Build the stream configuration map by applying the Android Camera3
> > requested resolutions and formats to the libcamera Camera device.
> >
> > For each required format test a list of required and optional
> > resolutions, construct a map to translate from Android format to the
> > libcamera formats and store the available stream configuration to
> > be provided to the Android framework through static metadata.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/android/camera_device.cpp | 218 ++++++++++++++++++++++++++++++++++
> > src/android/camera_device.h | 12 ++
> > 2 files changed, 230 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b30263451b76..0c462f53c9cc 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -8,6 +8,8 @@
> > #include "camera_device.h"
> > #include "camera_ops.h"
> >
> > +#include <vector>
> > +
> > #include <libcamera/controls.h>
> > #include <libcamera/property_ids.h>
> >
> > @@ -15,9 +17,70 @@
> > #include "libcamera/internal/utils.h"
> >
> > #include "camera_metadata.h"
> > +#include "system/graphics.h"
> >
> > using namespace libcamera;
> >
> > +namespace {
> > +
> > +/*
> > + * \var camera3Resolutions
> > + * \brief The list of image resolutions defined as mandatory to be supported by
> > + * the Android Camera3 specification
> > + */
> > +const std::vector<Size> camera3Resolutions = {
> > + { 320, 240 },
> > + { 640, 480 },
> > + { 1280, 720 },
> > + { 1920, 1080 }
> > +};
> > +
> > +/*
> > + * \struct Camera3Format
> > + * \brief Data associated with an Android format identifier
> > + * \var Camera3Format::libcameraFormats: List of libcamera pixel formats
>
> do you need to prefix Camera3Format:: in the same comment block?
>
Don't I ? Actually, this is not even parsed by Doxygen, I chose to
keep the same syntax for consistency, but I could drop the prefix
> > + * compatible with the Android format
> > + * \var Camera3Format::scalerFormat: The format identifier to be reported to the
> > + * android framework through the static format configuration map
> > + */
> > +struct Camera3Format {
> > + std::vector<PixelFormat> libcameraFormats;
> > + camera_metadata_enum_android_scaler_available_formats_t scalerFormat;
> > +};
> > +
> > +/*
> > + * \var camera3FormatsMap
> > + * \brief Associate Android format code with ancillary data
> > + */
> > +const std::map<int, const Camera3Format> camera3FormatsMap = {
> > + {
> > + HAL_PIXEL_FORMAT_BLOB, {
>
> Is there no defined MJPEG format for Android HAL ? 'just a blob' :-S
Seems so
>
>
> > + { PixelFormat(DRM_FORMAT_MJPEG) },
> > + ANDROID_SCALER_AVAILABLE_FORMATS_BLOB
> > + }
> > + },
> > +
> > + {
> > + HAL_PIXEL_FORMAT_YCbCr_420_888, {
> > + { PixelFormat(DRM_FORMAT_NV12), PixelFormat(DRM_FORMAT_NV21) },
> > + ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888
>
> How is the ordering determined between multiple formats like that ?
>
> I.e. - how does something know what the correct format to choose will be?
>
> Ah ok - I see below they get iterated with calls to validate them....
Yes
>
> > + }
> > + },
> > +
> > + /*
> > + * \todo Translate IMPLEMENTATION_DEFINED inspecting the
> > + * gralloc usage flag. For now, copy the YCbCr_420 configuration.
> > + */
> > + {
> > + HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, {
> > + { PixelFormat(DRM_FORMAT_NV12), PixelFormat(DRM_FORMAT_NV21) },
> > + ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888
> > + }
> > + },
> > +};
> > +
> > +} /* namespace */
> > +
> > LOG_DECLARE_CATEGORY(HAL);
> >
> > /*
> > @@ -100,6 +163,161 @@ int CameraDevice::initialize()
> > if (properties.contains(properties::Rotation))
> > orientation_ = properties.get(properties::Rotation);
> >
> > + int ret = camera_->acquire();
> > + if (ret) {
> > + LOG(HAL, Error) << "Failed to temporarily acquire the camera";
> > + return ret;
> > + }
> > +
> > + ret = initializeFormats();
> > + camera_->release();
> > + return ret;
> > +}
> > +
> > +/*
> > + * Initialize the format conversion map to translate from Android format
> > + * identifier to libcamera pixel formats and fill in the list of supported
> > + * stream configurations to be reported to the Android camera framework through
> > + * the static stream configuration metadata.
> > + */
> > +int CameraDevice::initializeFormats()
> > +{
> > + /*
> > + * Get the maximum output resolutions
> > + * \todo Get this from the camera properties once defined
> > + */
> > + std::unique_ptr<CameraConfiguration> cameraConfig =
> > + camera_->generateConfiguration({ StillCapture });
> > + if (!cameraConfig) {
> > + LOG(HAL, Error) << "Failed to get maximum resolution";
> > + return -EINVAL;
> > + }
> > + StreamConfiguration &cfg = cameraConfig->at(0);
> > +
> > + /*
> > + * \todo JPEG - Adjust the maximum available resolution by taking the
> > + * JPEG encoder requirements into account (alignment and aspect ratio).
> > + */
> > + const Size maxRes = cfg.size;
>
> Do we already define that a generateConfiguration({ StillCapture })
> should return the largest size as a default?
>
We're not enforcing that afaict, it's up to pipeline handlers, but I
expect they return the maximum available resolution in that case.
Anyway, this is just temporary, as we'll have a property that reports
the maximum image resolution you can get from a camera.
> Or should the 'max' size further inspect the cfg.formats() and determine
> the largest supported size from there?
>
If only more than 2 pipeline handlers provided StreamFormats we could,
but again, I would switch to inspecting the property once it's
available
> > + LOG(HAL, Debug) << "Maximum supported resolution: " << maxRes.toString();
> > +
> > + /*
> > + * Build the list of supported image resolutions.
> > + *
> > + * The resolutions listed in camera3Resolution are mandatory to be
> > + * supported, up to the camera maximum resolution.
> > + *
> > + * Augment the list by adding resolutions calculated from the camera
> > + * maximum one.
> > + */
> > + std::vector<Size> cameraResolutions;
> > + std::copy_if(camera3Resolutions.begin(), camera3Resolutions.end(),
> > + std::back_inserter(cameraResolutions),
> > + [&](const Size &res) { return res < maxRes; });
> > +
> > + /* Camera3 specification suggests to add 1/2 and 1/4 max resolution. */
> > + for (unsigned int divider = 2;; divider <<= 1) {
> > + Size derivedSize{
> > + maxRes.width / divider,
> > + maxRes.height / divider,
> > + };
> > +
> > + if (derivedSize.width < 320 ||
> > + derivedSize.height < 240)
> > + break;
> > +
> > + cameraResolutions.push_back(derivedSize);
> > + }
> > + cameraResolutions.push_back(maxRes);
> > +
> > + /* Remove duplicated entries from the list of supported resolutions. */
> > + std::sort(cameraResolutions.begin(), cameraResolutions.end());
> > + auto last = std::unique(cameraResolutions.begin(), cameraResolutions.end());
> > + cameraResolutions.erase(last, cameraResolutions.end());
>
> Ah, I had to look that up:
> https://en.cppreference.com/w/cpp/algorithm/unique shows you have to
> manually erase to the end after a unique. It looks odd in the code if
> you don't know that ;-)
>
> > +
> > + /*
> > + * Build the list of supported camera formats.
> > + *
> > + * To each Android format a list of compatible libcamera formats is
> > + * associated. The first libcamera format that tests successful is added
> > + * to the format translation map used when configuring the streams.
> > + * It is then tested against the list of supported camera resolutions to
> > + * build the stream configuration map reported through the camera static
> > + * metadata.
> > + */
> > + for (const auto &format : camera3FormatsMap) {
> > + int androidFormat = format.first;
> > + const Camera3Format &camera3Format = format.second;
> > + const std::vector<PixelFormat> &libcameraFormats =
> > + camera3Format.libcameraFormats;
> > +
> > + /*
> > + * Test the libcamera formats that can produce images
> > + * compatible with the Android defined format.
> > + */
> > + PixelFormat mappedFormat{};
> > + for (const PixelFormat &format : libcameraFormats) {
>
> You're aliasing const auto &format, and const PixelFormat &format here...
>
> Not necessarily a problem, but prevents accessing the camera3FormatsMap
> entry ...
Ah right, this was not intentional, even if it seems harmless. I'll
change this
>
> > + /* \todo Fixed mapping for JPEG. */
>
> /me takes note....
>
Yeah, about this, and the above todo note as well, please
/*
* \todo JPEG - Adjust the maximum available resolution by taking the
* JPEG encoder requirements into account (alignment and aspect ratio).
*/
> > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> > + mappedFormat = PixelFormat(DRM_FORMAT_MJPEG);
> > + break;
> > + }
> > +
> > + /*
> > + * The stream configuration size can be adjusted,
> > + * not the pixel format.
> > + *
> > + * \todo This could be simplified once all pipeline
> > + * handlers will report the StreamFormats list of
> > + * supported formats.
> > + */
> > + cfg.pixelFormat = format;
> > +
> > + CameraConfiguration::Status status = cameraConfig->validate();
> > + if (status != CameraConfiguration::Invalid &&
> > + cfg.pixelFormat == format) {
> > + mappedFormat = format;
> > + break;
> > + }
> > + }
> > + if (!mappedFormat.isValid()) {
> > + LOG(HAL, Error) << "Failed to map Android format "
> > + << utils::hex(androidFormat);
>
> Is it worth putting a char * mapping of the android format name in
> camera3FormatsMap to be able to easily diagnose which format failed here?
>
not strictly required but might be nice
Thanks
j
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Record the mapping and then proceed to generate the
> > + * stream configurations map, by testing the image resolutions.
> > + */
> > + formatsMap_[androidFormat] = mappedFormat;
> > +
> > + for (const Size &res : cameraResolutions) {
> > + cfg.pixelFormat = mappedFormat;
> > + 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)
> > + continue;
> > +
> > + streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
> > + }
> > + }
> > +
> > + LOG(HAL, Debug) << "Collected stream configuration map: ";
> > + for (const auto &entry : streamConfigurations_)
> > + LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> > + << utils::hex(entry.androidScalerCode);
> > +
> > return 0;
> > }
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a87f7623c790..d31b7233e795 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -7,12 +7,15 @@
> > #ifndef __ANDROID_CAMERA_DEVICE_H__
> > #define __ANDROID_CAMERA_DEVICE_H__
> >
> > +#include <map>
> > #include <memory>
> > +#include <vector>
> >
> > #include <hardware/camera3.h>
> >
> > #include <libcamera/buffer.h>
> > #include <libcamera/camera.h>
> > +#include <libcamera/geometry.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > @@ -59,6 +62,12 @@ private:
> > camera3_stream_buffer_t *buffers;
> > };
> >
> > + struct Camera3StreamConfiguration {
> > + libcamera::Size resolution;
> > + int androidScalerCode;
> > + };
> > +
> > + int initializeFormats();
> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> > @@ -75,6 +84,9 @@ private:
> > std::map<unsigned int, CameraMetadata *> requestTemplates_;
> > const camera3_callback_ops_t *callbacks_;
> >
> > + std::vector<Camera3StreamConfiguration> streamConfigurations_;
> > + std::map<int, libcamera::PixelFormat> formatsMap_;
> > +
> > int facing_;
> > int orientation_;
> > };
> >
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list