[{"id":11869,"web_url":"https://patchwork.libcamera.org/comment/11869/","msgid":"<20200805080306.sn6j3xqc4altvhbl@uno.localdomain>","date":"2020-08-05T08:03:06","subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Tue, Aug 04, 2020 at 10:47:11PM +0100, Kieran Bingham wrote:\n> MJPEG streams must be created referencing a libcamera stream.\n> This stream may already be provided by the request configuration,\n> in which case the existing stream is utilised.\n>\n> If no compatible stream is available to encode, a new stream is requested\n> from the libcamera configuration.\n>\n> Signed-off-by: Kieran Bingham \n>\n> ---\n>\n> v3\n> - Add max jpeg size todo\n> - Fix metadata allocations\n> - Use a class member to store the max jpeg buffer size\n> - Remove the scoping level for jpeg blob header\n> - Don't rely on the compressor pointer as a flag\n> - Fix camera metadata size allocations\n> ---\n> src/android/camera_device.cpp | 226 ++++++++++++++++++++++++++++++++--\n> src/android/camera_device.h | 12 ++\n> 2 files changed, 229 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index c529246e115c..433243c3bc56 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -8,6 +8,7 @@\n> #include \"camera_device.h\"\n> #include \"camera_ops.h\"\n>\n> +#include \n> #include \n> #include \n>\n> @@ -22,6 +23,8 @@\n> #include \"camera_metadata.h\"\n> #include \"system/graphics.h\"\n>\n> +#include \"jpeg/compressor_jpeg.h\"\n> +\n> using namespace libcamera;\n>\n> namespace {\n> @@ -129,6 +132,54 @@ const std::map camera3FormatsMap = {\n>\n> LOG_DECLARE_CATEGORY(HAL);\n>\n> +class MappedCamera3Buffer : public MappedBuffer\n> +{\n> +public:\n> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);\n> +};\n> +\n> +MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> +\t\t\t\t\t int flags)\n> +{\n> +\tmaps_.reserve(camera3buffer->numFds);\n> +\terror_ = 0;\n> +\n> +\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> +\t\tif (camera3buffer->data[i] == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n> +\t\tif (length < 0) {\n> +\t\t\terror_ = errno;\n> +\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> +\t\t\t\t camera3buffer->data[i], 0);\n> +\t\tif (address == MAP_FAILED) {\n> +\t\t\terror_ = errno;\n> +\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tmaps_.emplace_back(static_cast(address),\n> +\t\t\t\t static_cast(length));\n> +\t}\n> +\n> +\tvalid_ = error_ == 0;\n> +}\n> +\n> +CameraStream::CameraStream(PixelFormat f, Size s)\n> +\t: index(-1), format(f), size(s), jpeg(nullptr)\n> +{\n> +}\n> +\n> +CameraStream::~CameraStream()\n> +{\n> +\tdelete jpeg;\n> +};\n> +\n> /*\n> * \\struct Camera3RequestDescriptor\n> *\n> @@ -167,6 +218,12 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camer\n> \t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> +\n> +\t/*\n> +\t * \\todo Determine a more accurate value for this during\n> +\t * streamConfiguration.\n> +\t */\n> +\tmax_jpeg_buffer_size_ = 13 << 20; /* 13631488 from USB HAL */\n> }\n>\n> CameraDevice::~CameraDevice()\n> @@ -417,10 +474,10 @@ std::tuple CameraDevice::calculateStaticMetadataSize()\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 50 entries, 647 bytes of static metadata\n> +\t * Currently: 51 entries, 667 bytes of static metadata\n> \t */\n> -\tuint32_t numEntries = 50;\n> -\tuint32_t byteSize = 651;\n> +\tuint32_t numEntries = 51;\n> +\tuint32_t byteSize = 667;\n>\n> \t/*\n> \t * Calculate space occupation in bytes for dynamically built metadata\n> @@ -576,6 +633,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t availableThumbnailSizes.data(),\n> \t\t\t\t availableThumbnailSizes.size());\n>\n> +\t/*\n> +\t * \\todo Calculate the maximum JPEG buffer size by asking the compressor\n> +\t * giving the maximum frame size required.\n> +\t */\n> +\tstaticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &max_jpeg_buffer_size_, 1);\n> +\n> \t/* Sensor static metadata. */\n> \tint32_t pixelArraySize[] = {\n> \t\t2592, 1944,\n> @@ -789,6 +852,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> \t\tANDROID_CONTROL_AVAILABLE_MODES,\n> \t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> +\t\tANDROID_JPEG_MAX_SIZE,\n> \t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> \t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> \t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> @@ -855,6 +919,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tANDROID_SENSOR_EXPOSURE_TIME,\n> \t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> \t\tANDROID_STATISTICS_SCENE_FLICKER,\n> +\t\tANDROID_JPEG_SIZE,\n> +\t\tANDROID_JPEG_QUALITY,\n> +\t\tANDROID_JPEG_ORIENTATION,\n> \t};\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n> \t\t\t\t availableResultKeys.data(),\n> @@ -1016,8 +1083,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t */\n> \tunsigned int streamIndex = 0;\n>\n> +\t/* First handle all non-MJPEG streams. */\n> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> +\t\tSize size(stream->width, stream->height);\n>\n> \t\tPixelFormat format = toPixelFormat(stream->format);\n>\n> @@ -1031,16 +1100,71 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tif (!format.isValid())\n> \t\t\treturn -EINVAL;\n>\n> +\t\tstreams_.emplace_back(format, size);\n> +\t\tstream->priv = static_cast(&streams_[i]);\n> +\n> +\t\t/* Defer handling of MJPEG streams until all others are known. */\n> +\t\tif (format == formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> \t\tStreamConfiguration streamConfiguration;\n>\n> -\t\tstreamConfiguration.size.width = stream->width;\n> -\t\tstreamConfiguration.size.height = stream->height;\n> +\t\tstreamConfiguration.size = size;\n> \t\tstreamConfiguration.pixelFormat = format;\n>\n> \t\tconfig_->addConfiguration(streamConfiguration);\n>\n> \t\tstreams_[i].index = streamIndex++;\n> -\t\tstream->priv = static_cast(&streams_[i]);\n> +\t}\n> +\n> +\t/* Now handle MJPEG streams, adding a new stream if required. */\n> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> +\t\tbool match = false;\n> +\n> +\t\tif (streams_[i].format != formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Search for a compatible stream */\n> +\t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n> +\t\t\tStreamConfiguration &cfg = config_->at(j);\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo The PixelFormat must also be compatible with\n> +\t\t\t * the encoder.\n> +\t\t\t */\n> +\t\t\tif (cfg.size == streams_[i].size) {\n> +\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> +\t\t\t\t\t << \" using libcamera stream \" << j;\n> +\n> +\t\t\t\tmatch = true;\n> +\t\t\t\tstreams_[i].index = j;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Without a compatible match for JPEG encoding we must\n> +\t\t * introduce a new stream to satisfy the request requirements.\n> +\t\t */\n> +\t\tif (!match) {\n> +\t\t\tStreamConfiguration streamConfiguration;\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo: The pixelFormat should be a 'best-fit' choice\n> +\t\t\t * and may require a validation cycle. This is not yet\n> +\t\t\t * handled, and should be considered as part of any\n> +\t\t\t * stream configuration reworks.\n> +\t\t\t */\n> +\t\t\tstreamConfiguration.size.width = stream->width;\n> +\t\t\tstreamConfiguration.size.height = stream->height;\n> +\t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n> +\n> +\t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> +\t\t\t\t << \" for MJPEG support\";\n> +\n> +\t\t\tconfig_->addConfiguration(streamConfiguration);\n> +\t\t\tstreams_[i].index = streamIndex++;\n> +\t\t}\n> \t}\n>\n> \tswitch (config_->validate()) {\n> @@ -1067,6 +1191,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>\n> \t\t/* Use the bufferCount confirmed by the validation process. */\n> \t\tstream->max_buffers = cfg.bufferCount;\n> +\n> +\t\t/*\n> +\t\t * Construct a software compressor for MJPEG streams from the\n> +\t\t * chosen libcamera source stream.\n> +\t\t */\n> +\t\tif (cameraStream->format == formats::MJPEG) {\n> +\t\t\tcameraStream->jpeg = new CompressorJPEG();\n> +\t\t\tcameraStream->jpeg->configure(cfg);\n> +\t\t} else {\n> +\t\t\t/* Either construct this correctly, or use a better interface */\n> +\t\t\tcameraStream->jpeg = nullptr;\n> +\t\t}\n> \t}\n>\n> \t/*\n> @@ -1161,6 +1297,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n> \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n>\n> +\t\t/* Software streams are handled after hardware streams complete. */\n> +\t\tif (cameraStream->format == formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> \t\t/*\n> \t\t * Create a libcamera buffer using the dmabuf descriptors of\n> \t\t * the camera3Buffer for each stream. The FrameBuffer is\n> @@ -1217,8 +1357,76 @@ void CameraDevice::requestComplete(Request *request)\n> \tresultMetadata = getResultMetadata(descriptor->frameNumber,\n> \t\t\t\t\t buffer->metadata().timestamp);\n>\n> -\t/* Prepare to call back the Android camera stack. */\n> +\t/* Handle any JPEG compression */\n> +\tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> +\t\tCameraStream *cameraStream =\n> +\t\t\tstatic_cast(descriptor->buffers[i].stream->priv);\n> +\n> +\t\tif (cameraStream->format != formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> +\t\tCompressor *compressor = cameraStream->jpeg;\n> +\t\t/* Only handle streams with compression enabled. */\n> +\t\tif (!compressor)\n> +\t\t\tcontinue;\n\nSmall nit: Can it happen that format == MJPEG and !compressor ? If it\nhappen, isn't it an error ?\n\nAlso the order of\n Compressor *compressor =\n /* A comment */\n if (!compressor)\n\nis s abit weird\n\n\n> +\n> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> +\t\tStream *stream = streamConfiguration->stream();\n> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tMappedCamera3Buffer mapped(*descriptor->buffers[i].buffer, PROT_READ | PROT_WRITE);\n> +\t\tif (!mapped.isValid()) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to mmap android blob buffer\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tCompressedImage output;\n> +\t\toutput.data = static_cast(mapped.maps()[0].data());\n> +\t\toutput.length = mapped.maps()[0].size();\n> +\n> +\t\tint ret = compressor->compress(buffer, &output);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to compress stream image\";\n> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t}\n>\n> +\t\t/*\n> +\t\t * Fill in the JPEG blob header.\n> +\t\t *\n> +\t\t * The mapped size of the buffer is being returned as\n> +\t\t * substantially larger than the requested JPEG_MAX_SIZE\n> +\t\t * (which is referenced from max_jpeg_buffer_size_). Utilise\n> +\t\t * this static size to ensure the correct offset of the blob is\n> +\t\t * determined.\n> +\t\t *\n> +\t\t * \\todo Investigate if the buffer size mismatch is an issue or\n> +\t\t * expected behaviour.\n> +\t\t */\n> +\t\tuint8_t *resultPtr = mapped.maps()[0].data() +\n> +\t\t\t\t max_jpeg_buffer_size_ -\n> +\t\t\t\t sizeof(struct camera3_jpeg_blob);\n> +\t\tauto *blob = reinterpret_cast(resultPtr);\n> +\t\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> +\t\tblob->jpeg_size = output.length;\n> +\n> +\t\t/* Update the JPEG result Metadata. */\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_SIZE,\n> +\t\t\t\t\t &output.length, 1);\n> +\n> +\t\tconst uint32_t jpeg_quality = 95;\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY,\n> +\t\t\t\t\t &jpeg_quality, 1);\n> +\n> +\t\tconst uint32_t jpeg_orientation = 0;\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> +\t\t\t\t\t &jpeg_orientation, 1);\n> +\t}\n> +\n> +\t/* Prepare to call back the Android camera stack. */\n> \tcamera3_capture_result_t captureResult = {};\n> \tcaptureResult.frame_number = descriptor->frameNumber;\n> \tcaptureResult.num_output_buffers = descriptor->numBuffers;\n> @@ -1298,10 +1506,10 @@ std::unique_ptr CameraDevice::getResultMetadata(int frame_number\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 12 entries, 36 bytes\n> +\t * Currently: 18 entries, 62 bytes\n> \t */\n> \tstd::unique_ptr resultMetadata =\n> -\t\tstd::make_unique(15, 50);\n> +\t\tstd::make_unique(18, 62);\n> \tif (!resultMetadata->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> \t\treturn nullptr;\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 00472c219388..cfec5abeffa1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -23,15 +23,25 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/message.h\"\n>\n> +#include \"jpeg/compressor_jpeg.h\"\n> +\n> class CameraMetadata;\n>\n> struct CameraStream {\n> +\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n> +\t~CameraStream();\n> +\n> \t/*\n> \t * The index of the libcamera StreamConfiguration as added during\n> \t * configureStreams(). A single libcamera Stream may be used to deliver\n> \t * one or more streams to the Android framework.\n> \t */\n> \tunsigned int index;\n> +\n> +\tlibcamera::PixelFormat format;\n> +\tlibcamera::Size size;\n> +\n> +\tCompressorJPEG *jpeg;\n> };\n>\n> class CameraDevice : protected libcamera::Loggable\n> @@ -104,6 +114,8 @@ private:\n>\n> \tint facing_;\n> \tint orientation_;\n> +\n> +\tunsigned int max_jpeg_buffer_size_;\n\nThis looks good! Thanks for sticking to all the pesky discussions on\nv2!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> };\n>\n> #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3A68ABD87A\n\tfor ;\n\tWed, 5 Aug 2020 07:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6D076055E;\n\tWed, 5 Aug 2020 09:59:27 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41E2560554\n\tfor ;\n\tWed, 5 Aug 2020 09:59:26 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id B18B4100009;\n\tWed, 5 Aug 2020 07:59:25 +0000 (UTC)"],"Date":"Wed, 5 Aug 2020 10:03:06 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200805080306.sn6j3xqc4altvhbl@uno.localdomain>","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-14-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804214711.177645-14-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11885,"web_url":"https://patchwork.libcamera.org/comment/11885/","msgid":"<1c2e43dd-8ac9-5187-5035-e9514c83d505@ideasonboard.com>","date":"2020-08-05T13:59:35","subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 05/08/2020 09:03, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Tue, Aug 04, 2020 at 10:47:11PM +0100, Kieran Bingham wrote:\n>> MJPEG streams must be created referencing a libcamera stream.\n>> This stream may already be provided by the request configuration,\n>> in which case the existing stream is utilised.\n>>\n>> If no compatible stream is available to encode, a new stream is requested\n>> from the libcamera configuration.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>>\n>> v3\n>> - Add max jpeg size todo\n>> - Fix metadata allocations\n>> - Use a class member to store the max jpeg buffer size\n>> - Remove the scoping level for jpeg blob header\n>> - Don't rely on the compressor pointer as a flag\n>> - Fix camera metadata size allocations\n>> ---\n>> src/android/camera_device.cpp | 226 ++++++++++++++++++++++++++++++++--\n>> src/android/camera_device.h | 12 ++\n>> 2 files changed, 229 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index c529246e115c..433243c3bc56 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -8,6 +8,7 @@\n>> #include \"camera_device.h\"\n>> #include \"camera_ops.h\"\n>>\n>> +#include \n>> #include \n>> #include \n>>\n>> @@ -22,6 +23,8 @@\n>> #include \"camera_metadata.h\"\n>> #include \"system/graphics.h\"\n>>\n>> +#include \"jpeg/compressor_jpeg.h\"\n>> +\n>> using namespace libcamera;\n>>\n>> namespace {\n>> @@ -129,6 +132,54 @@ const std::map camera3FormatsMap = {\n>>\n>> LOG_DECLARE_CATEGORY(HAL);\n>>\n>> +class MappedCamera3Buffer : public MappedBuffer\n>> +{\n>> +public:\n>> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);\n>> +};\n>> +\n>> +MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n>> +\t\t\t\t\t int flags)\n>> +{\n>> +\tmaps_.reserve(camera3buffer->numFds);\n>> +\terror_ = 0;\n>> +\n>> +\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n>> +\t\tif (camera3buffer->data[i] == -1)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n>> +\t\tif (length < 0) {\n>> +\t\t\terror_ = errno;\n>> +\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n>> +\t\t\t\t camera3buffer->data[i], 0);\n>> +\t\tif (address == MAP_FAILED) {\n>> +\t\t\terror_ = errno;\n>> +\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tmaps_.emplace_back(static_cast(address),\n>> +\t\t\t\t static_cast(length));\n>> +\t}\n>> +\n>> +\tvalid_ = error_ == 0;\n>> +}\n>> +\n>> +CameraStream::CameraStream(PixelFormat f, Size s)\n>> +\t: index(-1), format(f), size(s), jpeg(nullptr)\n>> +{\n>> +}\n>> +\n>> +CameraStream::~CameraStream()\n>> +{\n>> +\tdelete jpeg;\n>> +};\n>> +\n>> /*\n>> * \\struct Camera3RequestDescriptor\n>> *\n>> @@ -167,6 +218,12 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camer\n>> \t facing_(CAMERA_FACING_FRONT), orientation_(0)\n>> {\n>> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n>> +\n>> +\t/*\n>> +\t * \\todo Determine a more accurate value for this during\n>> +\t * streamConfiguration.\n>> +\t */\n>> +\tmax_jpeg_buffer_size_ = 13 << 20; /* 13631488 from USB HAL */\n>> }\n>>\n>> CameraDevice::~CameraDevice()\n>> @@ -417,10 +474,10 @@ std::tuple CameraDevice::calculateStaticMetadataSize()\n>> {\n>> \t/*\n>> \t * \\todo Keep this in sync with the actual number of entries.\n>> -\t * Currently: 50 entries, 647 bytes of static metadata\n>> +\t * Currently: 51 entries, 667 bytes of static metadata\n>> \t */\n>> -\tuint32_t numEntries = 50;\n>> -\tuint32_t byteSize = 651;\n>> +\tuint32_t numEntries = 51;\n>> +\tuint32_t byteSize = 667;\n>>\n>> \t/*\n>> \t * Calculate space occupation in bytes for dynamically built metadata\n>> @@ -576,6 +633,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\t\t\t availableThumbnailSizes.data(),\n>> \t\t\t\t availableThumbnailSizes.size());\n>>\n>> +\t/*\n>> +\t * \\todo Calculate the maximum JPEG buffer size by asking the compressor\n>> +\t * giving the maximum frame size required.\n>> +\t */\n>> +\tstaticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &max_jpeg_buffer_size_, 1);\n>> +\n>> \t/* Sensor static metadata. */\n>> \tint32_t pixelArraySize[] = {\n>> \t\t2592, 1944,\n>> @@ -789,6 +852,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>> \t\tANDROID_CONTROL_AVAILABLE_MODES,\n>> \t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>> +\t\tANDROID_JPEG_MAX_SIZE,\n>> \t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>> \t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>> \t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>> @@ -855,6 +919,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\tANDROID_SENSOR_EXPOSURE_TIME,\n>> \t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n>> \t\tANDROID_STATISTICS_SCENE_FLICKER,\n>> +\t\tANDROID_JPEG_SIZE,\n>> +\t\tANDROID_JPEG_QUALITY,\n>> +\t\tANDROID_JPEG_ORIENTATION,\n>> \t};\n>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n>> \t\t\t\t availableResultKeys.data(),\n>> @@ -1016,8 +1083,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t */\n>> \tunsigned int streamIndex = 0;\n>>\n>> +\t/* First handle all non-MJPEG streams. */\n>> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>> \t\tcamera3_stream_t *stream = stream_list->streams[i];\n>> +\t\tSize size(stream->width, stream->height);\n>>\n>> \t\tPixelFormat format = toPixelFormat(stream->format);\n>>\n>> @@ -1031,16 +1100,71 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\tif (!format.isValid())\n>> \t\t\treturn -EINVAL;\n>>\n>> +\t\tstreams_.emplace_back(format, size);\n>> +\t\tstream->priv = static_cast(&streams_[i]);\n>> +\n>> +\t\t/* Defer handling of MJPEG streams until all others are known. */\n>> +\t\tif (format == formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> \t\tStreamConfiguration streamConfiguration;\n>>\n>> -\t\tstreamConfiguration.size.width = stream->width;\n>> -\t\tstreamConfiguration.size.height = stream->height;\n>> +\t\tstreamConfiguration.size = size;\n>> \t\tstreamConfiguration.pixelFormat = format;\n>>\n>> \t\tconfig_->addConfiguration(streamConfiguration);\n>>\n>> \t\tstreams_[i].index = streamIndex++;\n>> -\t\tstream->priv = static_cast(&streams_[i]);\n>> +\t}\n>> +\n>> +\t/* Now handle MJPEG streams, adding a new stream if required. */\n>> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n>> +\t\tbool match = false;\n>> +\n>> +\t\tif (streams_[i].format != formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t/* Search for a compatible stream */\n>> +\t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n>> +\t\t\tStreamConfiguration &cfg = config_->at(j);\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * \\todo The PixelFormat must also be compatible with\n>> +\t\t\t * the encoder.\n>> +\t\t\t */\n>> +\t\t\tif (cfg.size == streams_[i].size) {\n>> +\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n>> +\t\t\t\t\t << \" using libcamera stream \" << j;\n>> +\n>> +\t\t\t\tmatch = true;\n>> +\t\t\t\tstreams_[i].index = j;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Without a compatible match for JPEG encoding we must\n>> +\t\t * introduce a new stream to satisfy the request requirements.\n>> +\t\t */\n>> +\t\tif (!match) {\n>> +\t\t\tStreamConfiguration streamConfiguration;\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * \\todo: The pixelFormat should be a 'best-fit' choice\n>> +\t\t\t * and may require a validation cycle. This is not yet\n>> +\t\t\t * handled, and should be considered as part of any\n>> +\t\t\t * stream configuration reworks.\n>> +\t\t\t */\n>> +\t\t\tstreamConfiguration.size.width = stream->width;\n>> +\t\t\tstreamConfiguration.size.height = stream->height;\n>> +\t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n>> +\n>> +\t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n>> +\t\t\t\t << \" for MJPEG support\";\n>> +\n>> +\t\t\tconfig_->addConfiguration(streamConfiguration);\n>> +\t\t\tstreams_[i].index = streamIndex++;\n>> +\t\t}\n>> \t}\n>>\n>> \tswitch (config_->validate()) {\n>> @@ -1067,6 +1191,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>>\n>> \t\t/* Use the bufferCount confirmed by the validation process. */\n>> \t\tstream->max_buffers = cfg.bufferCount;\n>> +\n>> +\t\t/*\n>> +\t\t * Construct a software compressor for MJPEG streams from the\n>> +\t\t * chosen libcamera source stream.\n>> +\t\t */\n>> +\t\tif (cameraStream->format == formats::MJPEG) {\n>> +\t\t\tcameraStream->jpeg = new CompressorJPEG();\n>> +\t\t\tcameraStream->jpeg->configure(cfg);\n>> +\t\t} else {\n>> +\t\t\t/* Either construct this correctly, or use a better interface */\n>> +\t\t\tcameraStream->jpeg = nullptr;\n>> +\t\t}\n>> \t}\n>>\n>> \t/*\n>> @@ -1161,6 +1297,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n>> \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n>>\n>> +\t\t/* Software streams are handled after hardware streams complete. */\n>> +\t\tif (cameraStream->format == formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> \t\t/*\n>> \t\t * Create a libcamera buffer using the dmabuf descriptors of\n>> \t\t * the camera3Buffer for each stream. The FrameBuffer is\n>> @@ -1217,8 +1357,76 @@ void CameraDevice::requestComplete(Request *request)\n>> \tresultMetadata = getResultMetadata(descriptor->frameNumber,\n>> \t\t\t\t\t buffer->metadata().timestamp);\n>>\n>> -\t/* Prepare to call back the Android camera stack. */\n>> +\t/* Handle any JPEG compression */\n>> +\tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n>> +\t\tCameraStream *cameraStream =\n>> +\t\t\tstatic_cast(descriptor->buffers[i].stream->priv);\n>> +\n>> +\t\tif (cameraStream->format != formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCompressor *compressor = cameraStream->jpeg;\n>> +\t\t/* Only handle streams with compression enabled. */\n>> +\t\tif (!compressor)\n>> +\t\t\tcontinue;\n> \n> Small nit: Can it happen that format == MJPEG and !compressor ? If it\n> happen, isn't it an error ?\n> \n\nIt shouldn't happen; until someone starts doing work on different\ncompressors ... so lets put an error path in.\n\n\n> Also the order of\n> Compressor *compressor =\n> /* A comment */\n> if (!compressor)\n> \n> is s abit weird\n\n\nI can remove it.\n\n\n\n\n>> +\n>> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n>> +\t\tStream *stream = streamConfiguration->stream();\n>> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n>> +\t\tif (!buffer) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> +\t\tMappedCamera3Buffer mapped(*descriptor->buffers[i].buffer, PROT_READ | PROT_WRITE);\n>> +\t\tif (!mapped.isValid()) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to mmap android blob buffer\";\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> +\t\tCompressedImage output;\n>> +\t\toutput.data = static_cast(mapped.maps()[0].data());\n>> +\t\toutput.length = mapped.maps()[0].size();\n>> +\n>> +\t\tint ret = compressor->compress(buffer, &output);\n>> +\t\tif (ret) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to compress stream image\";\n>> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n>> +\t\t}\n>>\n>> +\t\t/*\n>> +\t\t * Fill in the JPEG blob header.\n>> +\t\t *\n>> +\t\t * The mapped size of the buffer is being returned as\n>> +\t\t * substantially larger than the requested JPEG_MAX_SIZE\n>> +\t\t * (which is referenced from max_jpeg_buffer_size_). Utilise\n>> +\t\t * this static size to ensure the correct offset of the blob is\n>> +\t\t * determined.\n>> +\t\t *\n>> +\t\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> +\t\t * expected behaviour.\n>> +\t\t */\n>> +\t\tuint8_t *resultPtr = mapped.maps()[0].data() +\n>> +\t\t\t\t max_jpeg_buffer_size_ -\n>> +\t\t\t\t sizeof(struct camera3_jpeg_blob);\n>> +\t\tauto *blob = reinterpret_cast(resultPtr);\n>> +\t\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> +\t\tblob->jpeg_size = output.length;\n>> +\n>> +\t\t/* Update the JPEG result Metadata. */\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_SIZE,\n>> +\t\t\t\t\t &output.length, 1);\n>> +\n>> +\t\tconst uint32_t jpeg_quality = 95;\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY,\n>> +\t\t\t\t\t &jpeg_quality, 1);\n>> +\n>> +\t\tconst uint32_t jpeg_orientation = 0;\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n>> +\t\t\t\t\t &jpeg_orientation, 1);\n>> +\t}\n>> +\n>> +\t/* Prepare to call back the Android camera stack. */\n>> \tcamera3_capture_result_t captureResult = {};\n>> \tcaptureResult.frame_number = descriptor->frameNumber;\n>> \tcaptureResult.num_output_buffers = descriptor->numBuffers;\n>> @@ -1298,10 +1506,10 @@ std::unique_ptr CameraDevice::getResultMetadata(int frame_number\n>> {\n>> \t/*\n>> \t * \\todo Keep this in sync with the actual number of entries.\n>> -\t * Currently: 12 entries, 36 bytes\n>> +\t * Currently: 18 entries, 62 bytes\n>> \t */\n>> \tstd::unique_ptr resultMetadata =\n>> -\t\tstd::make_unique(15, 50);\n>> +\t\tstd::make_unique(18, 62);\n>> \tif (!resultMetadata->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n>> \t\treturn nullptr;\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 00472c219388..cfec5abeffa1 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -23,15 +23,25 @@\n>> #include \"libcamera/internal/log.h\"\n>> #include \"libcamera/internal/message.h\"\n>>\n>> +#include \"jpeg/compressor_jpeg.h\"\n>> +\n>> class CameraMetadata;\n>>\n>> struct CameraStream {\n>> +\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n>> +\t~CameraStream();\n>> +\n>> \t/*\n>> \t * The index of the libcamera StreamConfiguration as added during\n>> \t * configureStreams(). A single libcamera Stream may be used to deliver\n>> \t * one or more streams to the Android framework.\n>> \t */\n>> \tunsigned int index;\n>> +\n>> +\tlibcamera::PixelFormat format;\n>> +\tlibcamera::Size size;\n>> +\n>> +\tCompressorJPEG *jpeg;\n>> };\n>>\n>> class CameraDevice : protected libcamera::Loggable\n>> @@ -104,6 +114,8 @@ private:\n>>\n>> \tint facing_;\n>> \tint orientation_;\n>> +\n>> +\tunsigned int max_jpeg_buffer_size_;\n> \n> This looks good! Thanks for sticking to all the pesky discussions on\n> v2!\n> \n> Reviewed-by: Jacopo Mondi \n\nThanks!\n\n> \n> Thanks\n> j\n> \n>> };\n>>\n>> #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n>> --\n>> 2.25.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C47D3BD87A\n\tfor ;\n\tWed, 5 Aug 2020 13:59:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5872C60599;\n\tWed, 5 Aug 2020 15:59:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D919D6039D\n\tfor ;\n\tWed, 5 Aug 2020 15:59:38 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3F95A2C0;\n\tWed, 5 Aug 2020 15:59:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sia7s2DP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596635978;\n\tbh=s4JwiFwJxFOMyp+LOJJIOU+Iee3K5Ft6hRG340dM4co=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=sia7s2DPw5yUrDz4EAJRp5Awr6eTGkkNfgQhkIXkQ/d++CF2VuXfCkLgkTuMOSrbV\n\tLjEkS3890U5jr2qWeDteJ97coE7DWHKz0PaTky8PNWnHnqmx23Q+gmBDC+Uk8biLY4\n\tmZvMJ4gCCkWcnZHESamV+WPuJMEjX56GJB3dW23k=","To":"Jacopo Mondi ","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-14-kieran.bingham@ideasonboard.com>\n\t<20200805080306.sn6j3xqc4altvhbl@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<1c2e43dd-8ac9-5187-5035-e9514c83d505@ideasonboard.com>","Date":"Wed, 5 Aug 2020 14:59:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200805080306.sn6j3xqc4altvhbl@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11886,"web_url":"https://patchwork.libcamera.org/comment/11886/","msgid":"<20200805142542.GF6751@pendragon.ideasonboard.com>","date":"2020-08-05T14:25:42","subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Aug 04, 2020 at 10:47:11PM +0100, Kieran Bingham wrote:\n> MJPEG streams must be created referencing a libcamera stream.\n> This stream may already be provided by the request configuration,\n> in which case the existing stream is utilised.\n> \n> If no compatible stream is available to encode, a new stream is requested\n> from the libcamera configuration.\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> \n> v3\n> - Add max jpeg size todo\n> - Fix metadata allocations\n> - Use a class member to store the max jpeg buffer size\n> - Remove the scoping level for jpeg blob header\n> - Don't rely on the compressor pointer as a flag\n> - Fix camera metadata size allocations\n> ---\n> src/android/camera_device.cpp | 226 ++++++++++++++++++++++++++++++++--\n> src/android/camera_device.h | 12 ++\n> 2 files changed, 229 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index c529246e115c..433243c3bc56 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -8,6 +8,7 @@\n> #include \"camera_device.h\"\n> #include \"camera_ops.h\"\n> \n> +#include \n> #include \n> #include \n> \n> @@ -22,6 +23,8 @@\n> #include \"camera_metadata.h\"\n> #include \"system/graphics.h\"\n> \n> +#include \"jpeg/compressor_jpeg.h\"\n> +\n> using namespace libcamera;\n> \n> namespace {\n> @@ -129,6 +132,54 @@ const std::map camera3FormatsMap = {\n> \n> LOG_DECLARE_CATEGORY(HAL);\n> \n> +class MappedCamera3Buffer : public MappedBuffer\n> +{\n> +public:\n> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);\n> +};\n> +\n> +MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> +\t\t\t\t\t int flags)\n> +{\n> +\tmaps_.reserve(camera3buffer->numFds);\n> +\terror_ = 0;\n> +\n> +\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> +\t\tif (camera3buffer->data[i] == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n> +\t\tif (length < 0) {\n> +\t\t\terror_ = errno;\n\nShould this be -errno ? I think the documentation of the error_ field\nstates it's negative.\n\n> +\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> +\t\t\t\t camera3buffer->data[i], 0);\n> +\t\tif (address == MAP_FAILED) {\n> +\t\t\terror_ = errno;\n> +\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tmaps_.emplace_back(static_cast(address),\n> +\t\t\t\t static_cast(length));\n> +\t}\n> +\n> +\tvalid_ = error_ == 0;\n> +}\n> +\n> +CameraStream::CameraStream(PixelFormat f, Size s)\n> +\t: index(-1), format(f), size(s), jpeg(nullptr)\n> +{\n> +}\n> +\n> +CameraStream::~CameraStream()\n> +{\n> +\tdelete jpeg;\n> +};\n> +\n> /*\n> * \\struct Camera3RequestDescriptor\n> *\n> @@ -167,6 +218,12 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camer\n> \t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> +\n> +\t/*\n> +\t * \\todo Determine a more accurate value for this during\n> +\t * streamConfiguration.\n> +\t */\n> +\tmax_jpeg_buffer_size_ = 13 << 20; /* 13631488 from USB HAL */\n\nThere's a function in turbojpeg to retrieve the maximum size,\ntjBufSize(). It essentially returns width * height * 4 plus some margin\nthough, so be prepared for big buffers :-)\n\nAs the plan seems to be to use the libjpeg RAW API, not the libturbojpeg\nAPI, we may want to reimplement this function. It should be fairly\nsimple.\n\nAnother item for your todo list, the maximum size should be queried from\nthe Compressor for a given PixelFormat and Size.\n\n> }\n> \n> CameraDevice::~CameraDevice()\n> @@ -417,10 +474,10 @@ std::tuple CameraDevice::calculateStaticMetadataSize()\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 50 entries, 647 bytes of static metadata\n> +\t * Currently: 51 entries, 667 bytes of static metadata\n> \t */\n> -\tuint32_t numEntries = 50;\n> -\tuint32_t byteSize = 651;\n> +\tuint32_t numEntries = 51;\n> +\tuint32_t byteSize = 667;\n> \n> \t/*\n> \t * Calculate space occupation in bytes for dynamically built metadata\n> @@ -576,6 +633,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t availableThumbnailSizes.data(),\n> \t\t\t\t availableThumbnailSizes.size());\n> \n> +\t/*\n> +\t * \\todo Calculate the maximum JPEG buffer size by asking the compressor\n> +\t * giving the maximum frame size required.\n> +\t */\n\nAh, there we go :-)\n\n> +\tstaticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &max_jpeg_buffer_size_, 1);\n> +\n> \t/* Sensor static metadata. */\n> \tint32_t pixelArraySize[] = {\n> \t\t2592, 1944,\n> @@ -789,6 +852,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> \t\tANDROID_CONTROL_AVAILABLE_MODES,\n> \t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> +\t\tANDROID_JPEG_MAX_SIZE,\n> \t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> \t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> \t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> @@ -855,6 +919,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tANDROID_SENSOR_EXPOSURE_TIME,\n> \t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> \t\tANDROID_STATISTICS_SCENE_FLICKER,\n> +\t\tANDROID_JPEG_SIZE,\n> +\t\tANDROID_JPEG_QUALITY,\n> +\t\tANDROID_JPEG_ORIENTATION,\n> \t};\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n> \t\t\t\t availableResultKeys.data(),\n> @@ -1016,8 +1083,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t */\n> \tunsigned int streamIndex = 0;\n> \n> +\t/* First handle all non-MJPEG streams. */\n> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> +\t\tSize size(stream->width, stream->height);\n> \n> \t\tPixelFormat format = toPixelFormat(stream->format);\n> \n> @@ -1031,16 +1100,71 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tif (!format.isValid())\n> \t\t\treturn -EINVAL;\n> \n> +\t\tstreams_.emplace_back(format, size);\n> +\t\tstream->priv = static_cast(&streams_[i]);\n> +\n> +\t\t/* Defer handling of MJPEG streams until all others are known. */\n> +\t\tif (format == formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> \t\tStreamConfiguration streamConfiguration;\n> \n> -\t\tstreamConfiguration.size.width = stream->width;\n> -\t\tstreamConfiguration.size.height = stream->height;\n> +\t\tstreamConfiguration.size = size;\n> \t\tstreamConfiguration.pixelFormat = format;\n> \n> \t\tconfig_->addConfiguration(streamConfiguration);\n> \n> \t\tstreams_[i].index = streamIndex++;\n> -\t\tstream->priv = static_cast(&streams_[i]);\n> +\t}\n> +\n> +\t/* Now handle MJPEG streams, adding a new stream if required. */\n> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> +\t\tbool match = false;\n> +\n> +\t\tif (streams_[i].format != formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Search for a compatible stream */\n> +\t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n> +\t\t\tStreamConfiguration &cfg = config_->at(j);\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo The PixelFormat must also be compatible with\n> +\t\t\t * the encoder.\n\nWe will likely also need to implement software down-scaling (and\npossibly even format conversion) support if the hardware can't provide\nus with an additional stream that matches the size and pixel format\nneeded for JPEG compression. Can you capture this in a todo as well ? I\nthink the code will need to be refactored to handle those additional\nstreams in a more generic way, not just for JPEG.\n\n> +\t\t\t */\n> +\t\t\tif (cfg.size == streams_[i].size) {\n> +\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> +\t\t\t\t\t << \" using libcamera stream \" << j;\n> +\n> +\t\t\t\tmatch = true;\n> +\t\t\t\tstreams_[i].index = j;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Without a compatible match for JPEG encoding we must\n> +\t\t * introduce a new stream to satisfy the request requirements.\n> +\t\t */\n> +\t\tif (!match) {\n> +\t\t\tStreamConfiguration streamConfiguration;\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo: The pixelFormat should be a 'best-fit' choice\n> +\t\t\t * and may require a validation cycle. This is not yet\n> +\t\t\t * handled, and should be considered as part of any\n> +\t\t\t * stream configuration reworks.\n> +\t\t\t */\n> +\t\t\tstreamConfiguration.size.width = stream->width;\n> +\t\t\tstreamConfiguration.size.height = stream->height;\n> +\t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n> +\n> +\t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> +\t\t\t\t << \" for MJPEG support\";\n> +\n> +\t\t\tconfig_->addConfiguration(streamConfiguration);\n> +\t\t\tstreams_[i].index = streamIndex++;\n> +\t\t}\n> \t}\n> \n> \tswitch (config_->validate()) {\n> @@ -1067,6 +1191,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \n> \t\t/* Use the bufferCount confirmed by the validation process. */\n> \t\tstream->max_buffers = cfg.bufferCount;\n> +\n> +\t\t/*\n> +\t\t * Construct a software compressor for MJPEG streams from the\n> +\t\t * chosen libcamera source stream.\n> +\t\t */\n> +\t\tif (cameraStream->format == formats::MJPEG) {\n> +\t\t\tcameraStream->jpeg = new CompressorJPEG();\n> +\t\t\tcameraStream->jpeg->configure(cfg);\n> +\t\t} else {\n> +\t\t\t/* Either construct this correctly, or use a better interface */\n\nI'm not sure what you mean here.\n\n> +\t\t\tcameraStream->jpeg = nullptr;\n> +\t\t}\n> \t}\n> \n> \t/*\n> @@ -1161,6 +1297,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n> \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n> \n> +\t\t/* Software streams are handled after hardware streams complete. */\n> +\t\tif (cameraStream->format == formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> \t\t/*\n> \t\t * Create a libcamera buffer using the dmabuf descriptors of\n> \t\t * the camera3Buffer for each stream. The FrameBuffer is\n> @@ -1217,8 +1357,76 @@ void CameraDevice::requestComplete(Request *request)\n> \tresultMetadata = getResultMetadata(descriptor->frameNumber,\n> \t\t\t\t\t buffer->metadata().timestamp);\n> \n> -\t/* Prepare to call back the Android camera stack. */\n> +\t/* Handle any JPEG compression */\n\ns/compression/compression./\n\n> +\tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> +\t\tCameraStream *cameraStream =\n> +\t\t\tstatic_cast(descriptor->buffers[i].stream->priv);\n> +\n> +\t\tif (cameraStream->format != formats::MJPEG)\n> +\t\t\tcontinue;\n> +\n> +\t\tCompressor *compressor = cameraStream->jpeg;\n> +\t\t/* Only handle streams with compression enabled. */\n> +\t\tif (!compressor)\n> +\t\t\tcontinue;\n> +\n> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> +\t\tStream *stream = streamConfiguration->stream();\n> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n\nCan you add a todo to mention that the code below (buffer mapping and\ncompression) needs to be moved to a separate thread ?\n\n> +\t\tMappedCamera3Buffer mapped(*descriptor->buffers[i].buffer, PROT_READ | PROT_WRITE);\n\nLine wrap ?\n\n> +\t\tif (!mapped.isValid()) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to mmap android blob buffer\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tCompressedImage output;\n> +\t\toutput.data = static_cast(mapped.maps()[0].data());\n> +\t\toutput.length = mapped.maps()[0].size();\n> +\n> +\t\tint ret = compressor->compress(buffer, &output);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to compress stream image\";\n> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n\nDo we need to proceed to adding the metadata entries then ? Or should we\ncontinue to the next loop iteration ?\n\n> +\t\t}\n> \n> +\t\t/*\n> +\t\t * Fill in the JPEG blob header.\n> +\t\t *\n> +\t\t * The mapped size of the buffer is being returned as\n> +\t\t * substantially larger than the requested JPEG_MAX_SIZE\n> +\t\t * (which is referenced from max_jpeg_buffer_size_). Utilise\n> +\t\t * this static size to ensure the correct offset of the blob is\n> +\t\t * determined.\n> +\t\t *\n> +\t\t * \\todo Investigate if the buffer size mismatch is an issue or\n> +\t\t * expected behaviour.\n> +\t\t */\n> +\t\tuint8_t *resultPtr = mapped.maps()[0].data() +\n> +\t\t\t\t max_jpeg_buffer_size_ -\n> +\t\t\t\t sizeof(struct camera3_jpeg_blob);\n> +\t\tauto *blob = reinterpret_cast(resultPtr);\n> +\t\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> +\t\tblob->jpeg_size = output.length;\n> +\n> +\t\t/* Update the JPEG result Metadata. */\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_SIZE,\n> +\t\t\t\t\t &output.length, 1);\n> +\n> +\t\tconst uint32_t jpeg_quality = 95;\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY,\n> +\t\t\t\t\t &jpeg_quality, 1);\n> +\n> +\t\tconst uint32_t jpeg_orientation = 0;\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> +\t\t\t\t\t &jpeg_orientation, 1);\n> +\t}\n> +\n> +\t/* Prepare to call back the Android camera stack. */\n> \tcamera3_capture_result_t captureResult = {};\n> \tcaptureResult.frame_number = descriptor->frameNumber;\n> \tcaptureResult.num_output_buffers = descriptor->numBuffers;\n> @@ -1298,10 +1506,10 @@ std::unique_ptr CameraDevice::getResultMetadata(int frame_number\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 12 entries, 36 bytes\n> +\t * Currently: 18 entries, 62 bytes\n> \t */\n> \tstd::unique_ptr resultMetadata =\n> -\t\tstd::make_unique(15, 50);\n> +\t\tstd::make_unique(18, 62);\n> \tif (!resultMetadata->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> \t\treturn nullptr;\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 00472c219388..cfec5abeffa1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -23,15 +23,25 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/message.h\"\n> \n> +#include \"jpeg/compressor_jpeg.h\"\n> +\n> class CameraMetadata;\n> \n> struct CameraStream {\n\nI'd turn this into a class when you get the chance.\n\n> +\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n> +\t~CameraStream();\n> +\n> \t/*\n> \t * The index of the libcamera StreamConfiguration as added during\n> \t * configureStreams(). A single libcamera Stream may be used to deliver\n> \t * one or more streams to the Android framework.\n> \t */\n> \tunsigned int index;\n> +\n> +\tlibcamera::PixelFormat format;\n> +\tlibcamera::Size size;\n> +\n> +\tCompressorJPEG *jpeg;\n\nShould this be a pointer to Compressor, not CompressorJPEG ?\n\n> };\n> \n> class CameraDevice : protected libcamera::Loggable\n> @@ -104,6 +114,8 @@ private:\n> \n> \tint facing_;\n> \tint orientation_;\n> +\n> +\tunsigned int max_jpeg_buffer_size_;\n\nmaxJpegBufferSize_ ?\n\n> };\n> \n> #endif /* __ANDROID_CAMERA_DEVICE_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 129E2BD86F\n\tfor ;\n\tWed, 5 Aug 2020 14:25:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95B46605AC;\n\tWed, 5 Aug 2020 16:25:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E98986039D\n\tfor ;\n\tWed, 5 Aug 2020 16:25:54 +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 468C02C0;\n\tWed, 5 Aug 2020 16:25:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZxOSBWrm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596637554;\n\tbh=h8l4q+xUmDvW3kVKt/QijL5yT1nWmHWcrAQ/WNnw3hw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZxOSBWrmlfj1wiZoENt6yZjjzH10TF2iZP5kHAnZAe4EDn+QgxQuf6NDhz4GoVNQa\n\t/k2fv6UIbg9Z8KFHmLr0Mkh0jsHsl9qwOK24uoG7vcrhO5VY9HaR7HzLaKbwyKI+Lo\n\tnTAM2tA8GKgOE+9TxjD8QK568WKmD0jQiqgngBhY=","Date":"Wed, 5 Aug 2020 17:25:42 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200805142542.GF6751@pendragon.ideasonboard.com>","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-14-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804214711.177645-14-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11893,"web_url":"https://patchwork.libcamera.org/comment/11893/","msgid":"<716da943-be06-1ab1-0163-243a59f5238a@ideasonboard.com>","date":"2020-08-05T15:10:15","subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 05/08/2020 15:25, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 04, 2020 at 10:47:11PM +0100, Kieran Bingham wrote:\n>> MJPEG streams must be created referencing a libcamera stream.\n>> This stream may already be provided by the request configuration,\n>> in which case the existing stream is utilised.\n>>\n>> If no compatible stream is available to encode, a new stream is requested\n>> from the libcamera configuration.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>>\n>> v3\n>> - Add max jpeg size todo\n>> - Fix metadata allocations\n>> - Use a class member to store the max jpeg buffer size\n>> - Remove the scoping level for jpeg blob header\n>> - Don't rely on the compressor pointer as a flag\n>> - Fix camera metadata size allocations\n>> ---\n>> src/android/camera_device.cpp | 226 ++++++++++++++++++++++++++++++++--\n>> src/android/camera_device.h | 12 ++\n>> 2 files changed, 229 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index c529246e115c..433243c3bc56 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -8,6 +8,7 @@\n>> #include \"camera_device.h\"\n>> #include \"camera_ops.h\"\n>> \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -22,6 +23,8 @@\n>> #include \"camera_metadata.h\"\n>> #include \"system/graphics.h\"\n>> \n>> +#include \"jpeg/compressor_jpeg.h\"\n>> +\n>> using namespace libcamera;\n>> \n>> namespace {\n>> @@ -129,6 +132,54 @@ const std::map camera3FormatsMap = {\n>> \n>> LOG_DECLARE_CATEGORY(HAL);\n>> \n>> +class MappedCamera3Buffer : public MappedBuffer\n>> +{\n>> +public:\n>> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags);\n>> +};\n>> +\n>> +MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n>> +\t\t\t\t\t int flags)\n>> +{\n>> +\tmaps_.reserve(camera3buffer->numFds);\n>> +\terror_ = 0;\n>> +\n>> +\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n>> +\t\tif (camera3buffer->data[i] == -1)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n>> +\t\tif (length < 0) {\n>> +\t\t\terror_ = errno;\n> \n> Should this be -errno ? I think the documentation of the error_ field\n> states it's negative.\n\nYes.\n\n> \n>> +\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n>> +\t\t\t\t camera3buffer->data[i], 0);\n>> +\t\tif (address == MAP_FAILED) {\n>> +\t\t\terror_ = errno;\n\nSame here.\n\n>> +\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tmaps_.emplace_back(static_cast(address),\n>> +\t\t\t\t static_cast(length));\n>> +\t}\n>> +\n>> +\tvalid_ = error_ == 0;\n>> +}\n>> +\n>> +CameraStream::CameraStream(PixelFormat f, Size s)\n>> +\t: index(-1), format(f), size(s), jpeg(nullptr)\n>> +{\n>> +}\n>> +\n>> +CameraStream::~CameraStream()\n>> +{\n>> +\tdelete jpeg;\n>> +};\n>> +\n>> /*\n>> * \\struct Camera3RequestDescriptor\n>> *\n>> @@ -167,6 +218,12 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camer\n>> \t facing_(CAMERA_FACING_FRONT), orientation_(0)\n>> {\n>> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n>> +\n>> +\t/*\n>> +\t * \\todo Determine a more accurate value for this during\n>> +\t * streamConfiguration.\n>> +\t */\n>> +\tmax_jpeg_buffer_size_ = 13 << 20; /* 13631488 from USB HAL */\n> \n> There's a function in turbojpeg to retrieve the maximum size,\n> tjBufSize(). It essentially returns width * height * 4 plus some margin\n> though, so be prepared for big buffers :-)\n> \n> As the plan seems to be to use the libjpeg RAW API, not the libturbojpeg\n> API, we may want to reimplement this function. It should be fairly\n> simple.\n> \n> Another item for your todo list, the maximum size should be queried from\n> the Compressor for a given PixelFormat and Size.\n\nIt's already in the todos.\n\n\n> \n>> }\n>> \n>> CameraDevice::~CameraDevice()\n>> @@ -417,10 +474,10 @@ std::tuple CameraDevice::calculateStaticMetadataSize()\n>> {\n>> \t/*\n>> \t * \\todo Keep this in sync with the actual number of entries.\n>> -\t * Currently: 50 entries, 647 bytes of static metadata\n>> +\t * Currently: 51 entries, 667 bytes of static metadata\n>> \t */\n>> -\tuint32_t numEntries = 50;\n>> -\tuint32_t byteSize = 651;\n>> +\tuint32_t numEntries = 51;\n>> +\tuint32_t byteSize = 667;\n>> \n>> \t/*\n>> \t * Calculate space occupation in bytes for dynamically built metadata\n>> @@ -576,6 +633,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\t\t\t availableThumbnailSizes.data(),\n>> \t\t\t\t availableThumbnailSizes.size());\n>> \n>> +\t/*\n>> +\t * \\todo Calculate the maximum JPEG buffer size by asking the compressor\n>> +\t * giving the maximum frame size required.\n>> +\t */\n> \n> Ah, there we go :-)\n\n:-D\n\n> \n>> +\tstaticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &max_jpeg_buffer_size_, 1);\n>> +\n>> \t/* Sensor static metadata. */\n>> \tint32_t pixelArraySize[] = {\n>> \t\t2592, 1944,\n>> @@ -789,6 +852,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>> \t\tANDROID_CONTROL_AVAILABLE_MODES,\n>> \t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>> +\t\tANDROID_JPEG_MAX_SIZE,\n>> \t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>> \t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>> \t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>> @@ -855,6 +919,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>> \t\tANDROID_SENSOR_EXPOSURE_TIME,\n>> \t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n>> \t\tANDROID_STATISTICS_SCENE_FLICKER,\n>> +\t\tANDROID_JPEG_SIZE,\n>> +\t\tANDROID_JPEG_QUALITY,\n>> +\t\tANDROID_JPEG_ORIENTATION,\n>> \t};\n>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n>> \t\t\t\t availableResultKeys.data(),\n>> @@ -1016,8 +1083,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t */\n>> \tunsigned int streamIndex = 0;\n>> \n>> +\t/* First handle all non-MJPEG streams. */\n>> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>> \t\tcamera3_stream_t *stream = stream_list->streams[i];\n>> +\t\tSize size(stream->width, stream->height);\n>> \n>> \t\tPixelFormat format = toPixelFormat(stream->format);\n>> \n>> @@ -1031,16 +1100,71 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\tif (!format.isValid())\n>> \t\t\treturn -EINVAL;\n>> \n>> +\t\tstreams_.emplace_back(format, size);\n>> +\t\tstream->priv = static_cast(&streams_[i]);\n>> +\n>> +\t\t/* Defer handling of MJPEG streams until all others are known. */\n>> +\t\tif (format == formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> \t\tStreamConfiguration streamConfiguration;\n>> \n>> -\t\tstreamConfiguration.size.width = stream->width;\n>> -\t\tstreamConfiguration.size.height = stream->height;\n>> +\t\tstreamConfiguration.size = size;\n>> \t\tstreamConfiguration.pixelFormat = format;\n>> \n>> \t\tconfig_->addConfiguration(streamConfiguration);\n>> \n>> \t\tstreams_[i].index = streamIndex++;\n>> -\t\tstream->priv = static_cast(&streams_[i]);\n>> +\t}\n>> +\n>> +\t/* Now handle MJPEG streams, adding a new stream if required. */\n>> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n>> +\t\tbool match = false;\n>> +\n>> +\t\tif (streams_[i].format != formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t/* Search for a compatible stream */\n>> +\t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n>> +\t\t\tStreamConfiguration &cfg = config_->at(j);\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * \\todo The PixelFormat must also be compatible with\n>> +\t\t\t * the encoder.\n> \n> We will likely also need to implement software down-scaling (and\n> possibly even format conversion) support if the hardware can't provide\n> us with an additional stream that matches the size and pixel format\n> needed for JPEG compression. Can you capture this in a todo as well ? I\n> think the code will need to be refactored to handle those additional\n> streams in a more generic way, not just for JPEG.\n\n\nYes, That's where I think we need to move out some CameraStream layer.\n\n\n\n>> +\t\t\t */\n>> +\t\t\tif (cfg.size == streams_[i].size) {\n>> +\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n>> +\t\t\t\t\t << \" using libcamera stream \" << j;\n>> +\n>> +\t\t\t\tmatch = true;\n>> +\t\t\t\tstreams_[i].index = j;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Without a compatible match for JPEG encoding we must\n>> +\t\t * introduce a new stream to satisfy the request requirements.\n>> +\t\t */\n>> +\t\tif (!match) {\n>> +\t\t\tStreamConfiguration streamConfiguration;\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * \\todo: The pixelFormat should be a 'best-fit' choice\n>> +\t\t\t * and may require a validation cycle. This is not yet\n>> +\t\t\t * handled, and should be considered as part of any\n>> +\t\t\t * stream configuration reworks.\n>> +\t\t\t */\n>> +\t\t\tstreamConfiguration.size.width = stream->width;\n>> +\t\t\tstreamConfiguration.size.height = stream->height;\n>> +\t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n>> +\n>> +\t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n>> +\t\t\t\t << \" for MJPEG support\";\n>> +\n>> +\t\t\tconfig_->addConfiguration(streamConfiguration);\n>> +\t\t\tstreams_[i].index = streamIndex++;\n>> +\t\t}\n>> \t}\n>> \n>> \tswitch (config_->validate()) {\n>> @@ -1067,6 +1191,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \n>> \t\t/* Use the bufferCount confirmed by the validation process. */\n>> \t\tstream->max_buffers = cfg.bufferCount;\n>> +\n>> +\t\t/*\n>> +\t\t * Construct a software compressor for MJPEG streams from the\n>> +\t\t * chosen libcamera source stream.\n>> +\t\t */\n>> +\t\tif (cameraStream->format == formats::MJPEG) {\n>> +\t\t\tcameraStream->jpeg = new CompressorJPEG();\n>> +\t\t\tcameraStream->jpeg->configure(cfg);\n>> +\t\t} else {\n>> +\t\t\t/* Either construct this correctly, or use a better interface */\n> \n> I'm not sure what you mean here.\n\nOld comment, can drop this I think.\n\nBut essentially somewhere here will need to be a Factory to construct\nthe right compressor anyway, depending on if it can use the hardware\ncompressor or not or such. I expect this to all get refactored.\n\n\n> \n>> +\t\t\tcameraStream->jpeg = nullptr;\n>> +\t\t}\n>> \t}\n>> \n>> \t/*\n>> @@ -1161,6 +1297,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n>> \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n>> \n>> +\t\t/* Software streams are handled after hardware streams complete. */\n>> +\t\tif (cameraStream->format == formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> \t\t/*\n>> \t\t * Create a libcamera buffer using the dmabuf descriptors of\n>> \t\t * the camera3Buffer for each stream. The FrameBuffer is\n>> @@ -1217,8 +1357,76 @@ void CameraDevice::requestComplete(Request *request)\n>> \tresultMetadata = getResultMetadata(descriptor->frameNumber,\n>> \t\t\t\t\t buffer->metadata().timestamp);\n>> \n>> -\t/* Prepare to call back the Android camera stack. */\n>> +\t/* Handle any JPEG compression */\n> \n> s/compression/compression./\n> \n>> +\tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n>> +\t\tCameraStream *cameraStream =\n>> +\t\t\tstatic_cast(descriptor->buffers[i].stream->priv);\n>> +\n>> +\t\tif (cameraStream->format != formats::MJPEG)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCompressor *compressor = cameraStream->jpeg;\n>> +\t\t/* Only handle streams with compression enabled. */\n>> +\t\tif (!compressor)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n>> +\t\tStream *stream = streamConfiguration->stream();\n>> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n>> +\t\tif (!buffer) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n> \n> Can you add a todo to mention that the code below (buffer mapping and\n> compression) needs to be moved to a separate thread ?\n> \n\nDone\n\n>> +\t\tMappedCamera3Buffer mapped(*descriptor->buffers[i].buffer, PROT_READ | PROT_WRITE);\n> \n> Line wrap ?\n> \n\ndone.\n\n>> +\t\tif (!mapped.isValid()) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to mmap android blob buffer\";\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> +\t\tCompressedImage output;\n>> +\t\toutput.data = static_cast(mapped.maps()[0].data());\n>> +\t\toutput.length = mapped.maps()[0].size();\n>> +\n>> +\t\tint ret = compressor->compress(buffer, &output);\n>> +\t\tif (ret) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to compress stream image\";\n>> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> \n> Do we need to proceed to adding the metadata entries then ? Or should we\n> continue to the next loop iteration ?\n\nLets put in a continue.\n\n> \n>> +\t\t}\n>> \n>> +\t\t/*\n>> +\t\t * Fill in the JPEG blob header.\n>> +\t\t *\n>> +\t\t * The mapped size of the buffer is being returned as\n>> +\t\t * substantially larger than the requested JPEG_MAX_SIZE\n>> +\t\t * (which is referenced from max_jpeg_buffer_size_). Utilise\n>> +\t\t * this static size to ensure the correct offset of the blob is\n>> +\t\t * determined.\n>> +\t\t *\n>> +\t\t * \\todo Investigate if the buffer size mismatch is an issue or\n>> +\t\t * expected behaviour.\n>> +\t\t */\n>> +\t\tuint8_t *resultPtr = mapped.maps()[0].data() +\n>> +\t\t\t\t max_jpeg_buffer_size_ -\n>> +\t\t\t\t sizeof(struct camera3_jpeg_blob);\n>> +\t\tauto *blob = reinterpret_cast(resultPtr);\n>> +\t\tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n>> +\t\tblob->jpeg_size = output.length;\n>> +\n>> +\t\t/* Update the JPEG result Metadata. */\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_SIZE,\n>> +\t\t\t\t\t &output.length, 1);\n>> +\n>> +\t\tconst uint32_t jpeg_quality = 95;\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY,\n>> +\t\t\t\t\t &jpeg_quality, 1);\n>> +\n>> +\t\tconst uint32_t jpeg_orientation = 0;\n>> +\t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n>> +\t\t\t\t\t &jpeg_orientation, 1);\n>> +\t}\n>> +\n>> +\t/* Prepare to call back the Android camera stack. */\n>> \tcamera3_capture_result_t captureResult = {};\n>> \tcaptureResult.frame_number = descriptor->frameNumber;\n>> \tcaptureResult.num_output_buffers = descriptor->numBuffers;\n>> @@ -1298,10 +1506,10 @@ std::unique_ptr CameraDevice::getResultMetadata(int frame_number\n>> {\n>> \t/*\n>> \t * \\todo Keep this in sync with the actual number of entries.\n>> -\t * Currently: 12 entries, 36 bytes\n>> +\t * Currently: 18 entries, 62 bytes\n>> \t */\n>> \tstd::unique_ptr resultMetadata =\n>> -\t\tstd::make_unique(15, 50);\n>> +\t\tstd::make_unique(18, 62);\n>> \tif (!resultMetadata->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n>> \t\treturn nullptr;\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 00472c219388..cfec5abeffa1 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -23,15 +23,25 @@\n>> #include \"libcamera/internal/log.h\"\n>> #include \"libcamera/internal/message.h\"\n>> \n>> +#include \"jpeg/compressor_jpeg.h\"\n>> +\n>> class CameraMetadata;\n>> \n>> struct CameraStream {\n> \n> I'd turn this into a class when you get the chance.\n> \n>> +\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n>> +\t~CameraStream();\n>> +\n>> \t/*\n>> \t * The index of the libcamera StreamConfiguration as added during\n>> \t * configureStreams(). A single libcamera Stream may be used to deliver\n>> \t * one or more streams to the Android framework.\n>> \t */\n>> \tunsigned int index;\n>> +\n>> +\tlibcamera::PixelFormat format;\n>> +\tlibcamera::Size size;\n>> +\n>> +\tCompressorJPEG *jpeg;\n> \n> Should this be a pointer to Compressor, not CompressorJPEG ?\n\nYes, that was the point of Compressor ;-)\n\n> \n>> };\n>> \n>> class CameraDevice : protected libcamera::Loggable\n>> @@ -104,6 +114,8 @@ private:\n>> \n>> \tint facing_;\n>> \tint orientation_;\n>> +\n>> +\tunsigned int max_jpeg_buffer_size_;\n> \n> maxJpegBufferSize_ ?\n\nYes, not sure what name scheme I was following there...\n\n> \n>> };\n>> \n>> #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 42713BD86F\n\tfor ;\n\tWed, 5 Aug 2020 15:10:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C280B605C0;\n\tWed, 5 Aug 2020 17:10:18 +0200 (CEST)","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 3ABB860392\n\tfor ;\n\tWed, 5 Aug 2020 17:10:18 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 871B12C0;\n\tWed, 5 Aug 2020 17:10:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kr1KuzsV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596640217;\n\tbh=THVTyPvDON8CDsFZG3gL6GZ+7iIqqdan3egAw/ENz3g=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=kr1KuzsVTeDMZKmLz3nzv7KimV0X0GDdti1OzseKg2M7JX/DE6GLLjpEPc8grNUYR\n\tgQNMY0p8DXFKBVfqxmuGbnclFpvT6gr5DGPLEiad5RNhiPmFyaSMuRpAM2hkfzoasR\n\tzHEbO/SN8aCGC7UJLHZJGlMgijZNgotv9uzoemr8=","To":"Laurent Pinchart ","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-14-kieran.bingham@ideasonboard.com>\n\t<20200805142542.GF6751@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<716da943-be06-1ab1-0163-243a59f5238a@ideasonboard.com>","Date":"Wed, 5 Aug 2020 16:10:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200805142542.GF6751@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] android: camera_device:\n\tSupport MJPEG stream construction","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]