[{"id":19324,"web_url":"https://patchwork.libcamera.org/comment/19324/","msgid":"","date":"2021-09-03T08:36:29","subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one stream for identical stream requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote:\n> An Android HAL client may request identical stream requests. It is\n\ns/identical stream requests/multiple identical streams/\n\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\nAnd here I'd write\n\nConfigure the camera with a single stream in that case. The other\nidentical HAL streams will be produced by the YUV post-processor.\n\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++-\n> 1 file changed, 27 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 324b997f..51d5370e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on\n> +\t\t * ChromeOS because cros::CameraBufferManager imports the stream\n> +\t\t * buffer with the usage.\n\nI'm still not sure to follow you. What happens if you don't set\nGRALLOC_USAGE_HW_CAMERA_WRITE ?\n\n> +\t\t */\n> +#if defined(OS_CHROMEOS)\n> +\t\tstream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> +#endif\n> +\n> +\t\t/*\n> +\t\t * If a CameraStream with the same size and format of the\n> +\t\t * current stream has already been requested, associate the two.\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> +\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> +\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\nThis isn't needed.\n\n> \t\t/* This stream will be produced by hardware. */\n> \t\tstream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> \t}","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 1A42FBDC71\n\tfor ;\n\tFri, 3 Sep 2021 08:36:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F9686916B;\n\tFri, 3 Sep 2021 10:36:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDE8B69165\n\tfor ;\n\tFri, 3 Sep 2021 10:36:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41052BBE;\n\tFri, 3 Sep 2021 10:36:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GLbPA1Vp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630658206;\n\tbh=9n5x0rtnm763UxVlhKIP0b9peXI5nZ2xlG4Kp3ku/kM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GLbPA1Vp4PUub9hjKjadyrWyYQ9RWC1adCn+4zQNgYllw0SeIysQrIrRNo5YE7OpY\n\tWeNfz7VTtlEWGFGgmEg6C4mXQD4n/foNPwB495NB2Cp3y5Aojx/G5TOooInUB6aL9m\n\tJpaXpfN+PY/zzDls1OXqolhfMDllhqF4JvX2uDT0=","Date":"Fri, 3 Sep 2021 11:36:29 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210901080302.68525-1-hiroh@chromium.org>\n\t<20210901080302.68525-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901080302.68525-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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":19334,"web_url":"https://patchwork.libcamera.org/comment/19334/","msgid":"","date":"2021-09-03T09:11:30","subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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 Laurent,\n\nOn Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote:\n> > An Android HAL client may request identical stream requests. It is\n>\n> s/identical stream requests/multiple identical streams/\n>\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> And here I'd write\n>\n> Configure the camera with a single stream in that case. The other\n> identical HAL streams will be produced by the YUV post-processor.\n>\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++-\n> > 1 file changed, 27 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 324b997f..51d5370e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > continue;\n> > }\n> >\n> > + /*\n> > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on\n> > + * ChromeOS because cros::CameraBufferManager imports the stream\n> > + * buffer with the usage.\n>\n> I'm still not sure to follow you. What happens if you don't set\n> GRALLOC_USAGE_HW_CAMERA_WRITE ?\n\nThis gmb_bo_import call fails. That is, the buffer cannot be mapped\nand even constructing CameraBuffer fails.\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n\nWell, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand\nthey want to add it for safety.\n\n-Hiro\n>\n> > + */\n> > +#if defined(OS_CHROMEOS)\n> > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > +#endif\n> > +\n> > + /*\n> > + * If a CameraStream with the same size and format of the\n> > + * current stream has already been requested, associate the two.\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> > + 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> > + 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>\n> This isn't needed.\n>\n> > /* This stream will be produced by hardware. */\n> > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 D3417BDC71\n\tfor ;\n\tFri, 3 Sep 2021 09:11:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3ED406916A;\n\tFri, 3 Sep 2021 11:11:43 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2FD560251\n\tfor ;\n\tFri, 3 Sep 2021 11:11:41 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id j13so7031223edv.13\n\tfor ;\n\tFri, 03 Sep 2021 02:11:41 -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=\"QU/V+QUB\"; 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=CYRmaxeaFG/k7YBtG25EUfhnCsWlcjja9bfrD5MFXpw=;\n\tb=QU/V+QUBw9y6r5DtKKDmkGgnRNzj5TQXEms2wtauPbkFQcJI/oo51LRLZxsqWtmh3W\n\tdTHdquv/zKIexxgivnznP32XR1URse80bU3DsXp4IRqF5P/YZkygqJxsTbh12WCCcDM1\n\tXqWC9rJOn3BLhA9cYx/QBBefO+YDhVb/3O9WQ=","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=CYRmaxeaFG/k7YBtG25EUfhnCsWlcjja9bfrD5MFXpw=;\n\tb=VTv1rjCOTCDpGsv83EvGzOun1WjD9xSslRSXOH1Kfd2xQVV8DDkBc/AicA/nUhxQsj\n\tnXOT0Mj4O5SWiUG+EOH2niBHk2P+9Nhh30A1Yb4gIB9hmjjaOgfQBlB45akkxn4p/27y\n\t4aFfvgNft9FUfBxZLYeaN0P15Wx13G51UWzxDwH1E7NVmoZYk+xFSKiThy9mTrIUuSDy\n\t6PHYelVqLSLlNI89YYS4Xk3U6Ysw58esMO5jgo3IaeVojjeC2BQl21l8KxZTwCM5ZK0n\n\tG1VtKZ6yt2/3JjiruNEEYAK2e+57UJGk2jlW/+cY7TAQIDBQlxuAnWJJNv3P23eyDlc6\n\tfQPA==","X-Gm-Message-State":"AOAM531lgl4CrkWeIl6jDo89L2Iog5pteVSwUEmpt2XQlq6VPezFAs90\n\tDQ620I3GoaeTLNnqQuNPprvmJleoLhTbrPt4zk6mNFUmcQqeCg==","X-Google-Smtp-Source":"ABdhPJzAxn6HNBdW6dteR5CjZgQVfXMnXurIi7vg2gOIPcPaOwa02CbKFUcPV0VYGQXaQWcaxt4YBCwvI0GqDXMlh/Y=","X-Received":"by 2002:aa7:cd92:: with SMTP id\n\tx18mr2899512edv.325.1630660301502; \n\tFri, 03 Sep 2021 02:11:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20210901080302.68525-1-hiroh@chromium.org>\n\t<20210901080302.68525-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 3 Sep 2021 18:11:30 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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":19762,"web_url":"https://patchwork.libcamera.org/comment/19762/","msgid":"","date":"2021-09-22T00:54:17","subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one stream for identical stream requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nSorry for the late reply.\n\nOn Fri, Sep 03, 2021 at 06:11:30PM +0900, Hirokazu Honda wrote:\n> On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart wrote:\n> > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote:\n> > > An Android HAL client may request identical stream requests. It is\n> >\n> > s/identical stream requests/multiple identical streams/\n> >\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> > And here I'd write\n> >\n> > Configure the camera with a single stream in that case. The other\n> > identical HAL streams will be produced by the YUV post-processor.\n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++-\n> > > 1 file changed, 27 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 324b997f..51d5370e 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > continue;\n> > > }\n> > >\n> > > + /*\n> > > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on\n> > > + * ChromeOS because cros::CameraBufferManager imports the stream\n> > > + * buffer with the usage.\n> >\n> > I'm still not sure to follow you. What happens if you don't set\n> > GRALLOC_USAGE_HW_CAMERA_WRITE ?\n> \n> This gmb_bo_import call fails. That is, the buffer cannot be mapped\n> and even constructing CameraBuffer fails.\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n> \n> Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand\n> they want to add it for safety.\n\nRight. This bothers me a bit, but it's not your fault :-) The gralloc\nusage flags are supposed to indicate expected usage of a buffer to allow\nselection of a suitable memory allocation strategy compatible with all\ndevices involved. In practice, it's also used to pick pixel formats, so\nwe need to specify GRALLOC_USAGE_HW_CAMERA_WRITE for all buffers that\nwill be filled by the camera HAL, or the wrong format will be selected\n(or allocation could fail completely). That's a design deficiency of the\ngralloc and camera HAL APIs in my opinion, but not something we can\naddress.\n\nI expect the same to be true for other Android system, so I think it\nwould make sense to set GRALLOC_USAGE_HW_CAMERA_WRITE unconditionally.\nHow about this ?\n\n\t\t/*\n\t\t * While gralloc usage flags are supposed to report usage\n\t\t * patterns to select a suitable buffer allocation strategy, in\n\t\t * practive they're also used to make other decisions, such as\n\t\t * selecting the actual format for the IMPLEMENTATION_DEFINED\n\t\t * HAL pixel format. To avoid issues, we thus have to set the\n\t\t * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for\n\t\t * streams that will be produced in software.\n\t\t */\n\t\tstream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n\nIf you're fine with this, I'll push the series with this change.\n\n> > > + */\n> > > +#if defined(OS_CHROMEOS)\n> > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > > +#endif\n> > > +\n> > > + /*\n> > > + * If a CameraStream with the same size and format of the\n> > > + * current stream has already been requested, associate the two.\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> > > + 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> > > + 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> >\n> > This isn't needed.\n> >\n> > > /* This stream will be produced by hardware. */\n> > > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\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 75A32BF01C\n\tfor ;\n\tWed, 22 Sep 2021 00:54:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8DB16918C;\n\tWed, 22 Sep 2021 02:54:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D438B69186\n\tfor ;\n\tWed, 22 Sep 2021 02:54:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3F196291;\n\tWed, 22 Sep 2021 02:54:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qySH5Rok\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632272088;\n\tbh=RUIVk0cpwYc2AiYJhYwB1wSg6k7N1mKJI/kYf3Mq150=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qySH5Rok2NAqrzfeiN6E9BJHCiykTbqbFWeojG9gHs0PbhkJ4W56m7hdfZB9MzsTz\n\tzAt1dmnGTiC31bu75mCDYzxO5DgjPh3AqjbTzrxpvknqTP7XbvoSzQoKPyhWubvZ7s\n\tcI/FzrldtCkBNEPARhM3kKnHqFhzhVyapM5Okxi0=","Date":"Wed, 22 Sep 2021 03:54:17 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210901080302.68525-1-hiroh@chromium.org>\n\t<20210901080302.68525-3-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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":19765,"web_url":"https://patchwork.libcamera.org/comment/19765/","msgid":"","date":"2021-09-22T03:21:32","subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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 Laurent,\n\nOn Wed, Sep 22, 2021 at 9:54 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Sorry for the late reply.\n>\n> On Fri, Sep 03, 2021 at 06:11:30PM +0900, Hirokazu Honda wrote:\n> > On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart wrote:\n> > > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote:\n> > > > An Android HAL client may request identical stream requests. It is\n> > >\n> > > s/identical stream requests/multiple identical streams/\n> > >\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> > > And here I'd write\n> > >\n> > > Configure the camera with a single stream in that case. The other\n> > > identical HAL streams will be produced by the YUV post-processor.\n> > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++-\n> > > > 1 file changed, 27 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 324b997f..51d5370e 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > continue;\n> > > > }\n> > > >\n> > > > + /*\n> > > > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on\n> > > > + * ChromeOS because cros::CameraBufferManager imports the stream\n> > > > + * buffer with the usage.\n> > >\n> > > I'm still not sure to follow you. What happens if you don't set\n> > > GRALLOC_USAGE_HW_CAMERA_WRITE ?\n> >\n> > This gmb_bo_import call fails. That is, the buffer cannot be mapped\n> > and even constructing CameraBuffer fails.\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e\n> >\n> > Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand\n> > they want to add it for safety.\n>\n> Right. This bothers me a bit, but it's not your fault :-) The gralloc\n> usage flags are supposed to indicate expected usage of a buffer to allow\n> selection of a suitable memory allocation strategy compatible with all\n> devices involved. In practice, it's also used to pick pixel formats, so\n> we need to specify GRALLOC_USAGE_HW_CAMERA_WRITE for all buffers that\n> will be filled by the camera HAL, or the wrong format will be selected\n> (or allocation could fail completely). That's a design deficiency of the\n> gralloc and camera HAL APIs in my opinion, but not something we can\n> address.\n>\n> I expect the same to be true for other Android system, so I think it\n> would make sense to set GRALLOC_USAGE_HW_CAMERA_WRITE unconditionally.\n> How about this ?\n>\n> /*\n> * While gralloc usage flags are supposed to report usage\n> * patterns to select a suitable buffer allocation strategy, in\n> * practive they're also used to make other decisions, such as\n\ns/practive/practice/.\n\n> * selecting the actual format for the IMPLEMENTATION_DEFINED\n> * HAL pixel format. To avoid issues, we thus have to set the\n> * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for\n> * streams that will be produced in software.\n> */\n> stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n>\n> If you're fine with this, I'll push the series with this change.\n>\n\nThe comment desciption looks good. Thanks.\n\n-Hiro\n> > > > + */\n> > > > +#if defined(OS_CHROMEOS)\n> > > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > > > +#endif\n> > > > +\n> > > > + /*\n> > > > + * If a CameraStream with the same size and format of the\n> > > > + * current stream has already been requested, associate the two.\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> > > > + 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> > > > + 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> > >\n> > > This isn't needed.\n> > >\n> > > > /* This stream will be produced by hardware. */\n> > > > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;\n> > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 BF90FBF01C\n\tfor ;\n\tWed, 22 Sep 2021 03:21:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E52B6918C;\n\tWed, 22 Sep 2021 05:21:45 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E78E6012C\n\tfor ;\n\tWed, 22 Sep 2021 05:21:44 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id u27so4235021edi.9\n\tfor ;\n\tTue, 21 Sep 2021 20:21:44 -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=\"iCJo9czh\"; 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=TPS6ZabkweEc7fmUASs+4gLo91X1a3xv9mSAUG0pDyw=;\n\tb=iCJo9czhxGKZSfRiNQVTUQ2zIU2I3keOkdYph0l1QP0W05+PeaYBVFmuAu+t1OrRHS\n\t6RYBW7oUJ5otj+KEZvGLQ2pQ3z0XlFzbE1pAhZc1rK+XgpgzStiGlrN2uwpztqixbgIX\n\thX11VinJpiTJf/tiuDFH2hnZIA+8TKLeq3BL4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TPS6ZabkweEc7fmUASs+4gLo91X1a3xv9mSAUG0pDyw=;\n\tb=xAE2ajw5XBIe4tFxHs4/Te8gtFdpbSiohSttRav04FK6NqB0laVaQ0CWGgWbhuGxmm\n\t37KGMWTI6yYY2KEcHAHl4qmHzrf8BTjSOkVNoQ9gFfwErtPjo5TylEXlPnaks4UEVd07\n\tAonCQ7+cU27qpNVoIpQoYGJMOd6k6QdAffS5NzkjEDHGI4ji/SWO2IwayM5gldfOkXnb\n\tuQZT/dlWS274aA0rMDD3ZXEm3VlF9p0HDR5httt1HnKmjJX6ph1Kdgiwvg57Es/jvdXd\n\tUafKQ1pswQH1u1rjXxjGRIqw3DuXaUc2gNtIiGt+IkE7JuuIIfInrjTAXQEsslqwzKhI\n\tzRHA==","X-Gm-Message-State":"AOAM5318TBXVB7uDlUSdUQEm2zUX46j8LZUrBdrY8lqpyUR8wsI39Q8R\n\tdkXdUG6Sxn+MMqFazKvcqs0ANfuU79s2bWx4TnNhFddmZ2A=","X-Google-Smtp-Source":"ABdhPJwnVOk25Q3mKTmYtNHKUQkH6kAogLTSIvwp6yOKYBs2BhcynQt3lnXWaBAeB1N+SuedCbYF/UYnegT28kmUGnA=","X-Received":"by 2002:a17:906:3adb:: with SMTP id\n\tz27mr38428113ejd.291.1632280902659; \n\tTue, 21 Sep 2021 20:21:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210901080302.68525-1-hiroh@chromium.org>\n\t<20210901080302.68525-3-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 22 Sep 2021 12:21:32 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] android: camera_device:\n\tConfigure one 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\" "}}]