[{"id":15337,"web_url":"https://patchwork.libcamera.org/comment/15337/","msgid":"","date":"2021-02-28T18:48:32","subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:\n> Platforms that do not provide a memory backend implementation should\n> keep using the maxJpegBufferSize() value to calculate the location where\n> to place the JPEG blob id, as the android_generic backend returns the\n> allocated buffer size as calculated using lseek which is larger than\n> the maximum JPEG frame size, which is where the framework expects the\n> JPEG blob to be placed.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index d6eeb962e81d..e7f66d66698c 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t}\n> \n> \t/* Fill in the JPEG blob header. */\n> -\tuint8_t *resultPtr = destination->plane(0) +\n> -\t\t\t destination->planeSize(0) -\n> -\t\t\t sizeof(struct camera3_jpeg_blob);\n> +\t/*\n> +\t * \\todo For backward compatibility reasons with the android_generic\n> +\t * memory backend, continue using the maxJpegBufferSize in case the\n> +\t * computed buffer size is larger. This can be dropped once all\n> +\t * supported platforms will have a working memory backend that\n> +\t * returns the correct buffer size.\n> +\t */\n> +\tsize_t blobSize = std::min(cameraDevice_->maxJpegBufferSize(),\n> +\t\t\t\t\t\t destination->planeSize(0));\n\nCan't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In\nthat case it would expect the JPEG blob to be at the end of the buffer,\nnot after maxJpegBufferSize() bytes.\n\nI'm tempted to just drop this patch. If we want to keep it for other\nAndroid platforms, we'll need an additional function in CameraBuffer to\nreport if the implementation has retrieved the size correctly. It's a\nbit of a hack, but this is a hack anyway :-)\n\nUp to you whether you prefer dropping the patch or keeping it with a new\nCameraBuffer function.\n\n> +\tuint8_t *resultPtr = destination->plane(0) + blobSize\n> +\t\t\t - sizeof(struct camera3_jpeg_blob);\n> \tauto *blob = reinterpret_cast(resultPtr);\n> \tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> \tblob->jpeg_size = jpeg_size;","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 50815BD808\n\tfor ;\n\tSun, 28 Feb 2021 18:49:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C798D68A7E;\n\tSun, 28 Feb 2021 19:49:02 +0100 (CET)","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 1B0E368A45\n\tfor ;\n\tSun, 28 Feb 2021 19:49:01 +0100 (CET)","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 8183E80F;\n\tSun, 28 Feb 2021 19:49:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bJ0QvacI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614538140;\n\tbh=A4wmKs+XzC+o1X1n/9eRaKqRe5a2T1B6QGda5My+3t8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bJ0QvacIVAEpC9+EmI4y4f8v0q7OEPw5kMVuy/0b0BHyEtV+WVla6WXpuwf9CJLtm\n\txAXEFfS6685JGeICTdGRGHuV6HSJUfzOUiegXogglg8UfQyVdkkO74pVdpPIGf6d9I\n\t+Q1cv9B9nu4nXu+o4wihVRRVl51cJ2y9rhyTZPJc=","Date":"Sun, 28 Feb 2021 20:48:32 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210226132932.165484-10-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","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":15343,"web_url":"https://patchwork.libcamera.org/comment/15343/","msgid":"<20210301074110.fy7ossm5mmkidocz@uno.localdomain>","date":"2021-03-01T07:41:10","subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Feb 28, 2021 at 08:48:32PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:\n> > Platforms that do not provide a memory backend implementation should\n> > keep using the maxJpegBufferSize() value to calculate the location where\n> > to place the JPEG blob id, as the android_generic backend returns the\n> > allocated buffer size as calculated using lseek which is larger than\n> > the maximum JPEG frame size, which is where the framework expects the\n> > JPEG blob to be placed.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---\n> > 1 file changed, 11 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index d6eeb962e81d..e7f66d66698c 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > \t}\n> >\n> > \t/* Fill in the JPEG blob header. */\n> > -\tuint8_t *resultPtr = destination->plane(0) +\n> > -\t\t\t destination->planeSize(0) -\n> > -\t\t\t sizeof(struct camera3_jpeg_blob);\n> > +\t/*\n> > +\t * \\todo For backward compatibility reasons with the android_generic\n> > +\t * memory backend, continue using the maxJpegBufferSize in case the\n> > +\t * computed buffer size is larger. This can be dropped once all\n> > +\t * supported platforms will have a working memory backend that\n> > +\t * returns the correct buffer size.\n> > +\t */\n> > +\tsize_t blobSize = std::min(cameraDevice_->maxJpegBufferSize(),\n> > +\t\t\t\t\t\t destination->planeSize(0));\n>\n> Can't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In\n\nI don't think the real buffer size, as calculated through the correct\nmemory backend could be larger than the maximum reported size. The\npurpose of the jpeg.MaxSize is to report the maximum allocation the\ncamera framework has to perform to support the largest JPEG size,\ndon't it ?\n\nThis (seems) to only happen when the backend mis-calculate the buffer\nsize and returns a value larger than jpeg.MaxSize\n\n> that case it would expect the JPEG blob to be at the end of the buffer,\n> not after maxJpegBufferSize() bytes.\n>\n> I'm tempted to just drop this patch. If we want to keep it for other\n> Android platforms, we'll need an additional function in CameraBuffer to\n> report if the implementation has retrieved the size correctly. It's a\n> bit of a hack, but this is a hack anyway :-)\n>\n> Up to you whether you prefer dropping the patch or keeping it with a new\n> CameraBuffer function.\n>\n> > +\tuint8_t *resultPtr = destination->plane(0) + blobSize\n> > +\t\t\t - sizeof(struct camera3_jpeg_blob);\n> > \tauto *blob = reinterpret_cast(resultPtr);\n> > \tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> > \tblob->jpeg_size = jpeg_size;\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 019D5BD808\n\tfor ;\n\tMon, 1 Mar 2021 07:40:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91D67689DD;\n\tMon, 1 Mar 2021 08:40:44 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 150E2602EC\n\tfor ;\n\tMon, 1 Mar 2021 08:40:43 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 7867240011;\n\tMon, 1 Mar 2021 07:40:42 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Mon, 1 Mar 2021 08:41:10 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210301074110.fy7ossm5mmkidocz@uno.localdomain>","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-10-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","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":15351,"web_url":"https://patchwork.libcamera.org/comment/15351/","msgid":"","date":"2021-03-01T09:28:22","subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 01, 2021 at 08:41:10AM +0100, Jacopo Mondi wrote:\n> On Sun, Feb 28, 2021 at 08:48:32PM +0200, Laurent Pinchart wrote:\n> > On Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:\n> > > Platforms that do not provide a memory backend implementation should\n> > > keep using the maxJpegBufferSize() value to calculate the location where\n> > > to place the JPEG blob id, as the android_generic backend returns the\n> > > allocated buffer size as calculated using lseek which is larger than\n> > > the maximum JPEG frame size, which is where the framework expects the\n> > > JPEG blob to be placed.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---\n> > > 1 file changed, 11 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > index d6eeb962e81d..e7f66d66698c 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > \t}\n> > >\n> > > \t/* Fill in the JPEG blob header. */\n> > > -\tuint8_t *resultPtr = destination->plane(0) +\n> > > -\t\t\t destination->planeSize(0) -\n> > > -\t\t\t sizeof(struct camera3_jpeg_blob);\n> > > +\t/*\n> > > +\t * \\todo For backward compatibility reasons with the android_generic\n> > > +\t * memory backend, continue using the maxJpegBufferSize in case the\n> > > +\t * computed buffer size is larger. This can be dropped once all\n> > > +\t * supported platforms will have a working memory backend that\n> > > +\t * returns the correct buffer size.\n> > > +\t */\n> > > +\tsize_t blobSize = std::min(cameraDevice_->maxJpegBufferSize(),\n> > > +\t\t\t\t\t\t destination->planeSize(0));\n> >\n> > Can't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In\n> \n> I don't think the real buffer size, as calculated through the correct\n> memory backend could be larger than the maximum reported size. The\n> purpose of the jpeg.MaxSize is to report the maximum allocation the\n> camera framework has to perform to support the largest JPEG size,\n> don't it ?\n\nYes, but there's nothing that would preclude the camera service from\nallocating more, especially if the maximum JPEG size we report isn't\npage-aligned. The page alignment could be included in or could be left\nout from the buffer size as seen from the camera service, that's an\nimplementation decision we can't control. That's why I'd prefer not\ndepending on this assumption here.\n\n> This (seems) to only happen when the backend mis-calculate the buffer\n> size and returns a value larger than jpeg.MaxSize\n> \n> > that case it would expect the JPEG blob to be at the end of the buffer,\n> > not after maxJpegBufferSize() bytes.\n> >\n> > I'm tempted to just drop this patch. If we want to keep it for other\n> > Android platforms, we'll need an additional function in CameraBuffer to\n> > report if the implementation has retrieved the size correctly. It's a\n> > bit of a hack, but this is a hack anyway :-)\n> >\n> > Up to you whether you prefer dropping the patch or keeping it with a new\n> > CameraBuffer function.\n> >\n> > > +\tuint8_t *resultPtr = destination->plane(0) + blobSize\n> > > +\t\t\t - sizeof(struct camera3_jpeg_blob);\n> > > \tauto *blob = reinterpret_cast(resultPtr);\n> > > \tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> > > \tblob->jpeg_size = jpeg_size;","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 21DACBD1F1\n\tfor ;\n\tMon, 1 Mar 2021 09:28:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0A7468A7D;\n\tMon, 1 Mar 2021 10:28:51 +0100 (CET)","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 ECC6B60521\n\tfor ;\n\tMon, 1 Mar 2021 10:28:49 +0100 (CET)","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 5D78B332;\n\tMon, 1 Mar 2021 10:28:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IUv015Lj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614590929;\n\tbh=B3Vjtu7jz82UB9Dg6Wa42CPHd36pPp/xoN23rUkfia4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IUv015LjmekEgs6t7S/DcwoaJUi596bb6RX7+KZVMkFANKVDvMpAT6afXInyFYuWv\n\tV/yPD1JfVT1zvi3iIy72qoyAkY5MTZYcxihIOrahjwvAXuP6vCEhfYqiAC0O4pYB9z\n\tgm5fmBXKpOKwtICF9sYjs+MVyeUqtpvhHlEqCCrQ=","Date":"Mon, 1 Mar 2021 11:28:22 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-10-jacopo@jmondi.org>\n\t\n\t<20210301074110.fy7ossm5mmkidocz@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301074110.fy7ossm5mmkidocz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","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":15355,"web_url":"https://patchwork.libcamera.org/comment/15355/","msgid":"<20210301094704.xbxdn7ojxdhms74z@uno.localdomain>","date":"2021-03-01T09:47:04","subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Mar 01, 2021 at 11:28:22AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Mar 01, 2021 at 08:41:10AM +0100, Jacopo Mondi wrote:\n> > On Sun, Feb 28, 2021 at 08:48:32PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:\n> > > > Platforms that do not provide a memory backend implementation should\n> > > > keep using the maxJpegBufferSize() value to calculate the location where\n> > > > to place the JPEG blob id, as the android_generic backend returns the\n> > > > allocated buffer size as calculated using lseek which is larger than\n> > > > the maximum JPEG frame size, which is where the framework expects the\n> > > > JPEG blob to be placed.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---\n> > > > 1 file changed, 11 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > index d6eeb962e81d..e7f66d66698c 100644\n> > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > \t}\n> > > >\n> > > > \t/* Fill in the JPEG blob header. */\n> > > > -\tuint8_t *resultPtr = destination->plane(0) +\n> > > > -\t\t\t destination->planeSize(0) -\n> > > > -\t\t\t sizeof(struct camera3_jpeg_blob);\n> > > > +\t/*\n> > > > +\t * \\todo For backward compatibility reasons with the android_generic\n> > > > +\t * memory backend, continue using the maxJpegBufferSize in case the\n> > > > +\t * computed buffer size is larger. This can be dropped once all\n> > > > +\t * supported platforms will have a working memory backend that\n> > > > +\t * returns the correct buffer size.\n> > > > +\t */\n> > > > +\tsize_t blobSize = std::min(cameraDevice_->maxJpegBufferSize(),\n> > > > +\t\t\t\t\t\t destination->planeSize(0));\n> > >\n> > > Can't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In\n> >\n> > I don't think the real buffer size, as calculated through the correct\n> > memory backend could be larger than the maximum reported size. The\n> > purpose of the jpeg.MaxSize is to report the maximum allocation the\n> > camera framework has to perform to support the largest JPEG size,\n> > don't it ?\n>\n> Yes, but there's nothing that would preclude the camera service from\n> allocating more, especially if the maximum JPEG size we report isn't\n> page-aligned. The page alignment could be included in or could be left\n\nAnd that's what happen at the moment with the generic backend. The\nsize computed through it is larger, but android wants the JPEG_BLOB_ID\nat the end of maxJpegBufferSize().\n\nWhen using the correct backend I always get a size smaller than the\nmax jpeg one.\n\n> out from the buffer size as seen from the camera service, that's an\n> implementation decision we can't control. That's why I'd prefer not\n> depending on this assumption here.\n>\n\nHow it works exactly and where the camera service expects the blob id\nto be in case the buffer size gets past the max size, I cannot tell.\n\nWhat's the alternative solution ? Provide a method in the CameraBuffer\nthat says \"yes, I'm reliable\" and if not fall-back on the\nmaxJpegBufferSize ? That's two bad hacks competing for the one being\nless nasty, I don't see a nice way to get away with this tbh.\n\n\n> > This (seems) to only happen when the backend mis-calculate the buffer\n> > size and returns a value larger than jpeg.MaxSize\n> >\n> > > that case it would expect the JPEG blob to be at the end of the buffer,\n> > > not after maxJpegBufferSize() bytes.\n> > >\n> > > I'm tempted to just drop this patch. If we want to keep it for other\n> > > Android platforms, we'll need an additional function in CameraBuffer to\n> > > report if the implementation has retrieved the size correctly. It's a\n> > > bit of a hack, but this is a hack anyway :-)\n> > >\n> > > Up to you whether you prefer dropping the patch or keeping it with a new\n> > > CameraBuffer function.\n> > >\n> > > > +\tuint8_t *resultPtr = destination->plane(0) + blobSize\n> > > > +\t\t\t - sizeof(struct camera3_jpeg_blob);\n> > > > \tauto *blob = reinterpret_cast(resultPtr);\n> > > > \tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> > > > \tblob->jpeg_size = jpeg_size;\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 186A0BD1F1\n\tfor ;\n\tMon, 1 Mar 2021 09:46:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE8D768A7E;\n\tMon, 1 Mar 2021 10:46:37 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0165660521\n\tfor ;\n\tMon, 1 Mar 2021 10:46:37 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 74683E0004;\n\tMon, 1 Mar 2021 09:46:36 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Mon, 1 Mar 2021 10:47:04 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210301094704.xbxdn7ojxdhms74z@uno.localdomain>","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-10-jacopo@jmondi.org>\n\t\n\t<20210301074110.fy7ossm5mmkidocz@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","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":15386,"web_url":"https://patchwork.libcamera.org/comment/15386/","msgid":"","date":"2021-03-02T01:53:30","subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 01, 2021 at 10:47:04AM +0100, Jacopo Mondi wrote:\n> On Mon, Mar 01, 2021 at 11:28:22AM +0200, Laurent Pinchart wrote:\n> > On Mon, Mar 01, 2021 at 08:41:10AM +0100, Jacopo Mondi wrote:\n> > > On Sun, Feb 28, 2021 at 08:48:32PM +0200, Laurent Pinchart wrote:\n> > > > On Fri, Feb 26, 2021 at 02:29:29PM +0100, Jacopo Mondi wrote:\n> > > > > Platforms that do not provide a memory backend implementation should\n> > > > > keep using the maxJpegBufferSize() value to calculate the location where\n> > > > > to place the JPEG blob id, as the android_generic backend returns the\n> > > > > allocated buffer size as calculated using lseek which is larger than\n> > > > > the maximum JPEG frame size, which is where the framework expects the\n> > > > > JPEG blob to be placed.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/jpeg/post_processor_jpeg.cpp | 14 +++++++++++---\n> > > > > 1 file changed, 11 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > index d6eeb962e81d..e7f66d66698c 100644\n> > > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > @@ -185,9 +185,17 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > > \t}\n> > > > >\n> > > > > \t/* Fill in the JPEG blob header. */\n> > > > > -\tuint8_t *resultPtr = destination->plane(0) +\n> > > > > -\t\t\t destination->planeSize(0) -\n> > > > > -\t\t\t sizeof(struct camera3_jpeg_blob);\n> > > > > +\t/*\n> > > > > +\t * \\todo For backward compatibility reasons with the android_generic\n> > > > > +\t * memory backend, continue using the maxJpegBufferSize in case the\n> > > > > +\t * computed buffer size is larger. This can be dropped once all\n> > > > > +\t * supported platforms will have a working memory backend that\n> > > > > +\t * returns the correct buffer size.\n> > > > > +\t */\n> > > > > +\tsize_t blobSize = std::min(cameraDevice_->maxJpegBufferSize(),\n> > > > > +\t\t\t\t\t\t destination->planeSize(0));\n\nBy the way this should be bufferSize, not blobSize.\n\n> > > >\n> > > > Can't Chrome OS allocate buffers larger than maxJpegBufferSize() ? In\n> > >\n> > > I don't think the real buffer size, as calculated through the correct\n> > > memory backend could be larger than the maximum reported size. The\n> > > purpose of the jpeg.MaxSize is to report the maximum allocation the\n> > > camera framework has to perform to support the largest JPEG size,\n> > > don't it ?\n> >\n> > Yes, but there's nothing that would preclude the camera service from\n> > allocating more, especially if the maximum JPEG size we report isn't\n> > page-aligned. The page alignment could be included in or could be left\n> \n> And that's what happen at the moment with the generic backend. The\n> size computed through it is larger, but android wants the JPEG_BLOB_ID\n> at the end of maxJpegBufferSize().\n\nAndroid wants the blob at the end of the buffer, and that could be\ndifferent (either smaller, as we've seen, or larger) than\nmaxJpegBufferSize() as nothing in the API guarantees that the buffer\nsize will always be smaller than or equal to maxJpegBufferSize(). I'd\nrather not make any assumption here, we've been bitten by incorrect\nassumptions before :-S\n\n> When using the correct backend I always get a size smaller than the\n> max jpeg one.\n\nThat's something I'd like to understand too (if the framework gives us a\nbuffer smaller than the maximum JPEG size we report, there's a risk the\nJPEG image wouldn't fit - it's theoretical, we report a random estimate,\nbut the principle still holds) but it's a separate issue.\n\n> > out from the buffer size as seen from the camera service, that's an\n> > implementation decision we can't control. That's why I'd prefer not\n> > depending on this assumption here.\n> \n> How it works exactly and where the camera service expects the blob id\n> to be in case the buffer size gets past the max size, I cannot tell.\n> \n> What's the alternative solution ? Provide a method in the CameraBuffer\n> that says \"yes, I'm reliable\" and if not fall-back on the\n> maxJpegBufferSize ? That's two bad hacks competing for the one being\n> less nasty, I don't see a nice way to get away with this tbh.\n\nThat's the part that bothers me too. All this is meant to be temporary,\nso I suppose we could live with this, but I'm worried the temporary code\nwill stay for some time, and result in a breakage on platforms that have\na real backend when they'll give us a buffer larger than\nmaxJpegBufferSize(), for any reason.\n\nOne option that may be a tiny bit cleaner would be to introduce a new\njpegBlobOffset() function in CameraBuffer, to return the offset of the\nblob. At least the name would be better than \"isReliable()\" :-)\n\n> > > This (seems) to only happen when the backend mis-calculate the buffer\n> > > size and returns a value larger than jpeg.MaxSize\n> > >\n> > > > that case it would expect the JPEG blob to be at the end of the buffer,\n> > > > not after maxJpegBufferSize() bytes.\n> > > >\n> > > > I'm tempted to just drop this patch. If we want to keep it for other\n> > > > Android platforms, we'll need an additional function in CameraBuffer to\n> > > > report if the implementation has retrieved the size correctly. It's a\n> > > > bit of a hack, but this is a hack anyway :-)\n> > > >\n> > > > Up to you whether you prefer dropping the patch or keeping it with a new\n> > > > CameraBuffer function.\n> > > >\n> > > > > +\tuint8_t *resultPtr = destination->plane(0) + blobSize\n> > > > > +\t\t\t - sizeof(struct camera3_jpeg_blob);\n> > > > > \tauto *blob = reinterpret_cast(resultPtr);\n> > > > > \tblob->jpeg_blob_id = CAMERA3_JPEG_BLOB_ID;\n> > > > > \tblob->jpeg_size = jpeg_size;","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 CA1B7BD1F1\n\tfor ;\n\tTue, 2 Mar 2021 01:54:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4911160522;\n\tTue, 2 Mar 2021 02:54:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41200602E9\n\tfor ;\n\tTue, 2 Mar 2021 02:53:59 +0100 (CET)","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 A464345D;\n\tTue, 2 Mar 2021 02:53:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EKasxPUk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614650038;\n\tbh=XZDE83xSeqrJIwu1LKq1UyHbd0eM0G/4kC+jzfAAhC0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EKasxPUkk5FkwGXKzaRrqosLwe2PVvAkOVNxpmVtdtt7ASbgr7pxky8rxz+bX+gwV\n\tAJs+TguWCBWaHXkiNDvAEdwtTRkhTZOVP5CZSIOKyyui+rwrM0PVQA9qeGHutCihAt\n\t+mkaf8H7S/WgQJ+X0DS+f1VJn4PaaxDAHN66JbuA=","Date":"Tue, 2 Mar 2021 03:53:30 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-10-jacopo@jmondi.org>\n\t\n\t<20210301074110.fy7ossm5mmkidocz@uno.localdomain>\n\t\n\t<20210301094704.xbxdn7ojxdhms74z@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301094704.xbxdn7ojxdhms74z@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 09/12] android: jpeg: Use\n\tmaxJpegBufferSize() for compatibility","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\" "}}]