[{"id":18911,"web_url":"https://patchwork.libcamera.org/comment/18911/","msgid":"<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>","date":"2021-08-18T11:21:44","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:\n> An Android HAL client may request identical stream requests. It is\n> redundant that a native camera device produces a separate stream for\n> each of the identical requests.\n> Configure camera one stream configuration for the identical stream\n> requests.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----\n> src/android/camera_device.h | 3 +--\n> src/android/camera_stream.cpp | 3 ++-\n> 3 files changed, 25 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 2466122d..68b2840c 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\t/* This stream will be produced by hardware. */\n> +\t\tstream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> +\n> +\t\t/*\n> +\t\t * If they are the same capture request, associate with the same\n> +\t\t * CameraStream.\n\n /*\n * If a CameraStream with the same size and format of\n * the current stream has already been requested\n * associate the two.\n */\n\n> +\t\t */\n> +\t\tauto iter = std::find_if(\n> +\t\t\tstreamConfigs.begin(), streamConfigs.end(),\n> +\t\t\t[size, format](const Camera3StreamConfig &streamConfig) {\n> +\t\t\t\treturn streamConfig.config.size == size &&\n> +\t\t\t\t streamConfig.config.pixelFormat == format;\n> +\t\t\t});\n\nSo this creates a mapped stream only if both the format AND the size\nare identical. Shouldn't we use the YUV post-processor to downscale\nNV12-to-NV12 as well ? Even if not part of this patch, the logic here\nimplemented won't work, unless the camera3_streams are presented to\nthe libcamera HAL ordered by size, doens't it ?\n\n> +\t\tif (iter != streamConfigs.end()) {\n> +\t\t\t/* Add usage to copy the buffer in streams[0] to stream. */\n> +\t\t\titer->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;\n> +\t\t\tstream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;\n\nShould the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be\noverridden ?\n\n\n> +\t\t\titer->streams.push_back({ stream, CameraStream::Type::Mapped });\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\tCamera3StreamConfig streamConfig;\n> \t\tstreamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> \t\tstreamConfig.config.size = size;\n> \t\tstreamConfig.config.pixelFormat = format;\n> \t\tstreamConfigs.push_back(std::move(streamConfig));\n> -\n> -\t\t/* This stream will be produced by hardware. */\n> -\t\tstream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n\nDepending on the answer to the previous question, should this be\nmoved ?\n\n> \t}\n>\n> \t/* Now handle the MJPEG streams, adding a new stream if required. */\n> @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tresultMetadata = std::make_unique(0, 0);\n> \t}\n>\n> -\t/* Handle any JPEG compression. */\n\nI would keep the comment as something like\n\n /* Handle post-processing. */\n\n> \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> \t\tCameraStream *cameraStream =\n> \t\t\tstatic_cast(buffer.stream->priv);\n>\n> -\t\tif (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n\nThis will accept Type::Internal. Is this intended ? Shouldn't this be\n if (cameraStream->type() != CameraStream::Type::Mapped)\n\n> \t\t\tcontinue;\n>\n> \t\tFrameBuffer *src = request->findBuffer(cameraStream->stream());\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index cbc71be4..c5f96d32 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -48,6 +48,7 @@ public:\n>\n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> +\tconst CameraCapabilities *capabilties() const { return &capabilities_; }\n\nShould be moved to 1/3\n\n> \tconst std::shared_ptr &camera() const { return camera_; }\n>\n> \tconst std::string &maker() const { return maker_; }\n> @@ -63,8 +64,6 @@ public:\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n>\n> -\tlibcamera::PixelFormat toPixelFormat(int format) const;\n> -\n\nAnd this should be dropped from 1/3\n\n> protected:\n> \tstd::string logPrefix() const override;\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 8c02cb43..d880cc18 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -8,6 +8,7 @@\n> #include \"camera_stream.h\"\n>\n> #include \"camera_buffer.h\"\n> +#include \"camera_capabilities.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> #include \"jpeg/post_processor_jpeg.h\"\n> @@ -62,7 +63,7 @@ int CameraStream::configure()\n> {\n> \tif (type_ == Type::Internal || type_ == Type::Mapped) {\n> \t\tconst PixelFormat outFormat =\n> -\t\t\tcameraDevice_->toPixelFormat(camera3Stream_->format);\n> +\t\t\tcameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);\n> \t\tStreamConfiguration output = configuration();\n> \t\toutput.pixelFormat = outFormat;\n> \t\tswitch (outFormat) {\n\nOverall a very nice step in the right direction!\n\nThanks\n j\n\n> --\n> 2.32.0.554.ge1b32706d8-goog\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 4C29FBD87C\n\tfor ;\n\tWed, 18 Aug 2021 11:20:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4F6E68895;\n\tWed, 18 Aug 2021 13:20:58 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4959A6888A\n\tfor ;\n\tWed, 18 Aug 2021 13:20:57 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 7791060006;\n\tWed, 18 Aug 2021 11:20:56 +0000 (UTC)"],"Date":"Wed, 18 Aug 2021 13:21:44 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>","References":"<20210805134530.825065-1-hiroh@chromium.org>\n\t<20210805134530.825065-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210805134530.825065-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18954,"web_url":"https://patchwork.libcamera.org/comment/18954/","msgid":"","date":"2021-08-19T20:10:03","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Wed, Aug 18, 2021 at 8:20 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:\n> > An Android HAL client may request identical stream requests. It is\n> > redundant that a native camera device produces a separate stream for\n> > each of the identical requests.\n> > Configure camera one stream configuration for the identical stream\n> > requests.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----\n> > src/android/camera_device.h | 3 +--\n> > src/android/camera_stream.cpp | 3 ++-\n> > 3 files changed, 25 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 2466122d..68b2840c 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > continue;\n> > }\n> >\n> > + /* This stream will be produced by hardware. */\n> > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > +\n> > + /*\n> > + * If they are the same capture request, associate with the same\n> > + * CameraStream.\n>\n> /*\n> * If a CameraStream with the same size and format of\n> * the current stream has already been requested\n> * associate the two.\n> */\n>\n> > + */\n> > + auto iter = std::find_if(\n> > + streamConfigs.begin(), streamConfigs.end(),\n> > + [size, format](const Camera3StreamConfig &streamConfig) {\n> > + return streamConfig.config.size == size &&\n> > + streamConfig.config.pixelFormat == format;\n> > + });\n>\n> So this creates a mapped stream only if both the format AND the size\n> are identical. Shouldn't we use the YUV post-processor to downscale\n> NV12-to-NV12 as well ?\n\nRight but using YUV-post processor for down-scaling is not a goal of\nthis patch series.\nI would not do it in this patch series.\n\n> Even if not part of this patch, the logic here\n> implemented won't work, unless the camera3_streams are presented to\n> the libcamera HAL ordered by size, doens't it ?\n\nI don't understand why this won't work unless that.\nCould you explain me more?\n\n>\n> > + if (iter != streamConfigs.end()) {\n> > + /* Add usage to copy the buffer in streams[0] to stream. */\n> > + iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;\n> > + stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;\n>\n> Should the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be\n> overridden ?\n>\n>\n> > + iter->streams.push_back({ stream, CameraStream::Type::Mapped });\n> > + continue;\n> > + }\n> > +\n> > Camera3StreamConfig streamConfig;\n> > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > streamConfig.config.size = size;\n> > streamConfig.config.pixelFormat = format;\n> > streamConfigs.push_back(std::move(streamConfig));\n> > -\n> > - /* This stream will be produced by hardware. */\n> > - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n>\n> Depending on the answer to the previous question, should this be\n> moved ?\n>\n\nIf I drop this usage, a buffer fails to be mapped because\ncros::CameraBufferManager requires the usage upon gbm_bo_import().\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=667;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\nI think the usage is mandatory.\n\n> > }\n> >\n> > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)\n> > resultMetadata = std::make_unique(0, 0);\n> > }\n> >\n> > - /* Handle any JPEG compression. */\n>\n> I would keep the comment as something like\n>\n> /* Handle post-processing. */\n>\n> > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > CameraStream *cameraStream =\n> > static_cast(buffer.stream->priv);\n> >\n> > - if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> > + if (cameraStream->type() == CameraStream::Type::Direct)\n>\n> This will accept Type::Internal. Is this intended ? Shouldn't this be\n> if (cameraStream->type() != CameraStream::Type::Mapped)\n>\n\nJPEG processing case is Internal. We need to handle Mapped and Internal.\n\n\nBest Regards,\n-Hiro\n\n\n> > continue;\n> >\n> > FrameBuffer *src = request->findBuffer(cameraStream->stream());\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index cbc71be4..c5f96d32 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -48,6 +48,7 @@ public:\n> >\n> > unsigned int id() const { return id_; }\n> > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > + const CameraCapabilities *capabilties() const { return &capabilities_; }\n>\n> Should be moved to 1/3\n>\n> > const std::shared_ptr &camera() const { return camera_; }\n> >\n> > const std::string &maker() const { return maker_; }\n> > @@ -63,8 +64,6 @@ public:\n> > int processCaptureRequest(camera3_capture_request_t *request);\n> > void requestComplete(libcamera::Request *request);\n> >\n> > - libcamera::PixelFormat toPixelFormat(int format) const;\n> > -\n>\n> And this should be dropped from 1/3\n>\n> > protected:\n> > std::string logPrefix() const override;\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 8c02cb43..d880cc18 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"camera_stream.h\"\n> >\n> > #include \"camera_buffer.h\"\n> > +#include \"camera_capabilities.h\"\n> > #include \"camera_device.h\"\n> > #include \"camera_metadata.h\"\n> > #include \"jpeg/post_processor_jpeg.h\"\n> > @@ -62,7 +63,7 @@ int CameraStream::configure()\n> > {\n> > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > const PixelFormat outFormat =\n> > - cameraDevice_->toPixelFormat(camera3Stream_->format);\n> > + cameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);\n> > StreamConfiguration output = configuration();\n> > output.pixelFormat = outFormat;\n> > switch (outFormat) {\n>\n> Overall a very nice step in the right direction!\n>\n> Thanks\n> j\n>\n> > --\n> > 2.32.0.554.ge1b32706d8-goog\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 46AFABD87C\n\tfor ;\n\tThu, 19 Aug 2021 20:10:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98A86688A3;\n\tThu, 19 Aug 2021 22:10:19 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B30860264\n\tfor ;\n\tThu, 19 Aug 2021 22:10:17 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id bt14so15314521ejb.3\n\tfor ;\n\tThu, 19 Aug 2021 13:10:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"e3ORs1aV\"; 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=eN+aGRT/0nVxk6hhDy9k8N48yTvPtFekaOEYF4ATQVk=;\n\tb=e3ORs1aVqhmK4oBzwkiL6GXqTrt++Rf+LvJSGjoJm9TENJF0W7EI3Z3SuzR5xfvXbB\n\tJGemZNI/4vGPusXcYAwxy+9Lx0uHeKUWehkjIz1IAmMyZEWxKFnHuCgk31qTFMtaVfmN\n\tni4Fq8mECKncI4tznsO4PBJ6BYFdFFaeY8YTM=","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=eN+aGRT/0nVxk6hhDy9k8N48yTvPtFekaOEYF4ATQVk=;\n\tb=KVw0ZhaWZkP71VZY7ZViGh8/LLlLEZNjKzElaTpLghfO3vIm+rPU7SlATyNzD5VSM+\n\t8/+djV0zid3T5vt9sANHfaUzua3T9DYazoSuqcnDlZEqNG3KTEJGFtCLrbUhv8Aa8HSj\n\tJWW8BYpAML07E+W45PjTLSM36TL6bZed7TTHvGOWlrK+Hlu7ntb/xgBBUCZDo1xS/qhg\n\t+v3MjH4CECTDZ4RN2tX7fF2ulP1kkVY6/6UV6MbKP1XtceUkhk1AGi6EDO54LmsFaB+O\n\tlxGv8NrOwJPIxRDW8/xA3Iku+C+NNdsMrjtWmWBP3pYESaNvH9i0Mhn9bf3+MM66TBEF\n\t/1ZQ==","X-Gm-Message-State":"AOAM531dwmsBc20YtcmqqLgjVrKwkGLBDHU3bMGVJyRQ+kf9Z8FEy4EB\n\tJYhWdKgzW9aL5bNssL2i2g9BWuwdCkBYFPdho/QEIQ==","X-Google-Smtp-Source":"ABdhPJyt+OMxUqXmirTxvX5l0kxh2dEe0pD95y/nUfxqSRq9Kzzonrgy30jfxhQQ9yNt+ScZ9dnkHFHgsO1J0eWUI2U=","X-Received":"by 2002:a17:906:4890:: with SMTP id\n\tv16mr17010521ejq.42.1629403816325; \n\tThu, 19 Aug 2021 13:10:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210805134530.825065-1-hiroh@chromium.org>\n\t<20210805134530.825065-4-hiroh@chromium.org>\n\t<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>","In-Reply-To":"<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 20 Aug 2021 05:10:03 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18981,"web_url":"https://patchwork.libcamera.org/comment/18981/","msgid":"<20210820135414.lesdp5c3trdstbey@uno.localdomain>","date":"2021-08-20T13:54:14","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, Aug 20, 2021 at 05:10:03AM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n>\n> On Wed, Aug 18, 2021 at 8:20 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:\n> > > An Android HAL client may request identical stream requests. It is\n> > > redundant that a native camera device produces a separate stream for\n> > > each of the identical requests.\n> > > Configure camera one stream configuration for the identical stream\n> > > requests.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----\n> > > src/android/camera_device.h | 3 +--\n> > > src/android/camera_stream.cpp | 3 ++-\n> > > 3 files changed, 25 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 2466122d..68b2840c 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > continue;\n> > > }\n> > >\n> > > + /* This stream will be produced by hardware. */\n> > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > > +\n> > > + /*\n> > > + * If they are the same capture request, associate with the same\n> > > + * CameraStream.\n> >\n> > /*\n> > * If a CameraStream with the same size and format of\n> > * the current stream has already been requested\n> > * associate the two.\n> > */\n> >\n> > > + */\n> > > + auto iter = std::find_if(\n> > > + streamConfigs.begin(), streamConfigs.end(),\n> > > + [size, format](const Camera3StreamConfig &streamConfig) {\n> > > + return streamConfig.config.size == size &&\n> > > + streamConfig.config.pixelFormat == format;\n> > > + });\n> >\n> > So this creates a mapped stream only if both the format AND the size\n> > are identical. Shouldn't we use the YUV post-processor to downscale\n> > NV12-to-NV12 as well ?\n>\n> Right but using YUV-post processor for down-scaling is not a goal of\n> this patch series.\n> I would not do it in this patch series.\n\nThat's fine! I was not asking for this, but it was related to my\nquestion below.\n\n>\n> > Even if not part of this patch, the logic here\n> > implemented won't work, unless the camera3_streams are presented to\n> > the libcamera HAL ordered by size, doens't it ?\n>\n> I don't understand why this won't work unless that.\n> Could you explain me more?\n>\n\nIf the streams requested by the HAL client are not ordered from the\nbigger to the smaller it won't be possible to here identify what\nstream can be generated by downscaling another one.\n\nIn other words: if we sort streams by size, will it be possible here\nto get YUV downscaling by simply searching for a YUV stream which is\nbigger in size and map the current on to it ?\n\n> >\n> > > + if (iter != streamConfigs.end()) {\n> > > + /* Add usage to copy the buffer in streams[0] to stream. */\n> > > + iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;\n> > > + stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;\n> >\n> > Should the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be\n> > overridden ?\n> >\n> >\n> > > + iter->streams.push_back({ stream, CameraStream::Type::Mapped });\n> > > + continue;\n> > > + }\n> > > +\n> > > Camera3StreamConfig streamConfig;\n> > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > streamConfig.config.size = size;\n> > > streamConfig.config.pixelFormat = format;\n> > > streamConfigs.push_back(std::move(streamConfig));\n> > > -\n> > > - /* This stream will be produced by hardware. */\n> > > - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> >\n> > Depending on the answer to the previous question, should this be\n> > moved ?\n> >\n>\n> If I drop this usage, a buffer fails to be mapped because\n> cros::CameraBufferManager requires the usage upon gbm_bo_import().\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=667;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n> I think the usage is mandatory.\n\nFine with me, I was asking to check if this was intentional or not.\n\n>\n> > > }\n> > >\n> > > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > > @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)\n> > > resultMetadata = std::make_unique(0, 0);\n> > > }\n> > >\n> > > - /* Handle any JPEG compression. */\n> >\n> > I would keep the comment as something like\n> >\n> > /* Handle post-processing. */\n> >\n> > > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > CameraStream *cameraStream =\n> > > static_cast(buffer.stream->priv);\n> > >\n> > > - if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> > > + if (cameraStream->type() == CameraStream::Type::Direct)\n> >\n> > This will accept Type::Internal. Is this intended ? Shouldn't this be\n> > if (cameraStream->type() != CameraStream::Type::Mapped)\n> >\n>\n> JPEG processing case is Internal. We need to handle Mapped and Internal.\n\nYou're right, it can be internal and this is the same check which is\nnow performed in CameraStream::configure().\n\n>\n>\n> Best Regards,\n> -Hiro\n>\n>\n> > > continue;\n> > >\n> > > FrameBuffer *src = request->findBuffer(cameraStream->stream());\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index cbc71be4..c5f96d32 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -48,6 +48,7 @@ public:\n> > >\n> > > unsigned int id() const { return id_; }\n> > > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > > + const CameraCapabilities *capabilties() const { return &capabilities_; }\n> >\n> > Should be moved to 1/3\n> >\n> > > const std::shared_ptr &camera() const { return camera_; }\n> > >\n> > > const std::string &maker() const { return maker_; }\n> > > @@ -63,8 +64,6 @@ public:\n> > > int processCaptureRequest(camera3_capture_request_t *request);\n> > > void requestComplete(libcamera::Request *request);\n> > >\n> > > - libcamera::PixelFormat toPixelFormat(int format) const;\n> > > -\n> >\n> > And this should be dropped from 1/3\n> >\n> > > protected:\n> > > std::string logPrefix() const override;\n> > >\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index 8c02cb43..d880cc18 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \"camera_stream.h\"\n> > >\n> > > #include \"camera_buffer.h\"\n> > > +#include \"camera_capabilities.h\"\n> > > #include \"camera_device.h\"\n> > > #include \"camera_metadata.h\"\n> > > #include \"jpeg/post_processor_jpeg.h\"\n> > > @@ -62,7 +63,7 @@ int CameraStream::configure()\n> > > {\n> > > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > > const PixelFormat outFormat =\n> > > - cameraDevice_->toPixelFormat(camera3Stream_->format);\n> > > + cameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);\n> > > StreamConfiguration output = configuration();\n> > > output.pixelFormat = outFormat;\n> > > switch (outFormat) {\n> >\n> > Overall a very nice step in the right direction!\n> >\n> > Thanks\n> > j\n> >\n> > > --\n> > > 2.32.0.554.ge1b32706d8-goog\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 CEF25BD87D\n\tfor ;\n\tFri, 20 Aug 2021 13:53:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16C30688A2;\n\tFri, 20 Aug 2021 15:53:30 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 910E86888E\n\tfor ;\n\tFri, 20 Aug 2021 15:53:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 7EF4460005;\n\tFri, 20 Aug 2021 13:53:27 +0000 (UTC)"],"Date":"Fri, 20 Aug 2021 15:54:14 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210820135414.lesdp5c3trdstbey@uno.localdomain>","References":"<20210805134530.825065-1-hiroh@chromium.org>\n\t<20210805134530.825065-4-hiroh@chromium.org>\n\t<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18996,"web_url":"https://patchwork.libcamera.org/comment/18996/","msgid":"","date":"2021-08-23T07:50:33","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"HI Jacopo, thank you for reply.\n\nOn Fri, Aug 20, 2021 at 10:53 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Aug 20, 2021 at 05:10:03AM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for reviewing.\n> >\n> > On Wed, Aug 18, 2021 at 8:20 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:\n> > > > An Android HAL client may request identical stream requests. It is\n> > > > redundant that a native camera device produces a separate stream for\n> > > > each of the identical requests.\n> > > > Configure camera one stream configuration for the identical stream\n> > > > requests.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----\n> > > > src/android/camera_device.h | 3 +--\n> > > > src/android/camera_stream.cpp | 3 ++-\n> > > > 3 files changed, 25 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 2466122d..68b2840c 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > continue;\n> > > > }\n> > > >\n> > > > + /* This stream will be produced by hardware. */\n> > > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > > > +\n> > > > + /*\n> > > > + * If they are the same capture request, associate with the same\n> > > > + * CameraStream.\n> > >\n> > > /*\n> > > * If a CameraStream with the same size and format of\n> > > * the current stream has already been requested\n> > > * associate the two.\n> > > */\n> > >\n> > > > + */\n> > > > + auto iter = std::find_if(\n> > > > + streamConfigs.begin(), streamConfigs.end(),\n> > > > + [size, format](const Camera3StreamConfig &streamConfig) {\n> > > > + return streamConfig.config.size == size &&\n> > > > + streamConfig.config.pixelFormat == format;\n> > > > + });\n> > >\n> > > So this creates a mapped stream only if both the format AND the size\n> > > are identical. Shouldn't we use the YUV post-processor to downscale\n> > > NV12-to-NV12 as well ?\n> >\n> > Right but using YUV-post processor for down-scaling is not a goal of\n> > this patch series.\n> > I would not do it in this patch series.\n>\n> That's fine! I was not asking for this, but it was related to my\n> question below.\n>\n> >\n> > > Even if not part of this patch, the logic here\n> > > implemented won't work, unless the camera3_streams are presented to\n> > > the libcamera HAL ordered by size, doens't it ?\n> >\n> > I don't understand why this won't work unless that.\n> > Could you explain me more?\n> >\n>\n> If the streams requested by the HAL client are not ordered from the\n> bigger to the smaller it won't be possible to here identify what\n> stream can be generated by downscaling another one.\n>\n> In other words: if we sort streams by size, will it be possible here\n> to get YUV downscaling by simply searching for a YUV stream which is\n> bigger in size and map the current on to it ?\n>\n\nNote that sorting is done in sortCamera3StreamConfigs().\nSearching for a stream to be down-scaled should not be done here.\nIt is because a native camera may be able to produce the multiple streams.\nThis search for the best configured streams is achievable with a\nreprocessing configuration, which will be done by Laurent.\nIt is questionable whether squashing the streams should also be done\nin the reprocessing process.\nI asked it in IRC, and I was answered that it should be done\nbeforehand regardless of the reprocessing configuration.\n\n+Laurent Pinchart what do you think?\n\nBest Regards,\n-Hiro\n> > >\n> > > > + if (iter != streamConfigs.end()) {\n> > > > + /* Add usage to copy the buffer in streams[0] to stream. */\n> > > > + iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;\n> > > > + stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;\n> > >\n> > > Should the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be\n> > > overridden ?\n> > >\n> > >\n> > > > + iter->streams.push_back({ stream, CameraStream::Type::Mapped });\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > Camera3StreamConfig streamConfig;\n> > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > streamConfig.config.size = size;\n> > > > streamConfig.config.pixelFormat = format;\n> > > > streamConfigs.push_back(std::move(streamConfig));\n> > > > -\n> > > > - /* This stream will be produced by hardware. */\n> > > > - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > >\n> > > Depending on the answer to the previous question, should this be\n> > > moved ?\n> > >\n> >\n> > If I drop this usage, a buffer fails to be mapped because\n> > cros::CameraBufferManager requires the usage upon gbm_bo_import().\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=667;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n> > I think the usage is mandatory.\n>\n> Fine with me, I was asking to check if this was intentional or not.\n>\n> >\n> > > > }\n> > > >\n> > > > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > > > @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)\n> > > > resultMetadata = std::make_unique(0, 0);\n> > > > }\n> > > >\n> > > > - /* Handle any JPEG compression. */\n> > >\n> > > I would keep the comment as something like\n> > >\n> > > /* Handle post-processing. */\n> > >\n> > > > for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > CameraStream *cameraStream =\n> > > > static_cast(buffer.stream->priv);\n> > > >\n> > > > - if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> > > > + if (cameraStream->type() == CameraStream::Type::Direct)\n> > >\n> > > This will accept Type::Internal. Is this intended ? Shouldn't this be\n> > > if (cameraStream->type() != CameraStream::Type::Mapped)\n> > >\n> >\n> > JPEG processing case is Internal. We need to handle Mapped and Internal.\n>\n> You're right, it can be internal and this is the same check which is\n> now performed in CameraStream::configure().\n>\n> >\n> >\n> > Best Regards,\n> > -Hiro\n> >\n> >\n> > > > continue;\n> > > >\n> > > > FrameBuffer *src = request->findBuffer(cameraStream->stream());\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index cbc71be4..c5f96d32 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -48,6 +48,7 @@ public:\n> > > >\n> > > > unsigned int id() const { return id_; }\n> > > > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > > > + const CameraCapabilities *capabilties() const { return &capabilities_; }\n> > >\n> > > Should be moved to 1/3\n> > >\n> > > > const std::shared_ptr &camera() const { return camera_; }\n> > > >\n> > > > const std::string &maker() const { return maker_; }\n> > > > @@ -63,8 +64,6 @@ public:\n> > > > int processCaptureRequest(camera3_capture_request_t *request);\n> > > > void requestComplete(libcamera::Request *request);\n> > > >\n> > > > - libcamera::PixelFormat toPixelFormat(int format) const;\n> > > > -\n> > >\n> > > And this should be dropped from 1/3\n> > >\n> > > > protected:\n> > > > std::string logPrefix() const override;\n> > > >\n> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > index 8c02cb43..d880cc18 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -8,6 +8,7 @@\n> > > > #include \"camera_stream.h\"\n> > > >\n> > > > #include \"camera_buffer.h\"\n> > > > +#include \"camera_capabilities.h\"\n> > > > #include \"camera_device.h\"\n> > > > #include \"camera_metadata.h\"\n> > > > #include \"jpeg/post_processor_jpeg.h\"\n> > > > @@ -62,7 +63,7 @@ int CameraStream::configure()\n> > > > {\n> > > > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > > > const PixelFormat outFormat =\n> > > > - cameraDevice_->toPixelFormat(camera3Stream_->format);\n> > > > + cameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);\n> > > > StreamConfiguration output = configuration();\n> > > > output.pixelFormat = outFormat;\n> > > > switch (outFormat) {\n> > >\n> > > Overall a very nice step in the right direction!\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > --\n> > > > 2.32.0.554.ge1b32706d8-goog\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 03133BD87C\n\tfor ;\n\tMon, 23 Aug 2021 07:50:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D1CD688A3;\n\tMon, 23 Aug 2021 09:50:50 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4025D68891\n\tfor ;\n\tMon, 23 Aug 2021 09:50:46 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id me10so6721029ejb.11\n\tfor ;\n\tMon, 23 Aug 2021 00:50:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hBMUrKBz\"; 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=OJFOOnxa/CiA71OUn+jeejC6NNUVWYfWBZLCoP3ILXw=;\n\tb=hBMUrKBzSr71ZQXds7AmoNzK2Znqt3ibujUc+2HHBX7ozbGjy6Qx/vSr4c5QBbXpcX\n\tDdazVWDB++loksSYFReP9pIkwP+d0h/SNWEL0wzpJjv2QrIPQTKbniFSFJ+GnroEKp3l\n\ttdG3Xzh2xP8q8vfcy4HPVzG60M9Y12P+U1QpY=","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=OJFOOnxa/CiA71OUn+jeejC6NNUVWYfWBZLCoP3ILXw=;\n\tb=lb8KEyxP4BZOlij3yRo+GxjxNV5beUeIj9q6fX3EFEPBufHVS/m6L5791/MQxzKZr+\n\twEkKg2Oig1WAsVE8y29bTBxgfP8dFGJZ8Am/q3/hJvXq0F9InRgeH/k27hKU1mJrzibH\n\tDRHScPDWdlykbA9+uNo5NFAZpuo65nAgQ++FpWs34qCNoQDe1Jc/BPEwu0X23kHt8kLF\n\tpbdKuiRAMtpT62kkn7nucWy1eaJvEEQ4s20ZhoWnhh3CLY8wXKgdF8NJ0Ro+eKpiA6/5\n\tSmqpydHfoZzSemtIFZHSajuyXGfK32qCRRLrA+td61HIL6Wbieqe4c2ZVB2eAej5bz93\n\tBeAg==","X-Gm-Message-State":"AOAM532lVI7HfBVM1DZNv8XonTZRszevhQl+gXLQTTLP1krp4OhzkPY4\n\tYRBLIZMIsfXX5X7VXgAKqr6asaj/VpgV8D9xc4T69Q==","X-Google-Smtp-Source":"ABdhPJyM5BFjEY/UggyfXnB/CNK8c0BWtl6OFNn8N+9jGPYTk0/bAac0765V/4vfF+DdpM+0hS47xZXRHeJmUHQrQgY=","X-Received":"by 2002:a17:906:90d9:: with SMTP id\n\tv25mr33662444ejw.221.1629705045729; \n\tMon, 23 Aug 2021 00:50:45 -0700 (PDT)","MIME-Version":"1.0","References":"<20210805134530.825065-1-hiroh@chromium.org>\n\t<20210805134530.825065-4-hiroh@chromium.org>\n\t<20210818112144.tntmkh5cgpadaqjl@uno.localdomain>\n\t\n\t<20210820135414.lesdp5c3trdstbey@uno.localdomain>","In-Reply-To":"<20210820135414.lesdp5c3trdstbey@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 23 Aug 2021 16:50:33 +0900","Message-ID":"","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Configure\n\tone stream for identical stream requests","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]