[{"id":13528,"web_url":"https://patchwork.libcamera.org/comment/13528/","msgid":"","date":"2020-10-28T10:04:55","subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"I would like to call for comments about adding this ad-hoc reordering\nbefore CameraDevice configures pipeline handles.\nThe purpose is that a pipeline handler can produce streams from which\ngiven configurations can be achieved.\n\nBest Regards,\n-Hiro\n\nOn Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda wrote:\n>\n> Reorder configurations given by a HAL client in configureStreams()\n> so that CameraDevice and pipeline handlers can accept them as much\n> as possible.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 109 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ca60f51..eee7248 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -132,6 +133,16 @@ const std::map camera3FormatsMap = {\n>\n> LOG_DECLARE_CATEGORY(HAL)\n>\n> +void printStreamList(const camera3_stream_configuration_t& streamList) {\n> + LOG(HAL, Error) << \"num_streams=\" << streamList.num_streams;\n> + for (unsigned int i = 0; i < streamList.num_streams; i++) {\n> + const auto* stream = streamList.streams[i];\n> + LOG(HAL, Error) << i << \": width=\" << stream->width\n> + << \", height=\" << stream->height\n> + << \", format=\" << stream->format;\n> + }\n> +}\n> +\n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> int flags)\n> {\n> @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n> return requestTemplate->get();\n> }\n>\n> -PixelFormat CameraDevice::toPixelFormat(int format)\n> +PixelFormat CameraDevice::toPixelFormat(int format) const\n> {\n> /* Translate Android format code to libcamera pixel format. */\n> auto it = formatsMap_.find(format);\n> @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)\n> return it->second;\n> }\n>\n> +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const\n> +{\n> + std::map> streamMap;\n> + for (unsigned int i = 0; i < streamList->num_streams; ++i) {\n> + const camera3_stream_t* stream = streamList->streams[i];\n> + PixelFormat format = toPixelFormat(stream->format);\n> + if (!format.isValid())\n> + return -EINVAL;\n> + streamMap[format.fourcc()].push_back(stream);\n> + }\n> + std::vector formats;\n> + for (auto& s: streamMap) {\n> + formats.push_back(s.first);\n> + auto& streams = s.second;\n> + /* Sorted by resolution. Smaller is put first. */\n> + std::sort(streams.begin(), streams.end(),\n> + [](const camera3_stream_t* streamA,\n> + const camera3_stream_t* streamB) {\n> + if (streamA->width != streamB->width)\n> + return streamA->width < streamB->width;\n> + return streamA->height < streamB->height;\n> + });\n> + }\n> +\n> + if (streamMap.find(formats::MJPEG) != streamMap.end() &&\n> + streamMap[formats::MJPEG].size() > 1) {\n> + LOG(HAL, Error) << \"Multiple JPEG streams are not supported\";\n> + return -EINVAL;\n> + }\n> +\n> + std::vector sortedStreams;\n> + /*\n> + * NV12 is the most prioritized format. Put the configuration with NV12\n> + * and the largest resolution first.\n> + */\n> + auto nv12StreamsIt = streamMap.find(formats::NV12);\n> + if (nv12StreamsIt != streamMap.end()) {\n> + auto& nv12Streams = nv12StreamsIt->second;\n> + const camera3_stream_t* nv12Stream = nv12Streams.back();\n> + sortedStreams.push_back(nv12Stream);\n> + nv12Streams.pop_back();\n> + }\n> +\n> + /*\n> + * If JPEG is there, then put the JPEG configuration second and a\n> + * configuration whose specified stream the JPEG is encoded from third\n> + * if there is.\n> + */\n> + auto jpegStreamsIt = streamMap.find(formats::MJPEG);\n> + if (jpegStreamsIt != streamMap.end()) {\n> + auto& jpegStreams = jpegStreamsIt->second;\n> + const camera3_stream_t* jpegStream = jpegStreams[0];\n> + sortedStreams.push_back(jpegStream);\n> + streamMap.erase(jpegStreamsIt);\n> + if (nv12StreamsIt != streamMap.end()) {\n> + auto& nv12Streams = nv12StreamsIt->second;\n> + auto nv12StreamForJpegIt =\n> + std::find_if(nv12Streams.begin(),\n> + nv12Streams.end(),\n> + [&jpegStream](const auto* nv12Stream) {\n> + return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;\n> + });\n> + if (nv12StreamForJpegIt != nv12Streams.end()) {\n> + sortedStreams.push_back(*nv12StreamForJpegIt);\n> + nv12Streams.erase(nv12StreamForJpegIt);\n> + }\n> + }\n> + }\n> +\n> + /*\n> + * Put configurations with different formats and larger resolutions\n> + * earlier.\n> + */\n> + while (!streamMap.empty()) {\n> + for (auto it = streamMap.begin(); it != streamMap.end();) {\n> + auto& streams = it->second;\n> + if (streams.empty()) {\n> + it = streamMap.erase(it);\n> + continue;\n> + }\n> + sortedStreams.push_back(streams.back());\n> + streams.pop_back();\n> + }\n> + }\n> +\n> + assert(sortedStreams.size() == streamList->num_streams);\n> + memcpy(streamList->streams, sortedStreams.data(),\n> + sizeof(camera3_stream_t*) * streamList->num_streams);\n> + return 0;\n> +}\n> +\n> /*\n> * Inspect the stream_list to produce a list of StreamConfiguration to\n> * be use to configure the Camera.\n> @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> streams_.clear();\n> streams_.reserve(stream_list->num_streams);\n>\n> + printStreamList(*stream_list);\n> + sortStreamConfig(stream_list);\n> + printStreamList(*stream_list);\n> +\n> /* First handle all non-MJPEG streams. */\n> camera3_stream_t *jpegStream = nullptr;\n> for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index fd08738..f8dc28b 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -104,7 +104,8 @@ private:\n> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> CameraMetadata *requestTemplatePreview();\n> - libcamera::PixelFormat toPixelFormat(int format);\n> + libcamera::PixelFormat toPixelFormat(int format) const;\n> + int sortStreamConfig(camera3_stream_configuration_t* streamList) const;\n> std::unique_ptr getResultMetadata(int frame_number,\n> int64_t timestamp);\n>\n> --\n> 2.29.0.rc2.309.g374f81d7ae-goog","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 8DA3DBDB1E\n\tfor ;\n\tWed, 28 Oct 2020 10:05:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08F5E623D2;\n\tWed, 28 Oct 2020 11:05:11 +0100 (CET)","from mail-ed1-x544.google.com (mail-ed1-x544.google.com\n\t[IPv6:2a00:1450:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 037AE62067\n\tfor ;\n\tWed, 28 Oct 2020 11:05:10 +0100 (CET)","by mail-ed1-x544.google.com with SMTP id a6so4486310edx.6\n\tfor ;\n\tWed, 28 Oct 2020 03:05:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PunXEoh4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=lMTjAtIzDnGZXiOoaX2Yr992+1zhEHF9/LI2DBuPTHU=;\n\tb=PunXEoh4RsdriUOC7cFtm/XFdi9rEdZdCsSx+Y9ZfAXKq5px3zV7ynE8DkJlW/cVL9\n\tMIvT7aH9wflY1Y3A6TTxzy33jM5jBeatzEZz3/IUlpUUgf+vwdPMp7KI22MtSMeCAbMt\n\tVVGjjAhatBirNOMBCuYEbG/AsIEHU9nf83eMg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=lMTjAtIzDnGZXiOoaX2Yr992+1zhEHF9/LI2DBuPTHU=;\n\tb=C0Cy24eVxtD4laobcCoBfb1yrLVoR1sobhccFX+eX2fQGO6PuRiwPOLyNkZfLol3A3\n\tx5mV3RpN+9tVVDyr36412J4rlqrFdam+Il4EIi2mBmax9t0as7oSm+8HmurTIfj19+0c\n\tWx3fGl6f29XbjFg4ULG73itzzp325kgJ2/boKTbSvfApSlFYuuVTfjtwlW7iColGgwwH\n\t+85G9j82f1wEtht5R4RxhzATubhkGGTuCxnTPynb1GwavqtApxrxsmk9MFZWCcrf+DMP\n\tao8Suy77RNv0L1jWmATX+w+a+6crhn/ckTXONTMYAd9XkPIPK0SxrcPxPHuMiGcDM7GA\n\tAGLQ==","X-Gm-Message-State":"AOAM533Pt0YHP0hzbi2OWU3C6vetGwFN3jVxm/kQnrBT09tNbeanVrvi\n\tvYmlkXt6qRjTuS2gefTuYklGO+zWWkNRm/lW5oUiLxhGEG8=","X-Google-Smtp-Source":"ABdhPJzS3kkPwhMOxl8k5r9ei3LIBzopnJrIDdvGSx77lA3Ai9aivpy/ojj9oRSPGFLOGjs0uUgHuVVFX37uuQZuJJg=","X-Received":"by 2002:a50:ef0a:: with SMTP id\n\tm10mr6978560eds.116.1603879509142; \n\tWed, 28 Oct 2020 03:05:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20201028100134.3007829-1-hiroh@chromium.org>","In-Reply-To":"<20201028100134.3007829-1-hiroh@chromium.org>","From":"Hirokazu Honda ","Date":"Wed, 28 Oct 2020 19:04:55 +0900","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13529,"web_url":"https://patchwork.libcamera.org/comment/13529/","msgid":"","date":"2020-10-28T10:15:42","subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Definitely please ignore printStreamList(). I forgot removing that in\nuploading the patch.\n\nOn Wed, Oct 28, 2020 at 7:04 PM Hirokazu Honda wrote:\n>\n> I would like to call for comments about adding this ad-hoc reordering\n> before CameraDevice configures pipeline handles.\n> The purpose is that a pipeline handler can produce streams from which\n> given configurations can be achieved.\n>\n> Best Regards,\n> -Hiro\n>\n> On Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda wrote:\n> >\n> > Reorder configurations given by a HAL client in configureStreams()\n> > so that CameraDevice and pipeline handlers can accept them as much\n> > as possible.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 109 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ca60f51..eee7248 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -9,6 +9,7 @@\n> > #include \"camera_ops.h\"\n> > #include \"post_processor.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -132,6 +133,16 @@ const std::map camera3FormatsMap = {\n> >\n> > LOG_DECLARE_CATEGORY(HAL)\n> >\n> > +void printStreamList(const camera3_stream_configuration_t& streamList) {\n> > + LOG(HAL, Error) << \"num_streams=\" << streamList.num_streams;\n> > + for (unsigned int i = 0; i < streamList.num_streams; i++) {\n> > + const auto* stream = streamList.streams[i];\n> > + LOG(HAL, Error) << i << \": width=\" << stream->width\n> > + << \", height=\" << stream->height\n> > + << \", format=\" << stream->format;\n> > + }\n> > +}\n> > +\n> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > int flags)\n> > {\n> > @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n> > return requestTemplate->get();\n> > }\n> >\n> > -PixelFormat CameraDevice::toPixelFormat(int format)\n> > +PixelFormat CameraDevice::toPixelFormat(int format) const\n> > {\n> > /* Translate Android format code to libcamera pixel format. */\n> > auto it = formatsMap_.find(format);\n> > @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)\n> > return it->second;\n> > }\n> >\n> > +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const\n> > +{\n> > + std::map> streamMap;\n> > + for (unsigned int i = 0; i < streamList->num_streams; ++i) {\n> > + const camera3_stream_t* stream = streamList->streams[i];\n> > + PixelFormat format = toPixelFormat(stream->format);\n> > + if (!format.isValid())\n> > + return -EINVAL;\n> > + streamMap[format.fourcc()].push_back(stream);\n> > + }\n> > + std::vector formats;\n> > + for (auto& s: streamMap) {\n> > + formats.push_back(s.first);\n> > + auto& streams = s.second;\n> > + /* Sorted by resolution. Smaller is put first. */\n> > + std::sort(streams.begin(), streams.end(),\n> > + [](const camera3_stream_t* streamA,\n> > + const camera3_stream_t* streamB) {\n> > + if (streamA->width != streamB->width)\n> > + return streamA->width < streamB->width;\n> > + return streamA->height < streamB->height;\n> > + });\n> > + }\n> > +\n> > + if (streamMap.find(formats::MJPEG) != streamMap.end() &&\n> > + streamMap[formats::MJPEG].size() > 1) {\n> > + LOG(HAL, Error) << \"Multiple JPEG streams are not supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::vector sortedStreams;\n> > + /*\n> > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > + * and the largest resolution first.\n> > + */\n> > + auto nv12StreamsIt = streamMap.find(formats::NV12);\n> > + if (nv12StreamsIt != streamMap.end()) {\n> > + auto& nv12Streams = nv12StreamsIt->second;\n> > + const camera3_stream_t* nv12Stream = nv12Streams.back();\n> > + sortedStreams.push_back(nv12Stream);\n> > + nv12Streams.pop_back();\n> > + }\n> > +\n> > + /*\n> > + * If JPEG is there, then put the JPEG configuration second and a\n> > + * configuration whose specified stream the JPEG is encoded from third\n> > + * if there is.\n> > + */\n> > + auto jpegStreamsIt = streamMap.find(formats::MJPEG);\n> > + if (jpegStreamsIt != streamMap.end()) {\n> > + auto& jpegStreams = jpegStreamsIt->second;\n> > + const camera3_stream_t* jpegStream = jpegStreams[0];\n> > + sortedStreams.push_back(jpegStream);\n> > + streamMap.erase(jpegStreamsIt);\n> > + if (nv12StreamsIt != streamMap.end()) {\n> > + auto& nv12Streams = nv12StreamsIt->second;\n> > + auto nv12StreamForJpegIt =\n> > + std::find_if(nv12Streams.begin(),\n> > + nv12Streams.end(),\n> > + [&jpegStream](const auto* nv12Stream) {\n> > + return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;\n> > + });\n> > + if (nv12StreamForJpegIt != nv12Streams.end()) {\n> > + sortedStreams.push_back(*nv12StreamForJpegIt);\n> > + nv12Streams.erase(nv12StreamForJpegIt);\n> > + }\n> > + }\n> > + }\n> > +\n> > + /*\n> > + * Put configurations with different formats and larger resolutions\n> > + * earlier.\n> > + */\n> > + while (!streamMap.empty()) {\n> > + for (auto it = streamMap.begin(); it != streamMap.end();) {\n> > + auto& streams = it->second;\n> > + if (streams.empty()) {\n> > + it = streamMap.erase(it);\n> > + continue;\n> > + }\n> > + sortedStreams.push_back(streams.back());\n> > + streams.pop_back();\n> > + }\n> > + }\n> > +\n> > + assert(sortedStreams.size() == streamList->num_streams);\n> > + memcpy(streamList->streams, sortedStreams.data(),\n> > + sizeof(camera3_stream_t*) * streamList->num_streams);\n> > + return 0;\n> > +}\n> > +\n> > /*\n> > * Inspect the stream_list to produce a list of StreamConfiguration to\n> > * be use to configure the Camera.\n> > @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streams_.clear();\n> > streams_.reserve(stream_list->num_streams);\n> >\n> > + printStreamList(*stream_list);\n> > + sortStreamConfig(stream_list);\n> > + printStreamList(*stream_list);\n> > +\n> > /* First handle all non-MJPEG streams. */\n> > camera3_stream_t *jpegStream = nullptr;\n> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index fd08738..f8dc28b 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -104,7 +104,8 @@ private:\n> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > CameraMetadata *requestTemplatePreview();\n> > - libcamera::PixelFormat toPixelFormat(int format);\n> > + libcamera::PixelFormat toPixelFormat(int format) const;\n> > + int sortStreamConfig(camera3_stream_configuration_t* streamList) const;\n> > std::unique_ptr getResultMetadata(int frame_number,\n> > int64_t timestamp);\n> >\n> > --\n> > 2.29.0.rc2.309.g374f81d7ae-goog","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 57AAEC3B5C\n\tfor ;\n\tWed, 28 Oct 2020 10:15:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C56F4623AD;\n\tWed, 28 Oct 2020 11:15:56 +0100 (CET)","from mail-ed1-x543.google.com (mail-ed1-x543.google.com\n\t[IPv6:2a00:1450:4864:20::543])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1B0F62034\n\tfor ;\n\tWed, 28 Oct 2020 11:15:55 +0100 (CET)","by mail-ed1-x543.google.com with SMTP id a6so4519401edx.6\n\tfor ;\n\tWed, 28 Oct 2020 03:15:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BIsDvTtg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=YcIX9tOjNmYGb251Yv3BZFGvBesqxjrgLyEG1ZvYVIo=;\n\tb=BIsDvTtgoiRSVgIXke02mrNy0RdpzAPC+g+ZSf0d7rGe3ZZZh83AnQovYEebkqO/s5\n\tnu55jMit5jcbtrIGFmaByGFfdQIGovwxwUs2fdIYOB5OaXLR/Gvv1wIr7mb/nTZeFdcA\n\tyVCOve0H4Lm/IVlRen9rLqpjy2KLdXc+/QQ9s=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=YcIX9tOjNmYGb251Yv3BZFGvBesqxjrgLyEG1ZvYVIo=;\n\tb=JWueZaaKHo0n/3AqI4MWWPyjQlxKa11ESbkPr5TDYfQE7Ek7IsSEP45i7d2j0VWtRd\n\tSev5uifv83yU47B0Gc6StYKg9HWQOUJn3tliHI4ID7kDt5Too04mCKs7jnRPRxqh46F1\n\tvZ4LBhogJMXgHHI+DtCeBxXlNMC8yhgml6w7CCKm+sN9ju4qbZQ7AUEiQz68QajTubVk\n\tPlwk1JJ39YO8g1DMGh7Uu+4Ou66ulUBfpCuq+juRBtSP3vp2orp5nOJmsgNpCbPDBtiX\n\tS3tZ1ZA3lwE471dPmv0Oa84MtsNXOBnEADzUudkvMEWlsZKZD+osi5yY3+Nefj8MIbb9\n\tg3+w==","X-Gm-Message-State":"AOAM532eGr2FHA9GxAKvpFuxNbYFd6p2syLH7bFDHSYe1rUNqylsltZV\n\tQ1jjAH+QGRjmiTam2giHd755K8uYAAGCaU7izOD016IgF2A=","X-Google-Smtp-Source":"ABdhPJxP/Ty9zuy1jyH+1Ultiy4UkGDTgIiUFD+DDddwpWH8Sj1afohwTMaNE1mJSW7FezCPBa6RI4UHoYgwVnLHHrQ=","X-Received":"by 2002:a05:6402:13cc:: with SMTP id\n\ta12mr6959939edx.73.1603880155135; \n\tWed, 28 Oct 2020 03:15:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20201028100134.3007829-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 28 Oct 2020 19:15:42 +0900","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13596,"web_url":"https://patchwork.libcamera.org/comment/13596/","msgid":"","date":"2020-11-04T09:46:30","subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"gentle ping!\n\nOn Wed, Oct 28, 2020 at 7:15 PM Hirokazu Honda wrote:\n>\n> Definitely please ignore printStreamList(). I forgot removing that in\n> uploading the patch.\n>\n> On Wed, Oct 28, 2020 at 7:04 PM Hirokazu Honda wrote:\n> >\n> > I would like to call for comments about adding this ad-hoc reordering\n> > before CameraDevice configures pipeline handles.\n> > The purpose is that a pipeline handler can produce streams from which\n> > given configurations can be achieved.\n> >\n> > Best Regards,\n> > -Hiro\n> >\n> > On Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda wrote:\n> > >\n> > > Reorder configurations given by a HAL client in configureStreams()\n> > > so that CameraDevice and pipeline handlers can accept them as much\n> > > as possible.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-\n> > > src/android/camera_device.h | 3 +-\n> > > 2 files changed, 109 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ca60f51..eee7248 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -9,6 +9,7 @@\n> > > #include \"camera_ops.h\"\n> > > #include \"post_processor.h\"\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -132,6 +133,16 @@ const std::map camera3FormatsMap = {\n> > >\n> > > LOG_DECLARE_CATEGORY(HAL)\n> > >\n> > > +void printStreamList(const camera3_stream_configuration_t& streamList) {\n> > > + LOG(HAL, Error) << \"num_streams=\" << streamList.num_streams;\n> > > + for (unsigned int i = 0; i < streamList.num_streams; i++) {\n> > > + const auto* stream = streamList.streams[i];\n> > > + LOG(HAL, Error) << i << \": width=\" << stream->width\n> > > + << \", height=\" << stream->height\n> > > + << \", format=\" << stream->format;\n> > > + }\n> > > +}\n> > > +\n> > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > int flags)\n> > > {\n> > > @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n> > > return requestTemplate->get();\n> > > }\n> > >\n> > > -PixelFormat CameraDevice::toPixelFormat(int format)\n> > > +PixelFormat CameraDevice::toPixelFormat(int format) const\n> > > {\n> > > /* Translate Android format code to libcamera pixel format. */\n> > > auto it = formatsMap_.find(format);\n> > > @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)\n> > > return it->second;\n> > > }\n> > >\n> > > +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const\n> > > +{\n> > > + std::map> streamMap;\n> > > + for (unsigned int i = 0; i < streamList->num_streams; ++i) {\n> > > + const camera3_stream_t* stream = streamList->streams[i];\n> > > + PixelFormat format = toPixelFormat(stream->format);\n> > > + if (!format.isValid())\n> > > + return -EINVAL;\n> > > + streamMap[format.fourcc()].push_back(stream);\n> > > + }\n> > > + std::vector formats;\n> > > + for (auto& s: streamMap) {\n> > > + formats.push_back(s.first);\n> > > + auto& streams = s.second;\n> > > + /* Sorted by resolution. Smaller is put first. */\n> > > + std::sort(streams.begin(), streams.end(),\n> > > + [](const camera3_stream_t* streamA,\n> > > + const camera3_stream_t* streamB) {\n> > > + if (streamA->width != streamB->width)\n> > > + return streamA->width < streamB->width;\n> > > + return streamA->height < streamB->height;\n> > > + });\n> > > + }\n> > > +\n> > > + if (streamMap.find(formats::MJPEG) != streamMap.end() &&\n> > > + streamMap[formats::MJPEG].size() > 1) {\n> > > + LOG(HAL, Error) << \"Multiple JPEG streams are not supported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + std::vector sortedStreams;\n> > > + /*\n> > > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > > + * and the largest resolution first.\n> > > + */\n> > > + auto nv12StreamsIt = streamMap.find(formats::NV12);\n> > > + if (nv12StreamsIt != streamMap.end()) {\n> > > + auto& nv12Streams = nv12StreamsIt->second;\n> > > + const camera3_stream_t* nv12Stream = nv12Streams.back();\n> > > + sortedStreams.push_back(nv12Stream);\n> > > + nv12Streams.pop_back();\n> > > + }\n> > > +\n> > > + /*\n> > > + * If JPEG is there, then put the JPEG configuration second and a\n> > > + * configuration whose specified stream the JPEG is encoded from third\n> > > + * if there is.\n> > > + */\n> > > + auto jpegStreamsIt = streamMap.find(formats::MJPEG);\n> > > + if (jpegStreamsIt != streamMap.end()) {\n> > > + auto& jpegStreams = jpegStreamsIt->second;\n> > > + const camera3_stream_t* jpegStream = jpegStreams[0];\n> > > + sortedStreams.push_back(jpegStream);\n> > > + streamMap.erase(jpegStreamsIt);\n> > > + if (nv12StreamsIt != streamMap.end()) {\n> > > + auto& nv12Streams = nv12StreamsIt->second;\n> > > + auto nv12StreamForJpegIt =\n> > > + std::find_if(nv12Streams.begin(),\n> > > + nv12Streams.end(),\n> > > + [&jpegStream](const auto* nv12Stream) {\n> > > + return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;\n> > > + });\n> > > + if (nv12StreamForJpegIt != nv12Streams.end()) {\n> > > + sortedStreams.push_back(*nv12StreamForJpegIt);\n> > > + nv12Streams.erase(nv12StreamForJpegIt);\n> > > + }\n> > > + }\n> > > + }\n> > > +\n> > > + /*\n> > > + * Put configurations with different formats and larger resolutions\n> > > + * earlier.\n> > > + */\n> > > + while (!streamMap.empty()) {\n> > > + for (auto it = streamMap.begin(); it != streamMap.end();) {\n> > > + auto& streams = it->second;\n> > > + if (streams.empty()) {\n> > > + it = streamMap.erase(it);\n> > > + continue;\n> > > + }\n> > > + sortedStreams.push_back(streams.back());\n> > > + streams.pop_back();\n> > > + }\n> > > + }\n> > > +\n> > > + assert(sortedStreams.size() == streamList->num_streams);\n> > > + memcpy(streamList->streams, sortedStreams.data(),\n> > > + sizeof(camera3_stream_t*) * streamList->num_streams);\n> > > + return 0;\n> > > +}\n> > > +\n> > > /*\n> > > * Inspect the stream_list to produce a list of StreamConfiguration to\n> > > * be use to configure the Camera.\n> > > @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > streams_.clear();\n> > > streams_.reserve(stream_list->num_streams);\n> > >\n> > > + printStreamList(*stream_list);\n> > > + sortStreamConfig(stream_list);\n> > > + printStreamList(*stream_list);\n> > > +\n> > > /* First handle all non-MJPEG streams. */\n> > > camera3_stream_t *jpegStream = nullptr;\n> > > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index fd08738..f8dc28b 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -104,7 +104,8 @@ private:\n> > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > CameraMetadata *requestTemplatePreview();\n> > > - libcamera::PixelFormat toPixelFormat(int format);\n> > > + libcamera::PixelFormat toPixelFormat(int format) const;\n> > > + int sortStreamConfig(camera3_stream_configuration_t* streamList) const;\n> > > std::unique_ptr getResultMetadata(int frame_number,\n> > > int64_t timestamp);\n> > >\n> > > --\n> > > 2.29.0.rc2.309.g374f81d7ae-goog","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 0EDFCBE080\n\tfor ;\n\tWed, 4 Nov 2020 09:46:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D118662C58;\n\tWed, 4 Nov 2020 10:46:43 +0100 (CET)","from mail-ej1-x643.google.com (mail-ej1-x643.google.com\n\t[IPv6:2a00:1450:4864:20::643])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 792C460346\n\tfor ;\n\tWed, 4 Nov 2020 10:46:42 +0100 (CET)","by mail-ej1-x643.google.com with SMTP id za3so28826445ejb.5\n\tfor ;\n\tWed, 04 Nov 2020 01:46:42 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"j/HTK+P7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=Vh4ttMrKyIL4QAwxM9Tz9t87EIjR1WZFrbQCyWt6Ilk=;\n\tb=j/HTK+P72aRZzSaqfZada5Qspj1BY9Uj9NbC5oxtLUa3G6qzr5lSQSrXcjeUlZOf9C\n\tMKwvVCnoG6aHe3EzTBfosAirPuJP+I8Ua6V5XYgZo2cyyejHvOZDJiXnmittJo0Z0q9a\n\tj1zrdH1xX795ctIOXeu6rj6XYFWcAQbZiIqUI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=Vh4ttMrKyIL4QAwxM9Tz9t87EIjR1WZFrbQCyWt6Ilk=;\n\tb=LsnCh6vmnn+f6mlqHKJHPuegvUvTAgDuVy4WvEUinSuLq739H2vzgTIcUeWfJVKyPY\n\tXvkQ+Iu68I3LeH6LqwkQ9my6EPEhibTfpQPb3SkbuHYLVClUVcQ7TnVhr7Bci6tgDoCs\n\tnRWqE/bnLpVpkmRlxEydXqBt2fKgzqB/28J4YYSEiAgbKxv+dYPODMJ1pzk5Mx5v+ulb\n\tnM+aJBPImmP2H2eZgMsvORKM0KKdFNq6JqVDqTuCu+w3eCb0s+LCzE/C/rZdWBC1yW5+\n\tNCxhQIB33stomgLxadSAmfFsBzTbCPB6Exuw9Yt0XCkBgU0Cv39dWjduMJihWHJiWYey\n\tM4tg==","X-Gm-Message-State":"AOAM5336Ru+91N8WYf092XzaSHXB7cTmzOqo6LJWtzM4qX7jqjNKfi20\n\tZBpku+C2IUBOgyz3aUen1kabJu4396//JGPAt+NmCMzbsMQ=","X-Google-Smtp-Source":"ABdhPJwfsZwjZuHYW5RQ3kXodV/uXLU9tLcjRf5A50jmHOSXmg6rt4+o+G7ZtB/AHRnpkD1XsgaGkeBmuzSmk3iEwUQ=","X-Received":"by 2002:a17:906:2697:: with SMTP id\n\tt23mr193016ejc.292.1604483201802; \n\tWed, 04 Nov 2020 01:46:41 -0800 (PST)","MIME-Version":"1.0","References":"<20201028100134.3007829-1-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 4 Nov 2020 18:46:30 +0900","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13602,"web_url":"https://patchwork.libcamera.org/comment/13602/","msgid":"<20201104144821.ghwyq6bsdblruvhl@uno.localdomain>","date":"2020-11-04T14:48:21","subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n sorry for the delay.\n\nNot a really detailed review but more generic questions on the design\n\nOn Wed, Oct 28, 2020 at 07:01:34PM +0900, Hirokazu Honda wrote:\n> Reorder configurations given by a HAL client in configureStreams()\n> so that CameraDevice and pipeline handlers can accept them as much\n> as possible.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-\n> src/android/camera_device.h | 3 +-\n> 2 files changed, 109 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ca60f51..eee7248 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -132,6 +133,16 @@ const std::map camera3FormatsMap = {\n>\n> LOG_DECLARE_CATEGORY(HAL)\n>\n> +void printStreamList(const camera3_stream_configuration_t& streamList) {\n> +\tLOG(HAL, Error) << \"num_streams=\" << streamList.num_streams;\n> +\tfor (unsigned int i = 0; i < streamList.num_streams; i++) {\n> +\t\tconst auto* stream = streamList.streams[i];\n> +\t\tLOG(HAL, Error) << i << \": width=\" << stream->width\n> +\t\t\t\t<< \", height=\" << stream->height\n> +\t\t\t\t<< \", format=\" << stream->format;\n> +\t}\n> +}\n> +\n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> {\n> @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n> \treturn requestTemplate->get();\n> }\n>\n> -PixelFormat CameraDevice::toPixelFormat(int format)\n> +PixelFormat CameraDevice::toPixelFormat(int format) const\n> {\n> \t/* Translate Android format code to libcamera pixel format. */\n> \tauto it = formatsMap_.find(format);\n> @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)\n> \treturn it->second;\n> }\n>\n> +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const\n\nIs the intended sorting order of the Android streams the following one?\n\n { largest NV12, JPEG, NV12 same as JPEG, other streams.. }\n\nIt see you are sorting the stream requested by Android.\nI might see the reason to do so, as StreamConfiguration are added to\nthe CameraConfiguration while we walk the Android streams, so\npresenting the android streams ordered has the effect of ordering the\nStreamConfiguration consequentally. Is this the intent ?\n\nIn example:\n\n{ NV12-max, JPEG-vga, NV12-vga, .. }\n\nworks because the CameraDevice::configureStreams() function will associate\nJPEG-vga with NV12-vga and the resulting StreamConfiguration will be\n\n{ NV12-max, NV12-vga, .. }\n\nIf there's no NV12-vga to generate JPEG-vga from, if I'm not mistaken,\nthe result will be\n\n{ NV12-max, ... , NV12-vga }\n\nas configureStreams inspects the JPEG streams after all the other\nones, and eventually adds an NV12 stream to generate JPEG streams\nfrom.\n\nApart from this, what concerns me a bit is that this ordering is meant\nto satisfy the configureStreams() implementation, and if one changes,\nthe one has to change as well, as they are interdependent.\n\nIf the final goal is to be able to sort the StreamConfiguration before\npresenting them to the libcamera::Camera I wonder if it wouldn't be\nbetter to rework configureStream() to:\n1) Store StreamConfiguration in an std::vector<>\n2) Order the vector of StreamConfiguration with the same logic you\n have here\n3) Add the sorted StreamConfiguration to the CameraConfiguration\n\nThis implies the CameraStream class will have to change, as the index\nof the associated StreamConfiguration will have to be injected later,\nafter the StreamConfiguration is CameraConfiguration::add() to the\nCameraConfiguration. It's quite a substantial change, but I feel like\nit would be more robust than sorting the android stream to please code\nthat we're in control of.\n\nWhat do you think ?\n\n\n> +{\n> +\tstd::map> streamMap;\n> +\tfor (unsigned int i = 0; i < streamList->num_streams; ++i) {\n> +\t\tconst camera3_stream_t* stream = streamList->streams[i];\n> +\t\tPixelFormat format = toPixelFormat(stream->format);\n> +\t\tif (!format.isValid())\n> +\t\t\treturn -EINVAL;\n> +\t\tstreamMap[format.fourcc()].push_back(stream);\n> +\t}\n> +\tstd::vector formats;\n> +\tfor (auto& s: streamMap) {\n> +\t\tformats.push_back(s.first);\n> +\t\tauto& streams = s.second;\n> +\t\t/* Sorted by resolution. Smaller is put first. */\n> +\t\tstd::sort(streams.begin(), streams.end(),\n> +\t\t\t [](const camera3_stream_t* streamA,\n> +\t\t\t const camera3_stream_t* streamB) {\n> +\t\t\t\t if (streamA->width != streamB->width)\n> +\t\t\t\t\t return streamA->width < streamB->width;\n> +\t\t\t\t return streamA->height < streamB->height;\n> +\t\t\t });\n> +\t}\n> +\n> +\tif (streamMap.find(formats::MJPEG) != streamMap.end() &&\n> +\t streamMap[formats::MJPEG].size() > 1) {\n> +\t\tLOG(HAL, Error) << \"Multiple JPEG streams are not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::vector sortedStreams;\n> +\t/*\n> +\t * NV12 is the most prioritized format. Put the configuration with NV12\n> +\t * and the largest resolution first.\n> +\t */\n> +\tauto nv12StreamsIt = streamMap.find(formats::NV12);\n> +\tif (nv12StreamsIt != streamMap.end()) {\n> +\t\tauto& nv12Streams = nv12StreamsIt->second;\n> +\t\tconst camera3_stream_t* nv12Stream = nv12Streams.back();\n> +\t\tsortedStreams.push_back(nv12Stream);\n> +\t\tnv12Streams.pop_back();\n> +\t}\n> +\n> +\t/*\n> +\t * If JPEG is there, then put the JPEG configuration second and a\n> +\t * configuration whose specified stream the JPEG is encoded from third\n> +\t * if there is.\n> +\t */\n> +\tauto jpegStreamsIt = streamMap.find(formats::MJPEG);\n> +\tif (jpegStreamsIt != streamMap.end()) {\n> +\t\tauto& jpegStreams = jpegStreamsIt->second;\n> +\t\tconst camera3_stream_t* jpegStream = jpegStreams[0];\n> +\t\tsortedStreams.push_back(jpegStream);\n> +\t\tstreamMap.erase(jpegStreamsIt);\n> +\t\tif (nv12StreamsIt != streamMap.end()) {\n> +\t\t\tauto& nv12Streams = nv12StreamsIt->second;\n> +\t\t\tauto nv12StreamForJpegIt =\n> +\t\t\t\tstd::find_if(nv12Streams.begin(),\n> +\t\t\t\t\t nv12Streams.end(),\n> +\t\t\t\t\t [&jpegStream](const auto* nv12Stream) {\n> +\t\t\t\t\t\t return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;\n> +\t\t\t\t\t });\n> +\t\t\tif (nv12StreamForJpegIt != nv12Streams.end()) {\n> +\t\t\t\tsortedStreams.push_back(*nv12StreamForJpegIt);\n> +\t\t\t\tnv12Streams.erase(nv12StreamForJpegIt);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Put configurations with different formats and larger resolutions\n> +\t * earlier.\n> +\t */\n> +\twhile (!streamMap.empty()) {\n> +\t\tfor (auto it = streamMap.begin(); it != streamMap.end();) {\n> +\t\t\tauto& streams = it->second;\n> +\t\t\tif (streams.empty()) {\n> +\t\t\t\tit = streamMap.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tsortedStreams.push_back(streams.back());\n> +\t\t\tstreams.pop_back();\n> +\t\t}\n> +\t}\n> +\n> +\tassert(sortedStreams.size() == streamList->num_streams);\n> +\tmemcpy(streamList->streams, sortedStreams.data(),\n> +\t sizeof(camera3_stream_t*) * streamList->num_streams);\n> +\treturn 0;\n> +}\n> +\n> /*\n> * Inspect the stream_list to produce a list of StreamConfiguration to\n> * be use to configure the Camera.\n> @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \tstreams_.clear();\n> \tstreams_.reserve(stream_list->num_streams);\n>\n> +\tprintStreamList(*stream_list);\n> +\tsortStreamConfig(stream_list);\n> +\tprintStreamList(*stream_list);\n> +\n> \t/* First handle all non-MJPEG streams. */\n> \tcamera3_stream_t *jpegStream = nullptr;\n> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index fd08738..f8dc28b 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -104,7 +104,8 @@ private:\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> \tCameraMetadata *requestTemplatePreview();\n> -\tlibcamera::PixelFormat toPixelFormat(int format);\n> +\tlibcamera::PixelFormat toPixelFormat(int format) const;\n> +\tint sortStreamConfig(camera3_stream_configuration_t* streamList) const;\n> \tstd::unique_ptr getResultMetadata(int frame_number,\n> \t\t\t\t\t\t\t int64_t timestamp);\n>\n> --\n> 2.29.0.rc2.309.g374f81d7ae-goog\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 C76FFBE080\n\tfor ;\n\tWed, 4 Nov 2020 14:48:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 684AB62C1A;\n\tWed, 4 Nov 2020 15:48:23 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A6D362B91\n\tfor ;\n\tWed, 4 Nov 2020 15:48:21 +0100 (CET)","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 E517B100017;\n\tWed, 4 Nov 2020 14:48:20 +0000 (UTC)"],"Date":"Wed, 4 Nov 2020 15:48:21 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201104144821.ghwyq6bsdblruvhl@uno.localdomain>","References":"<20201028100134.3007829-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201028100134.3007829-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13609,"web_url":"https://patchwork.libcamera.org/comment/13609/","msgid":"","date":"2020-11-05T11:29:08","subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\nThanks for replying.\n\nOn Wed, Nov 4, 2020 at 11:48 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n> sorry for the delay.\n>\n> Not a really detailed review but more generic questions on the design\n>\n> On Wed, Oct 28, 2020 at 07:01:34PM +0900, Hirokazu Honda wrote:\n> > Reorder configurations given by a HAL client in configureStreams()\n> > so that CameraDevice and pipeline handlers can accept them as much\n> > as possible.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-\n> > src/android/camera_device.h | 3 +-\n> > 2 files changed, 109 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ca60f51..eee7248 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -9,6 +9,7 @@\n> > #include \"camera_ops.h\"\n> > #include \"post_processor.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -132,6 +133,16 @@ const std::map camera3FormatsMap = {\n> >\n> > LOG_DECLARE_CATEGORY(HAL)\n> >\n> > +void printStreamList(const camera3_stream_configuration_t& streamList) {\n> > + LOG(HAL, Error) << \"num_streams=\" << streamList.num_streams;\n> > + for (unsigned int i = 0; i < streamList.num_streams; i++) {\n> > + const auto* stream = streamList.streams[i];\n> > + LOG(HAL, Error) << i << \": width=\" << stream->width\n> > + << \", height=\" << stream->height\n> > + << \", format=\" << stream->format;\n> > + }\n> > +}\n> > +\n> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > int flags)\n> > {\n> > @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n> > return requestTemplate->get();\n> > }\n> >\n> > -PixelFormat CameraDevice::toPixelFormat(int format)\n> > +PixelFormat CameraDevice::toPixelFormat(int format) const\n> > {\n> > /* Translate Android format code to libcamera pixel format. */\n> > auto it = formatsMap_.find(format);\n> > @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)\n> > return it->second;\n> > }\n> >\n> > +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const\n>\n> Is the intended sorting order of the Android streams the following one?\n>\n> { largest NV12, JPEG, NV12 same as JPEG, other streams.. }\n>\n> It see you are sorting the stream requested by Android.\n> I might see the reason to do so, as StreamConfiguration are added to\n> the CameraConfiguration while we walk the Android streams, so\n> presenting the android streams ordered has the effect of ordering the\n> StreamConfiguration consequentally. Is this the intent ?\n>\n\nYes, I don't add any new config in sortsortStreamConfig(), because I\nmust not change the given config and the \"backup\" config for jpeg is\nadded later as you said.\n\n> In example:\n>\n> { NV12-max, JPEG-vga, NV12-vga, .. }\n>\n> works because the CameraDevice::configureStreams() function will associate\n> JPEG-vga with NV12-vga and the resulting StreamConfiguration will be\n>\n> { NV12-max, NV12-vga, .. }\n>\n> If there's no NV12-vga to generate JPEG-vga from, if I'm not mistaken,\n> the result will be\n>\n> { NV12-max, ... , NV12-vga }\n>\n> as configureStreams inspects the JPEG streams after all the other\n> ones, and eventually adds an NV12 stream to generate JPEG streams\n> from.\n>\n> Apart from this, what concerns me a bit is that this ordering is meant\n> to satisfy the configureStreams() implementation, and if one changes,\n> the one has to change as well, as they are interdependent.\n>\n> If the final goal is to be able to sort the StreamConfiguration before\n> presenting them to the libcamera::Camera I wonder if it wouldn't be\n> better to rework configureStream() to:\n> 1) Store StreamConfiguration in an std::vector<>\n> 2) Order the vector of StreamConfiguration with the same logic you\n> have here\n> 3) Add the sorted StreamConfiguration to the CameraConfiguration\n>\n> This implies the CameraStream class will have to change, as the index\n> of the associated StreamConfiguration will have to be injected later,\n> after the StreamConfiguration is CameraConfiguration::add() to the\n> CameraConfiguration. It's quite a substantial change, but I feel like\n> it would be more robust than sorting the android stream to please code\n> that we're in control of.\n>\n> What do you think ?\n>\n\nI think this also works.\nI am going to try and submit your idea.\nLet's see how the code looks like in the next version patch.\n\nThanks so much,\n-Hiro\n>\n> > +{\n> > + std::map> streamMap;\n> > + for (unsigned int i = 0; i < streamList->num_streams; ++i) {\n> > + const camera3_stream_t* stream = streamList->streams[i];\n> > + PixelFormat format = toPixelFormat(stream->format);\n> > + if (!format.isValid())\n> > + return -EINVAL;\n> > + streamMap[format.fourcc()].push_back(stream);\n> > + }\n> > + std::vector formats;\n> > + for (auto& s: streamMap) {\n> > + formats.push_back(s.first);\n> > + auto& streams = s.second;\n> > + /* Sorted by resolution. Smaller is put first. */\n> > + std::sort(streams.begin(), streams.end(),\n> > + [](const camera3_stream_t* streamA,\n> > + const camera3_stream_t* streamB) {\n> > + if (streamA->width != streamB->width)\n> > + return streamA->width < streamB->width;\n> > + return streamA->height < streamB->height;\n> > + });\n> > + }\n> > +\n> > + if (streamMap.find(formats::MJPEG) != streamMap.end() &&\n> > + streamMap[formats::MJPEG].size() > 1) {\n> > + LOG(HAL, Error) << \"Multiple JPEG streams are not supported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::vector sortedStreams;\n> > + /*\n> > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > + * and the largest resolution first.\n> > + */\n> > + auto nv12StreamsIt = streamMap.find(formats::NV12);\n> > + if (nv12StreamsIt != streamMap.end()) {\n> > + auto& nv12Streams = nv12StreamsIt->second;\n> > + const camera3_stream_t* nv12Stream = nv12Streams.back();\n> > + sortedStreams.push_back(nv12Stream);\n> > + nv12Streams.pop_back();\n> > + }\n> > +\n> > + /*\n> > + * If JPEG is there, then put the JPEG configuration second and a\n> > + * configuration whose specified stream the JPEG is encoded from third\n> > + * if there is.\n> > + */\n> > + auto jpegStreamsIt = streamMap.find(formats::MJPEG);\n> > + if (jpegStreamsIt != streamMap.end()) {\n> > + auto& jpegStreams = jpegStreamsIt->second;\n> > + const camera3_stream_t* jpegStream = jpegStreams[0];\n> > + sortedStreams.push_back(jpegStream);\n> > + streamMap.erase(jpegStreamsIt);\n> > + if (nv12StreamsIt != streamMap.end()) {\n> > + auto& nv12Streams = nv12StreamsIt->second;\n> > + auto nv12StreamForJpegIt =\n> > + std::find_if(nv12Streams.begin(),\n> > + nv12Streams.end(),\n> > + [&jpegStream](const auto* nv12Stream) {\n> > + return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;\n> > + });\n> > + if (nv12StreamForJpegIt != nv12Streams.end()) {\n> > + sortedStreams.push_back(*nv12StreamForJpegIt);\n> > + nv12Streams.erase(nv12StreamForJpegIt);\n> > + }\n> > + }\n> > + }\n> > +\n> > + /*\n> > + * Put configurations with different formats and larger resolutions\n> > + * earlier.\n> > + */\n> > + while (!streamMap.empty()) {\n> > + for (auto it = streamMap.begin(); it != streamMap.end();) {\n> > + auto& streams = it->second;\n> > + if (streams.empty()) {\n> > + it = streamMap.erase(it);\n> > + continue;\n> > + }\n> > + sortedStreams.push_back(streams.back());\n> > + streams.pop_back();\n> > + }\n> > + }\n> > +\n> > + assert(sortedStreams.size() == streamList->num_streams);\n> > + memcpy(streamList->streams, sortedStreams.data(),\n> > + sizeof(camera3_stream_t*) * streamList->num_streams);\n> > + return 0;\n> > +}\n> > +\n> > /*\n> > * Inspect the stream_list to produce a list of StreamConfiguration to\n> > * be use to configure the Camera.\n> > @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streams_.clear();\n> > streams_.reserve(stream_list->num_streams);\n> >\n> > + printStreamList(*stream_list);\n> > + sortStreamConfig(stream_list);\n> > + printStreamList(*stream_list);\n> > +\n> > /* First handle all non-MJPEG streams. */\n> > camera3_stream_t *jpegStream = nullptr;\n> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index fd08738..f8dc28b 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -104,7 +104,8 @@ private:\n> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > CameraMetadata *requestTemplatePreview();\n> > - libcamera::PixelFormat toPixelFormat(int format);\n> > + libcamera::PixelFormat toPixelFormat(int format) const;\n> > + int sortStreamConfig(camera3_stream_configuration_t* streamList) const;\n> > std::unique_ptr getResultMetadata(int frame_number,\n> > int64_t timestamp);\n> >\n> > --\n> > 2.29.0.rc2.309.g374f81d7ae-goog\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 73212BDB89\n\tfor ;\n\tThu, 5 Nov 2020 11:29:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B5A462CF4;\n\tThu, 5 Nov 2020 12:29:21 +0100 (CET)","from mail-ed1-x541.google.com (mail-ed1-x541.google.com\n\t[IPv6:2a00:1450:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD4BC60353\n\tfor ;\n\tThu, 5 Nov 2020 12:29:19 +0100 (CET)","by mail-ed1-x541.google.com with SMTP id t11so1119623edj.13\n\tfor ;\n\tThu, 05 Nov 2020 03:29:19 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"S+Ky3aWz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=PGV0wBa+n8mYBqDUaJf2s5za2vjTDflK7o/ax10AclU=;\n\tb=S+Ky3aWzLkox0dZkxDAdUPs9l26/idG1HQcWrqLuXz5vxEpYkQe6baTdMbLH1vn6tJ\n\tH53m+sMJJH4oMELv2OpopjXDqRjGcll5EnH4mi81b1HJatbgpTKxvkTOgIpkPCUYPg8r\n\tm2wKnWp0sztdP+csNZezD9f9/m+SnmVwfkY84=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=PGV0wBa+n8mYBqDUaJf2s5za2vjTDflK7o/ax10AclU=;\n\tb=bwdRz3unz1t8D/Gj66sz8jjrlZtzbfARvxP0wrs+6hW6SsFJE46sU2a3RKF1D3cWXJ\n\tTx3bryGKolmpG5ughc/AUV379TC2mdkwwUf/RYAooyaCfXDZ22Ggv8o9reKEozvmenOE\n\tRZKcXF8pZdzV84to2MabqPD9dhruUCVHgEu9Bunbmnqh88JJHaleUVITAqtVhoYZX5e0\n\tOe/hi09s96l/yzlVDeSqQSwmlw2DbkMCSGnIBykjEaOUBDWTz2cko5sHnPUjapRpekC8\n\t9Z14uh2xYHLsRSktXtoKxwscgBxaW7NjKqpy0X4tGngoknP2lN4Q16gYN9LGpFZRZ1NL\n\tZ16Q==","X-Gm-Message-State":"AOAM5331cBtB1IJED8Kam/REVz5Rbd/IeJS9mTiWcjXsdmbNedLPTK2M\n\tBVmNpm9/KcfNISoqBfhDkTI5/Zc1Ma+epbV47HAYBA==","X-Google-Smtp-Source":"ABdhPJz24kFGfW7O3DyHDR5usAEW5QiS4rwyvWgNUrxmsNv/CpK9fEVhc2J1PfrMyR28Fl4wECN2kLWnjo6YFblPiOw=","X-Received":"by 2002:a05:6402:10d5:: with SMTP id\n\tp21mr2039601edu.327.1604575759254; \n\tThu, 05 Nov 2020 03:29:19 -0800 (PST)","MIME-Version":"1.0","References":"<20201028100134.3007829-1-hiroh@chromium.org>\n\t<20201104144821.ghwyq6bsdblruvhl@uno.localdomain>","In-Reply-To":"<20201104144821.ghwyq6bsdblruvhl@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 5 Nov 2020 20:29:08 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [RFC PATCH] android: camera_device: Reorder\n\tconfigurations given by a HAL client","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]