| Message ID | 20200526142237.407557-5-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@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_; > };
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@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
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 = { + { 320, 240 }, + { 640, 480 }, + { 1280, 720 }, + { 1920, 1080 } +}; + +std::map<int, std::forward_list<uint32_t>> camera3FormatsMap = { + { HAL_PIXEL_FORMAT_BLOB, { DRM_FORMAT_MJPEG } }, + { HAL_PIXEL_FORMAT_YCbCr_420_888, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } }, + /* + * \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 = { + { 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"; + 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 + * 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; + for (const Size &res : camera3Resolutions) { + if (res > maxRes) + continue; + + cameraResolutions.insert(res); + } + + /* 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; + + if (derivedSize.width < 320 || + derivedSize.height < 240) + break; + + /* std::set::insert() guarantees the entry is unique. */ + cameraResolutions.insert(derivedSize); + } + cameraResolutions.insert(maxRes); + + /* + * Build the list of supported camera format. + * + * 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; + const std::forward_list<uint32_t> testFormats = format.second; + + /* + * Test the libcamera formats that can produce images + * compatible with the Android's defined format + */ + uint32_t mappedFormatCode = 0; + 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); + cfg.pixelFormat = pixelFormat; + + CameraConfiguration::Status status = cameraConfig->validate(); + if (status != CameraConfiguration::Invalid && + cfg.pixelFormat == pixelFormat) { + mappedFormatCode = pixelFormat.fourcc(); + break; + } + } + if (!mappedFormatCode) { + LOG(HAL, Error) << "Failed to get map Android format " + << 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 */ + 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; + } + 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 }); + } + } + + 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; + }; + + 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_; };
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@jmondi.org> --- src/android/camera_device.cpp | 184 ++++++++++++++++++++++++++++++++++ src/android/camera_device.h | 13 +++ 2 files changed, 197 insertions(+)