[libcamera-devel] [PATCH 4/8] android: camera_device: Build stream configuration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 4 04:13:44 CEST 2020
Hi Jacopo,
Thank you for the patch.
On Tue, May 26, 2020 at 04:22:33PM +0200, 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 | 184 ++++++++++++++++++++++++++++++++++
> src/android/camera_device.h | 13 +++
> 2 files changed, 197 insertions(+)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 69b25ed2f11f..534bfb1df1ef 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 <set>
> +
> #include <libcamera/controls.h>
> #include <libcamera/property_ids.h>
>
> @@ -15,9 +17,37 @@
> #include "libcamera/internal/utils.h"
>
> #include "camera_metadata.h"
> +#include "system/graphics.h"
>
> using namespace libcamera;
>
> +namespace {
> +
> +std::set<Size> camera3Resolutions = {
Does this need to be a set, shouldn't it be a vector ? And shouldn't it
be const ?
> + { 320, 240 },
> + { 640, 480 },
> + { 1280, 720 },
> + { 1920, 1080 }
> +};
> +
> +std::map<int, std::forward_list<uint32_t>> camera3FormatsMap = {
The map should be const too, and I would make the value an std::vector
instead of an std::forward_list, it will be more efficient.
> + { HAL_PIXEL_FORMAT_BLOB, { DRM_FORMAT_MJPEG } },
> + { HAL_PIXEL_FORMAT_YCbCr_420_888, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },
I have no words to describe how lovely format handling is in Android.
For reference, there's some documentation I found in
platform/hardware/interfaces/graphics/common/1.0/types.hal while trying
to figure this out.
I think we'll eventually have to add DRM_FORMAT_YUV420 and
DRM_FORMAT_YVU420, but that can wait.
> + /*
> + * \todo Translate IMPLEMENTATION_DEFINED inspecting the
> + * gralloc usage flag. For now, copy the YCbCr_420 configuration.
> + */
> + { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },
> +};
> +
> +std::map<int32_t, int32_t> camera3ScalerFormatMap = {
const here too.
Ideally the second type should be
camera_metadata_enum_android_scaler_available_formats_t, but I won't
push for that :-)
> + { HAL_PIXEL_FORMAT_BLOB, ANDROID_SCALER_AVAILABLE_FORMATS_BLOB },
> + { HAL_PIXEL_FORMAT_YCbCr_420_888, ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888 },
> + { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED },
> +};
> +
> +} /* namespace */
> +
> LOG_DECLARE_CATEGORY(HAL);
>
> /*
> @@ -100,6 +130,160 @@ int CameraDevice::initialize()
> if (properties.contains(properties::Rotation))
> orientation_ = properties.get(properties::Rotation);
>
> + int ret = camera_->acquire();
> + if (ret) {
> + LOG(HAL, Error) << "Failed to temporary acquire the camera";
s/temporary/temporarily/
> + return ret;
> + }
> +
> + ret = initializeFormats();
> + camera_->release();
> + return ret;
> +}
> +
> +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 (alignement
s/alignement/alignment/
You can reflow the comment to 80 columns. Same for other comments below.
> + * and aspect ratio).
> + */
> + const Size maxRes = cfg.size;
> + 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::set<Size> cameraResolutions;
std::vector here too ?
> + for (const Size &res : camera3Resolutions) {
> + if (res > maxRes)
> + continue;
> +
> + cameraResolutions.insert(res);
> + }
An alternative is
std::copy_if(camera3Resolutions.begin(), camera3Resolutions.end(),
std::back_inserter(cameraResolutions),
[&](const Size &res) { return res > maxRes; });
> +
> + /* Camera3 specification suggest to add 1/2 and 1/4 max resolution. */
> + for (unsigned int divider = 2;; divider <<= 1) {
> + Size derivedSize{};
> + derivedSize.width = maxRes.width / divider;
> + derivedSize.height = maxRes.height / divider;
Maybe
Size derivedSize{
maxRes.width / divider,
maxRes.height / divider
};
? Up to you.
> +
> + if (derivedSize.width < 320 ||
> + derivedSize.height < 240)
> + break;
> +
> + /* std::set::insert() guarantees the entry is unique. */
> + cameraResolutions.insert(derivedSize);
Ah that's why you use std::set. Given the additional complexity of
std::set, I wonder if it wouldn't be simpler to still use a vector, and
when done, do
std::sort(cameraResolutions.begin(), cameraResolutions.end());
auto last = std::unique(cameraResolutions.begin(), cameraResolutions.end());
cameraResolutions.erase(last, cameraResolutions.end());
Up to you. For camera3Resolutions I would use a vector, regardless of
what you use for cameraResolutions.
> + }
> + cameraResolutions.insert(maxRes);
> +
> + /*
> + * Build the list of supported camera format.
s/format/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 in the camera static
> + * metadata.
> + */
> + for (const auto &format : camera3FormatsMap) {
> + int androidFormatCode = format.first;
Maybe s/androidFormatCode/androidFormat/ ?
> + const std::forward_list<uint32_t> testFormats = format.second;
This should be a reference.
And maybe s/testFormats/libcameraFormats/ ?
> +
> + /*
> + * Test the libcamera formats that can produce images
> + * compatible with the Android's defined format
s/$/./
> + */
> + uint32_t mappedFormatCode = 0;
s/Code// ?
> + for (int32_t formatCode : testFormats) {
> + /*
> + * \todo Fixed mapping for JPEG
> + */
> + if (androidFormatCode == HAL_PIXEL_FORMAT_BLOB) {
> + mappedFormatCode = DRM_FORMAT_MJPEG;
> + break;
> + }
> +
> + /*
> + * The stream configuration size can be adjusted,
> + * not the pixel format.
> + */
> + PixelFormat pixelFormat = PixelFormat(formatCode);
PixelFormat pixelFormat{ formatCode };
But how about storing instances of PixelFormat in camera3FormatsMap, and
turning mappedFormatCode into a PixelFormat ? You will have a nice
isValid() function for the check below :-)
> + cfg.pixelFormat = pixelFormat;
> +
> + CameraConfiguration::Status status = cameraConfig->validate();
> + if (status != CameraConfiguration::Invalid &&
> + cfg.pixelFormat == pixelFormat) {
> + mappedFormatCode = pixelFormat.fourcc();
> + break;
> + }
Wouldn't it be simpler to just check if formatCode is in
cfg.formats().pixelformats() ? Or is this code meant to work around the
fact that not all pipeline handlers provide StreamFormats ? In that case
a \todo should be recorded here to explain the problem, and we should
fix it in pipeline handlers (possibly reworking the StreamFormats API).
> + }
> + if (!mappedFormatCode) {
> + LOG(HAL, Error) << "Failed to get map Android format "
s/get map/map/ ?
> + << utils::hex(androidFormatCode);
> + return -EINVAL;
> + }
> +
> + /*
> + * Record the mapping and then proceed to generate the
> + * stream configuration map, by testing the image resolutions.
> + */
> + formatsMap_[androidFormatCode] = mappedFormatCode;
> +
> + for (const Size &res : cameraResolutions) {
> + PixelFormat pixelFormat = PixelFormat(mappedFormatCode);
> + cfg.pixelFormat = pixelFormat;
> + cfg.size = res;
> +
> + CameraConfiguration::Status status = cameraConfig->validate();
> + /* \todo Assume we the camera can produce JPEG */
s/we the/the/
Not a very good assumption, that's only for a minority of cameras :-) I
suppose we'll handle this with JPEG encoding in the HAL ? Should the
comment be reworded to mention that ?
> + if (androidFormatCode != HAL_PIXEL_FORMAT_BLOB &&
> + status != CameraConfiguration::Valid)
> + continue;
> +
> + auto it = camera3ScalerFormatMap.find(androidFormatCode);
> + if (it == camera3ScalerFormatMap.end()) {
> + LOG(HAL, Error) << "Format " << utils::hex(androidFormatCode)
> + << " has no scaler format associated";
> + return -EINVAL;
> + }
Can this happen ? Would it be enough to have a comment above to warn
that camera3FormatsMap and camera3ScalerFormatMap must match ? Or, maybe
better, merge the two maps into one, with the value being a structure
that contains both the PixelFormat and the scaler format ? That would
make the error impossible.
> + int32_t androidScalerCode = it->second;
> +
> + /*
> + * \todo Add support for input streams. At the moment
> + * register all stream configurations as output-only.
> + */
> + streamConfigurations_.push_front(
> + { res, androidScalerCode, false });
I'm curious, why front ? Does Android require sorting formats from
largest to smallest ? If so, wouldn't it make sense to sort
cameraResolutions in the same order ? That would allow turning
streamConfigurations_ into a vector and still keep the insertion
relatively efficient.
> + }
> + }
> +
> + LOG(HAL, Debug) << "Collected stream configuration map: ";
> + for (const auto &entry : streamConfigurations_) {
> + LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> + << utils::hex(entry.androidScalerCode) << ": "
> + << (entry.input ? "input" : "output") << " }";
> + }
> +
> return 0;
> }
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index ace9c1b7c929..95bd39f590ab 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 <forward_list>
> +#include <map>
> #include <memory>
>
> #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,13 @@ private:
> camera3_stream_buffer_t *buffers;
> };
>
> + struct Camera3StreamConfiguration {
> + libcamera::Size resolution;
> + int androidScalerCode;
> + bool input;
I would drop the input field for now, the HAL will see major reworks
when implementing reprocessing, it will very likely not be kept.
> + };
> +
> + 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 +85,9 @@ private:
> std::map<unsigned int, CameraMetadata *> requestTemplates_;
> const camera3_callback_ops_t *callbacks_;
>
> + std::forward_list<Camera3StreamConfiguration> streamConfigurations_;
> + std::map<int, uint32_t> formatsMap_;
> +
> int facing_;
> int orientation_;
> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list