[libcamera-devel] [PATCH 4/8] android: camera_device: Build stream configuration
Jacopo Mondi
jacopo at jmondi.org
Thu Jun 4 10:44:41 CEST 2020
Hi Laurent,
On Thu, Jun 04, 2020 at 05:13:44AM +0300, Laurent Pinchart wrote:
> 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 ?
>
You found out below here why a set, and it should be const indeed
> > + { 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.
>
I chose forward list as I considered more efficient as I only need to
walk it in one direction, so I assumed random access support was not
required. But I've now learned vector are much more space efficient,
so I could change this indeed
> > + { 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.
ack
>
> Ideally the second type should be
> camera_metadata_enum_android_scaler_available_formats_t, but I won't
> push for that :-)
bleah :/
>
> > + { 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; });
>
Much more C++, I'll take this in
> > +
> > + /* 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.
>
Nicer, yes
> > +
> > + 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());
I can't tell how more complex a set is, I wanted to avoid an
additional iteration to clean up the duplicated entries, but as this
is a one time operation that's probably acceptable.
>
> Up to you. For camera3Resolutions I would use a vector, regardless of
> what you use for cameraResolutions.
>
ack
> > + }
> > + 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 :-)
I wanted to avoid storing objects and I considered a uin32_t more
efficient. Not that PixelFormat is an heavyweight class, I think I
could store instances directly for a negligible overhead
>
> > + 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
Yes, very few pipeline handlers provide StreamFormats, and I was not
sure how appreciated that API is. I can add a \todo indeed
> 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 ?
Yeah, that's what I mean, I need to report JPEG regardless of the fact
we can produce it or not, and we can produce it from the Camera or
from an HAL-only stream. I'll expand the comment.
>
> > + 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.
That was meant to catch errors early when adding new formats. Unifying
the maps was an idea I briefly had, then I kept them separate as I was
not sure we actually need a map at al as
HAL_PIXEL_FORMAT_* == ANDROID_SCALER_AVAILABLE_FORMATS_*
(I know...)
so I thought this could end up by getting rid of the map entirely.
I'll see how it looks like with a single map.
>
> > + 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.
There's no push_back for std::forward_list
>
> > + }
> > + }
> > +
> > + 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.
Ack, at the moment is indeed not used.
Thanks
j
>
> > + };
> > +
> > + 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