[{"id":4996,"web_url":"https://patchwork.libcamera.org/comment/4996/","msgid":"<20200604021344.GA7617@pendragon.ideasonboard.com>","date":"2020-06-04T02:13:44","subject":"Re: [libcamera-devel] [PATCH 4/8] android: camera_device: Build\n\tstream configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, May 26, 2020 at 04:22:33PM +0200, Jacopo Mondi wrote:\n> Build the stream configuration map by applying the Android Camera3\n> requested resolutions and formats to the libcamera Camera device.\n> \n> For each required format test a list of required and optional\n> resolutions, construct a map to translate from Android format to the\n> libcamera formats and store the available stream configuration to\n> be provided to the Android framework through static metadata.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/android/camera_device.cpp | 184 ++++++++++++++++++++++++++++++++++\n>  src/android/camera_device.h   |  13 +++\n>  2 files changed, 197 insertions(+)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 69b25ed2f11f..534bfb1df1ef 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -8,6 +8,8 @@\n>  #include \"camera_device.h\"\n>  #include \"camera_ops.h\"\n>  \n> +#include <set>\n> +\n>  #include <libcamera/controls.h>\n>  #include <libcamera/property_ids.h>\n>  \n> @@ -15,9 +17,37 @@\n>  #include \"libcamera/internal/utils.h\"\n>  \n>  #include \"camera_metadata.h\"\n> +#include \"system/graphics.h\"\n>  \n>  using namespace libcamera;\n>  \n> +namespace {\n> +\n> +std::set<Size> camera3Resolutions = {\n\nDoes this need to be a set, shouldn't it be a vector ? And shouldn't it\nbe const ?\n\n> +\t{ 320, 240 },\n> +\t{ 640, 480 },\n> +\t{ 1280, 720 },\n> +\t{ 1920, 1080 }\n> +};\n> +\n> +std::map<int, std::forward_list<uint32_t>> camera3FormatsMap = {\n\nThe map should be const too, and I would make the value an std::vector\ninstead of an std::forward_list, it will be more efficient.\n\n> +\t{ HAL_PIXEL_FORMAT_BLOB, { DRM_FORMAT_MJPEG } },\n> +\t{ HAL_PIXEL_FORMAT_YCbCr_420_888, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },\n\nI have no words to describe how lovely format handling is in Android.\nFor reference, there's some documentation I found in\nplatform/hardware/interfaces/graphics/common/1.0/types.hal while trying\nto figure this out.\n\nI think we'll eventually have to add DRM_FORMAT_YUV420 and\nDRM_FORMAT_YVU420, but that can wait.\n\n> +\t/*\n> +\t * \\todo Translate IMPLEMENTATION_DEFINED inspecting the\n> +\t * gralloc usage flag. For now, copy the YCbCr_420 configuration.\n> +\t */\n> +\t{ HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },\n> +};\n> +\n> +std::map<int32_t, int32_t> camera3ScalerFormatMap = {\n\nconst here too.\n\nIdeally the second type should be\ncamera_metadata_enum_android_scaler_available_formats_t, but I won't\npush for that :-)\n\n> +\t{ HAL_PIXEL_FORMAT_BLOB, ANDROID_SCALER_AVAILABLE_FORMATS_BLOB },\n> +\t{ HAL_PIXEL_FORMAT_YCbCr_420_888, ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888 },\n> +\t{ HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED },\n> +};\n> +\n> +} /* namespace */\n> +\n>  LOG_DECLARE_CATEGORY(HAL);\n>  \n>  /*\n> @@ -100,6 +130,160 @@ int CameraDevice::initialize()\n>  \tif (properties.contains(properties::Rotation))\n>  \t\torientation_ = properties.get(properties::Rotation);\n>  \n> +\tint ret = camera_->acquire();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to temporary acquire the camera\";\n\ns/temporary/temporarily/\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = initializeFormats();\n> +\tcamera_->release();\n> +\treturn ret;\n> +}\n> +\n> +int CameraDevice::initializeFormats()\n> +{\n> +\t/*\n> +\t * Get the maximum output resolutions\n> +\t * \\todo Get this from the camera properties once defined\n> +\t */\n> +\tstd::unique_ptr<CameraConfiguration> cameraConfig =\n> +\t\tcamera_->generateConfiguration({ StillCapture });\n> +\tif (!cameraConfig) {\n> +\t\tLOG(HAL, Error) << \"Failed to get maximum resolution\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tStreamConfiguration &cfg = cameraConfig->at(0);\n> +\n> +\t/*\n> +\t * \\todo JPEG - Adjust the maximum available resolution by\n> +\t * taking the JPEG encoder requirements into account (alignement\n\ns/alignement/alignment/\n\nYou can reflow the comment to 80 columns. Same for other comments below.\n\n> +\t * and aspect ratio).\n> +\t */\n> +\tconst Size maxRes = cfg.size;\n> +\tLOG(HAL, Debug) << \"Maximum supported resolution: \" << maxRes.toString();\n> +\n> +\t/*\n> +\t * Build the list of supported image resolutions.\n> +\t *\n> +\t * The resolutions listed in camera3Resolution are mandatory to be\n> +\t * supported, up to the camera maximum resolution.\n> +\t *\n> +\t * Augment the list by adding resolutions calculated from the camera\n> +\t * maximum one.\n> +\t */\n> +\tstd::set<Size> cameraResolutions;\n\nstd::vector here too ?\n\n> +\tfor (const Size &res : camera3Resolutions) {\n> +\t\tif (res > maxRes)\n> +\t\t\tcontinue;\n> +\n> +\t\tcameraResolutions.insert(res);\n> +\t}\n\nAn alternative is\n\n\tstd::copy_if(camera3Resolutions.begin(), camera3Resolutions.end(),\n\t\t     std::back_inserter(cameraResolutions),\n\t\t     [&](const Size &res) { return res > maxRes; });\n\n> +\n> +\t/* Camera3 specification suggest to add 1/2 and 1/4 max resolution. */\n> +\tfor (unsigned int divider = 2;; divider <<= 1) {\n> +\t\tSize derivedSize{};\n> +\t\tderivedSize.width = maxRes.width / divider;\n> +\t\tderivedSize.height = maxRes.height / divider;\n\nMaybe\n\n\t\tSize derivedSize{\n\t\t\tmaxRes.width / divider,\n\t\t\tmaxRes.height / divider\n\t\t};\n\n? Up to you.\n\n> +\n> +\t\tif (derivedSize.width < 320 ||\n> +\t\t    derivedSize.height < 240)\n> +\t\t\tbreak;\n> +\n> +\t\t/* std::set::insert() guarantees the entry is unique. */\n> +\t\tcameraResolutions.insert(derivedSize);\n\nAh that's why you use std::set. Given the additional complexity of\nstd::set, I wonder if it wouldn't be simpler to still use a vector, and\nwhen done, do\n\n\tstd::sort(cameraResolutions.begin(), cameraResolutions.end());\n\tauto last = std::unique(cameraResolutions.begin(), cameraResolutions.end());\n\tcameraResolutions.erase(last, cameraResolutions.end());\n\nUp to you. For camera3Resolutions I would use a vector, regardless of\nwhat you use for cameraResolutions.\n\n> +\t}\n> +\tcameraResolutions.insert(maxRes);\n> +\n> +\t/*\n> +\t * Build the list of supported camera format.\n\ns/format/formats/\n\n> +\t *\n> +\t * To each Android format a list of compatible libcamera formats is\n> +\t * associated. The first libcamera format that tests successful is added\n> +\t * to the format translation map used when configuring the streams.\n> +\t * It is then tested against the list of supported camera resolutions to\n> +\t * build the stream configuration map reported in the camera static\n> +\t * metadata.\n> +\t */\n> +\tfor (const auto &format : camera3FormatsMap) {\n> +\t\tint androidFormatCode = format.first;\n\nMaybe s/androidFormatCode/androidFormat/ ?\n\n> +\t\tconst std::forward_list<uint32_t> testFormats = format.second;\n\nThis should be a reference.\n\nAnd maybe s/testFormats/libcameraFormats/ ?\n\n> +\n> +\t\t/*\n> +\t\t * Test the libcamera formats that can produce images\n> +\t\t * compatible with the Android's defined format\n\ns/$/./\n\n> +\t\t */\n> +\t\tuint32_t mappedFormatCode = 0;\n\ns/Code// ?\n\n> +\t\tfor (int32_t formatCode : testFormats) {\n> +\t\t\t/*\n> +\t\t\t * \\todo Fixed mapping for JPEG\n> +\t\t\t */\n> +\t\t\tif (androidFormatCode == HAL_PIXEL_FORMAT_BLOB) {\n> +\t\t\t\tmappedFormatCode = DRM_FORMAT_MJPEG;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\t/*\n> +\t\t\t * The stream configuration size can be adjusted,\n> +\t\t\t * not the pixel format.\n> +\t\t\t */\n> +\t\t\tPixelFormat pixelFormat = PixelFormat(formatCode);\n\n\t\t\tPixelFormat pixelFormat{ formatCode };\n\nBut how about storing instances of PixelFormat in camera3FormatsMap, and\nturning mappedFormatCode into a PixelFormat ? You will have a nice\nisValid() function for the check below :-)\n\n> +\t\t\tcfg.pixelFormat = pixelFormat;\n> +\n> +\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> +\t\t\tif (status != CameraConfiguration::Invalid &&\n> +\t\t\t    cfg.pixelFormat == pixelFormat) {\n> +\t\t\t\tmappedFormatCode = pixelFormat.fourcc();\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nWouldn't it be simpler to just check if formatCode is in\ncfg.formats().pixelformats() ? Or is this code meant to work around the\nfact that not all pipeline handlers provide StreamFormats ? In that case\na \\todo should be recorded here to explain the problem, and we should\nfix it in pipeline handlers (possibly reworking the StreamFormats API).\n\n> +\t\t}\n> +\t\tif (!mappedFormatCode) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to get map Android format \"\n\ns/get map/map/ ?\n\n> +\t\t\t\t\t<< utils::hex(androidFormatCode);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Record the mapping and then proceed to generate the\n> +\t\t * stream configuration map, by testing the image resolutions.\n> +\t\t */\n> +\t\tformatsMap_[androidFormatCode] = mappedFormatCode;\n> +\n> +\t\tfor (const Size &res : cameraResolutions) {\n> +\t\t\tPixelFormat pixelFormat = PixelFormat(mappedFormatCode);\n> +\t\t\tcfg.pixelFormat = pixelFormat;\n> +\t\t\tcfg.size = res;\n> +\n> +\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> +\t\t\t/* \\todo Assume we the camera can produce JPEG */\n\ns/we the/the/\n\nNot a very good assumption, that's only for a minority of cameras :-) I\nsuppose we'll handle this with JPEG encoding in the HAL ? Should the\ncomment be reworded to mention that ?\n\n> +\t\t\tif (androidFormatCode != HAL_PIXEL_FORMAT_BLOB &&\n> +\t\t\t    status != CameraConfiguration::Valid)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tauto it = camera3ScalerFormatMap.find(androidFormatCode);\n> +\t\t\tif (it == camera3ScalerFormatMap.end()) {\n> +\t\t\t\tLOG(HAL, Error) << \"Format \" << utils::hex(androidFormatCode)\n> +\t\t\t\t\t\t<< \" has no scaler format associated\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n\nCan this happen ? Would it be enough to have a comment above to warn\nthat camera3FormatsMap and camera3ScalerFormatMap must match ? Or, maybe\nbetter, merge the two maps into one, with the value being a structure\nthat contains both the PixelFormat and the scaler format ? That would\nmake the error impossible.\n\n> +\t\t\tint32_t androidScalerCode = it->second;\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo Add support for input streams. At the moment\n> +\t\t\t * register all stream configurations as output-only.\n> +\t\t\t */\n> +\t\t\tstreamConfigurations_.push_front(\n> +\t\t\t\t{ res, androidScalerCode, false });\n\nI'm curious, why front ? Does Android require sorting formats from\nlargest to smallest ? If so, wouldn't it make sense to sort\ncameraResolutions in the same order ? That would allow turning\nstreamConfigurations_ into a vector and still keep the insertion\nrelatively efficient.\n\n> +\t\t}\n> +\t}\n> +\n> +\tLOG(HAL, Debug) << \"Collected stream configuration map: \";\n> +\tfor (const auto &entry : streamConfigurations_) {\n> +\t\tLOG(HAL, Debug) << \"{ \" << entry.resolution.toString() << \" - \"\n> +\t\t\t\t<< utils::hex(entry.androidScalerCode) << \": \"\n> +\t\t\t\t<< (entry.input ? \"input\" : \"output\") << \" }\";\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index ace9c1b7c929..95bd39f590ab 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,12 +7,15 @@\n>  #ifndef __ANDROID_CAMERA_DEVICE_H__\n>  #define __ANDROID_CAMERA_DEVICE_H__\n>  \n> +#include <forward_list>\n> +#include <map>\n>  #include <memory>\n>  \n>  #include <hardware/camera3.h>\n>  \n>  #include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n> +#include <libcamera/geometry.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -59,6 +62,13 @@ private:\n>  \t\tcamera3_stream_buffer_t *buffers;\n>  \t};\n>  \n> +\tstruct Camera3StreamConfiguration {\n> +\t\tlibcamera::Size resolution;\n> +\t\tint androidScalerCode;\n> +\t\tbool input;\n\nI would drop the input field for now, the HAL will see major reworks\nwhen implementing reprocessing, it will very likely not be kept.\n\n> +\t};\n> +\n> +\tint initializeFormats();\n>  \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>  \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n>  \tstd::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,\n> @@ -75,6 +85,9 @@ private:\n>  \tstd::map<unsigned int, CameraMetadata *> requestTemplates_;\n>  \tconst camera3_callback_ops_t *callbacks_;\n>  \n> +\tstd::forward_list<Camera3StreamConfiguration> streamConfigurations_;\n> +\tstd::map<int, uint32_t> formatsMap_;\n> +\n>  \tint facing_;\n>  \tint orientation_;\n>  };","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C228661012\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  4 Jun 2020 04:14:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 25D0A29B;\n\tThu,  4 Jun 2020 04:14:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UJxZoVF8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591236840;\n\tbh=ydyz5//5kvt0BL5RPK/h6/XYlKyKiroQy6rodBgnNGg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UJxZoVF8kY8vx89j7/0OpevnX4v6cL6oFFjnuYW4QewjwZJu3LXbIA8NrdjI/Ftl/\n\tSSlRrK+nB6+sDXuBTrJWDaQuL1ETAqntIrDg4QTUKEqPDHPBp73W4ZtLoM5hrdy/D+\n\t6yeZBUnAevoGehaArDsa8k1VS5jdS9vklSMukq6I=","Date":"Thu, 4 Jun 2020 05:13:44 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200604021344.GA7617@pendragon.ideasonboard.com>","References":"<20200526142237.407557-1-jacopo@jmondi.org>\n\t<20200526142237.407557-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200526142237.407557-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/8] android: camera_device: Build\n\tstream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 04 Jun 2020 02:14:01 -0000"}},{"id":5008,"web_url":"https://patchwork.libcamera.org/comment/5008/","msgid":"<20200604084441.5qkq5u57yyp7ljj7@uno.localdomain>","date":"2020-06-04T08:44:41","subject":"Re: [libcamera-devel] [PATCH 4/8] android: camera_device: Build\n\tstream configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Jun 04, 2020 at 05:13:44AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, May 26, 2020 at 04:22:33PM +0200, Jacopo Mondi wrote:\n> > Build the stream configuration map by applying the Android Camera3\n> > requested resolutions and formats to the libcamera Camera device.\n> >\n> > For each required format test a list of required and optional\n> > resolutions, construct a map to translate from Android format to the\n> > libcamera formats and store the available stream configuration to\n> > be provided to the Android framework through static metadata.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/android/camera_device.cpp | 184 ++++++++++++++++++++++++++++++++++\n> >  src/android/camera_device.h   |  13 +++\n> >  2 files changed, 197 insertions(+)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 69b25ed2f11f..534bfb1df1ef 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -8,6 +8,8 @@\n> >  #include \"camera_device.h\"\n> >  #include \"camera_ops.h\"\n> >\n> > +#include <set>\n> > +\n> >  #include <libcamera/controls.h>\n> >  #include <libcamera/property_ids.h>\n> >\n> > @@ -15,9 +17,37 @@\n> >  #include \"libcamera/internal/utils.h\"\n> >\n> >  #include \"camera_metadata.h\"\n> > +#include \"system/graphics.h\"\n> >\n> >  using namespace libcamera;\n> >\n> > +namespace {\n> > +\n> > +std::set<Size> camera3Resolutions = {\n>\n> Does this need to be a set, shouldn't it be a vector ? And shouldn't it\n> be const ?\n>\n\nYou found out below here why a set, and it should be const indeed\n\n> > +\t{ 320, 240 },\n> > +\t{ 640, 480 },\n> > +\t{ 1280, 720 },\n> > +\t{ 1920, 1080 }\n> > +};\n> > +\n> > +std::map<int, std::forward_list<uint32_t>> camera3FormatsMap = {\n>\n> The map should be const too, and I would make the value an std::vector\n> instead of an std::forward_list, it will be more efficient.\n>\n\nI chose forward list as I considered more efficient as I only need to\nwalk it in one direction, so I assumed random access support was not\nrequired. But I've now learned vector are much more space efficient,\nso I could change this indeed\n\n> > +\t{ HAL_PIXEL_FORMAT_BLOB, { DRM_FORMAT_MJPEG } },\n> > +\t{ HAL_PIXEL_FORMAT_YCbCr_420_888, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },\n>\n> I have no words to describe how lovely format handling is in Android.\n> For reference, there's some documentation I found in\n> platform/hardware/interfaces/graphics/common/1.0/types.hal while trying\n> to figure this out.\n>\n> I think we'll eventually have to add DRM_FORMAT_YUV420 and\n> DRM_FORMAT_YVU420, but that can wait.\n>\n> > +\t/*\n> > +\t * \\todo Translate IMPLEMENTATION_DEFINED inspecting the\n> > +\t * gralloc usage flag. For now, copy the YCbCr_420 configuration.\n> > +\t */\n> > +\t{ HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, { DRM_FORMAT_NV12, DRM_FORMAT_NV21 } },\n> > +};\n> > +\n> > +std::map<int32_t, int32_t> camera3ScalerFormatMap = {\n>\n> const here too.\n\nack\n\n>\n> Ideally the second type should be\n> camera_metadata_enum_android_scaler_available_formats_t, but I won't\n> push for that :-)\n\nbleah :/\n\n>\n> > +\t{ HAL_PIXEL_FORMAT_BLOB, ANDROID_SCALER_AVAILABLE_FORMATS_BLOB },\n> > +\t{ HAL_PIXEL_FORMAT_YCbCr_420_888, ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888 },\n> > +\t{ HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED },\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> >  LOG_DECLARE_CATEGORY(HAL);\n> >\n> >  /*\n> > @@ -100,6 +130,160 @@ int CameraDevice::initialize()\n> >  \tif (properties.contains(properties::Rotation))\n> >  \t\torientation_ = properties.get(properties::Rotation);\n> >\n> > +\tint ret = camera_->acquire();\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to temporary acquire the camera\";\n>\n> s/temporary/temporarily/\n>\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = initializeFormats();\n> > +\tcamera_->release();\n> > +\treturn ret;\n> > +}\n> > +\n> > +int CameraDevice::initializeFormats()\n> > +{\n> > +\t/*\n> > +\t * Get the maximum output resolutions\n> > +\t * \\todo Get this from the camera properties once defined\n> > +\t */\n> > +\tstd::unique_ptr<CameraConfiguration> cameraConfig =\n> > +\t\tcamera_->generateConfiguration({ StillCapture });\n> > +\tif (!cameraConfig) {\n> > +\t\tLOG(HAL, Error) << \"Failed to get maximum resolution\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tStreamConfiguration &cfg = cameraConfig->at(0);\n> > +\n> > +\t/*\n> > +\t * \\todo JPEG - Adjust the maximum available resolution by\n> > +\t * taking the JPEG encoder requirements into account (alignement\n>\n> s/alignement/alignment/\n>\n> You can reflow the comment to 80 columns. Same for other comments below.\n>\n> > +\t * and aspect ratio).\n> > +\t */\n> > +\tconst Size maxRes = cfg.size;\n> > +\tLOG(HAL, Debug) << \"Maximum supported resolution: \" << maxRes.toString();\n> > +\n> > +\t/*\n> > +\t * Build the list of supported image resolutions.\n> > +\t *\n> > +\t * The resolutions listed in camera3Resolution are mandatory to be\n> > +\t * supported, up to the camera maximum resolution.\n> > +\t *\n> > +\t * Augment the list by adding resolutions calculated from the camera\n> > +\t * maximum one.\n> > +\t */\n> > +\tstd::set<Size> cameraResolutions;\n>\n> std::vector here too ?\n>\n> > +\tfor (const Size &res : camera3Resolutions) {\n> > +\t\tif (res > maxRes)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tcameraResolutions.insert(res);\n> > +\t}\n>\n> An alternative is\n>\n> \tstd::copy_if(camera3Resolutions.begin(), camera3Resolutions.end(),\n> \t\t     std::back_inserter(cameraResolutions),\n> \t\t     [&](const Size &res) { return res > maxRes; });\n>\n\nMuch more C++, I'll take this in\n\n> > +\n> > +\t/* Camera3 specification suggest to add 1/2 and 1/4 max resolution. */\n> > +\tfor (unsigned int divider = 2;; divider <<= 1) {\n> > +\t\tSize derivedSize{};\n> > +\t\tderivedSize.width = maxRes.width / divider;\n> > +\t\tderivedSize.height = maxRes.height / divider;\n>\n> Maybe\n>\n> \t\tSize derivedSize{\n> \t\t\tmaxRes.width / divider,\n> \t\t\tmaxRes.height / divider\n> \t\t};\n>\n> ? Up to you.\n>\n\nNicer, yes\n\n> > +\n> > +\t\tif (derivedSize.width < 320 ||\n> > +\t\t    derivedSize.height < 240)\n> > +\t\t\tbreak;\n> > +\n> > +\t\t/* std::set::insert() guarantees the entry is unique. */\n> > +\t\tcameraResolutions.insert(derivedSize);\n>\n> Ah that's why you use std::set. Given the additional complexity of\n> std::set, I wonder if it wouldn't be simpler to still use a vector, and\n> when done, do\n>\n> \tstd::sort(cameraResolutions.begin(), cameraResolutions.end());\n> \tauto last = std::unique(cameraResolutions.begin(), cameraResolutions.end());\n> \tcameraResolutions.erase(last, cameraResolutions.end());\n\nI can't tell how more complex a set is, I wanted to avoid an\nadditional iteration to clean up the duplicated entries, but as this\nis a one time operation that's probably acceptable.\n\n>\n> Up to you. For camera3Resolutions I would use a vector, regardless of\n> what you use for cameraResolutions.\n>\n\nack\n\n> > +\t}\n> > +\tcameraResolutions.insert(maxRes);\n> > +\n> > +\t/*\n> > +\t * Build the list of supported camera format.\n>\n> s/format/formats/\n>\n> > +\t *\n> > +\t * To each Android format a list of compatible libcamera formats is\n> > +\t * associated. The first libcamera format that tests successful is added\n> > +\t * to the format translation map used when configuring the streams.\n> > +\t * It is then tested against the list of supported camera resolutions to\n> > +\t * build the stream configuration map reported in the camera static\n> > +\t * metadata.\n> > +\t */\n> > +\tfor (const auto &format : camera3FormatsMap) {\n> > +\t\tint androidFormatCode = format.first;\n>\n> Maybe s/androidFormatCode/androidFormat/ ?\n>\n> > +\t\tconst std::forward_list<uint32_t> testFormats = format.second;\n>\n> This should be a reference.\n>\n> And maybe s/testFormats/libcameraFormats/ ?\n>\n> > +\n> > +\t\t/*\n> > +\t\t * Test the libcamera formats that can produce images\n> > +\t\t * compatible with the Android's defined format\n>\n> s/$/./\n>\n> > +\t\t */\n> > +\t\tuint32_t mappedFormatCode = 0;\n>\n> s/Code// ?\n>\n> > +\t\tfor (int32_t formatCode : testFormats) {\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Fixed mapping for JPEG\n> > +\t\t\t */\n> > +\t\t\tif (androidFormatCode == HAL_PIXEL_FORMAT_BLOB) {\n> > +\t\t\t\tmappedFormatCode = DRM_FORMAT_MJPEG;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * The stream configuration size can be adjusted,\n> > +\t\t\t * not the pixel format.\n> > +\t\t\t */\n> > +\t\t\tPixelFormat pixelFormat = PixelFormat(formatCode);\n>\n> \t\t\tPixelFormat pixelFormat{ formatCode };\n>\n> But how about storing instances of PixelFormat in camera3FormatsMap, and\n> turning mappedFormatCode into a PixelFormat ? You will have a nice\n> isValid() function for the check below :-)\n\nI wanted to avoid storing objects and I considered a uin32_t more\nefficient. Not that PixelFormat is an heavyweight class, I think I\ncould store instances directly for a negligible overhead\n\n>\n> > +\t\t\tcfg.pixelFormat = pixelFormat;\n> > +\n> > +\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > +\t\t\tif (status != CameraConfiguration::Invalid &&\n> > +\t\t\t    cfg.pixelFormat == pixelFormat) {\n> > +\t\t\t\tmappedFormatCode = pixelFormat.fourcc();\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n>\n> Wouldn't it be simpler to just check if formatCode is in\n> cfg.formats().pixelformats() ? Or is this code meant to work around the\n> fact that not all pipeline handlers provide StreamFormats ? In that case\n\nYes, very few pipeline handlers provide StreamFormats, and I was not\nsure how appreciated that API is. I can add a \\todo indeed\n\n> a \\todo should be recorded here to explain the problem, and we should\n> fix it in pipeline handlers (possibly reworking the StreamFormats API).\n>\n> > +\t\t}\n> > +\t\tif (!mappedFormatCode) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed to get map Android format \"\n>\n> s/get map/map/ ?\n>\n> > +\t\t\t\t\t<< utils::hex(androidFormatCode);\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Record the mapping and then proceed to generate the\n> > +\t\t * stream configuration map, by testing the image resolutions.\n> > +\t\t */\n> > +\t\tformatsMap_[androidFormatCode] = mappedFormatCode;\n> > +\n> > +\t\tfor (const Size &res : cameraResolutions) {\n> > +\t\t\tPixelFormat pixelFormat = PixelFormat(mappedFormatCode);\n> > +\t\t\tcfg.pixelFormat = pixelFormat;\n> > +\t\t\tcfg.size = res;\n> > +\n> > +\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > +\t\t\t/* \\todo Assume we the camera can produce JPEG */\n>\n> s/we the/the/\n>\n> Not a very good assumption, that's only for a minority of cameras :-) I\n> suppose we'll handle this with JPEG encoding in the HAL ? Should the\n> comment be reworded to mention that ?\n\nYeah, that's what I mean, I need to report JPEG regardless of the fact\nwe can produce it or not, and we can produce it from the Camera or\nfrom an HAL-only stream. I'll expand the comment.\n\n>\n> > +\t\t\tif (androidFormatCode != HAL_PIXEL_FORMAT_BLOB &&\n> > +\t\t\t    status != CameraConfiguration::Valid)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tauto it = camera3ScalerFormatMap.find(androidFormatCode);\n> > +\t\t\tif (it == camera3ScalerFormatMap.end()) {\n> > +\t\t\t\tLOG(HAL, Error) << \"Format \" << utils::hex(androidFormatCode)\n> > +\t\t\t\t\t\t<< \" has no scaler format associated\";\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n>\n> Can this happen ? Would it be enough to have a comment above to warn\n> that camera3FormatsMap and camera3ScalerFormatMap must match ? Or, maybe\n> better, merge the two maps into one, with the value being a structure\n> that contains both the PixelFormat and the scaler format ? That would\n> make the error impossible.\n\nThat was meant to catch errors early when adding new formats. Unifying\nthe maps was an idea I briefly had, then I kept them separate as I was\nnot sure we actually need a map at al as\n\nHAL_PIXEL_FORMAT_*  == ANDROID_SCALER_AVAILABLE_FORMATS_*\n(I know...)\n\nso I thought this could end up by getting rid of the map entirely.\nI'll see how it looks like with a single map.\n\n\n>\n> > +\t\t\tint32_t androidScalerCode = it->second;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Add support for input streams. At the moment\n> > +\t\t\t * register all stream configurations as output-only.\n> > +\t\t\t */\n> > +\t\t\tstreamConfigurations_.push_front(\n> > +\t\t\t\t{ res, androidScalerCode, false });\n>\n> I'm curious, why front ? Does Android require sorting formats from\n> largest to smallest ? If so, wouldn't it make sense to sort\n> cameraResolutions in the same order ? That would allow turning\n> streamConfigurations_ into a vector and still keep the insertion\n> relatively efficient.\n\nThere's no push_back for std::forward_list\n\n>\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tLOG(HAL, Debug) << \"Collected stream configuration map: \";\n> > +\tfor (const auto &entry : streamConfigurations_) {\n> > +\t\tLOG(HAL, Debug) << \"{ \" << entry.resolution.toString() << \" - \"\n> > +\t\t\t\t<< utils::hex(entry.androidScalerCode) << \": \"\n> > +\t\t\t\t<< (entry.input ? \"input\" : \"output\") << \" }\";\n> > +\t}\n> > +\n> >  \treturn 0;\n> >  }\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index ace9c1b7c929..95bd39f590ab 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,12 +7,15 @@\n> >  #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >  #define __ANDROID_CAMERA_DEVICE_H__\n> >\n> > +#include <forward_list>\n> > +#include <map>\n> >  #include <memory>\n> >\n> >  #include <hardware/camera3.h>\n> >\n> >  #include <libcamera/buffer.h>\n> >  #include <libcamera/camera.h>\n> > +#include <libcamera/geometry.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> >\n> > @@ -59,6 +62,13 @@ private:\n> >  \t\tcamera3_stream_buffer_t *buffers;\n> >  \t};\n> >\n> > +\tstruct Camera3StreamConfiguration {\n> > +\t\tlibcamera::Size resolution;\n> > +\t\tint androidScalerCode;\n> > +\t\tbool input;\n>\n> I would drop the input field for now, the HAL will see major reworks\n> when implementing reprocessing, it will very likely not be kept.\n\nAck, at the moment is indeed not used.\n\nThanks\n  j\n\n>\n> > +\t};\n> > +\n> > +\tint initializeFormats();\n> >  \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> >  \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> >  \tstd::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,\n> > @@ -75,6 +85,9 @@ private:\n> >  \tstd::map<unsigned int, CameraMetadata *> requestTemplates_;\n> >  \tconst camera3_callback_ops_t *callbacks_;\n> >\n> > +\tstd::forward_list<Camera3StreamConfiguration> streamConfigurations_;\n> > +\tstd::map<int, uint32_t> formatsMap_;\n> > +\n> >  \tint facing_;\n> >  \tint orientation_;\n> >  };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 108A9603C8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  4 Jun 2020 10:41:19 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 5B98410000B;\n\tThu,  4 Jun 2020 08:41:18 +0000 (UTC)"],"Date":"Thu, 4 Jun 2020 10:44:41 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200604084441.5qkq5u57yyp7ljj7@uno.localdomain>","References":"<20200526142237.407557-1-jacopo@jmondi.org>\n\t<20200526142237.407557-5-jacopo@jmondi.org>\n\t<20200604021344.GA7617@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200604021344.GA7617@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/8] android: camera_device: Build\n\tstream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 04 Jun 2020 08:41:19 -0000"}}]