[{"id":15076,"web_url":"https://patchwork.libcamera.org/comment/15076/","msgid":"","date":"2021-02-09T01:50:53","subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","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 Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:\n> ---\n> src/android/camera_device.cpp | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index e3d43bea4700..5a8072a8a007 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tminFrameDurationUsec = frameDurationsInfo->second.min().get();\n> \t\tmaxFrameDurationUsec = frameDurationsInfo->second.max().get();\n> \n> +\t\t/*\n> +\t\t * Adjust the minimum frame duration to comply with Android\n> +\t\t * requirements. The camera service mandates all preview/record\n> +\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> +\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> +\t\t * implementation).\n> +\t\t *\n> +\t\t * If we're close enough (+- 500 useconds) to that value round\n> +\t\t * the minimum frame duration of the camera to an accepted\n> +\t\t * value.\n> +\t\t */\n> +\t\tstatic constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;\n> +\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;\n\nYou could write\n\n\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;\n\nas MIN_PREVIEW_RECORD_FPS is only used here.\n\n> +\t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {\n> +\t\t\tdouble frameDurationDelta = minFrameDurationUsec -\n> +\t\t\t\t\t\t MAX_PREVIEW_RECORD_DURATION_US;\n> +\t\t\tif (frameDurationDelta < 500)\n> +\t\t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n> +\t\t}\n\nIs the following more readable ?\n\n\t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&\n\t\t minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {\n\t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n\nThis is of course a horrible hack, but if we have no choice...\n\nReviewed-by: Laurent Pinchart \n\nWhat's the maximum FPS we otherwise report ? Is it really that the\nsensor can't reach 30fps, or is there an issue somewhere else ?\n\n> \t\t/*\n> \t\t * The AE routine frame rate limits are computed using the frame\n> \t\t * duration limits, as libcamera clips the AE routine to the","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 B17E8BD160\n\tfor ;\n\tTue, 9 Feb 2021 01:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4015361623;\n\tTue, 9 Feb 2021 02:51:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F882613F3\n\tfor ;\n\tTue, 9 Feb 2021 02:51:18 +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 00779583;\n\tTue, 9 Feb 2021 02:51:17 +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=\"LX+l6/ew\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612835478;\n\tbh=FlNfzp+P2V3WOz7XKe4GV5IbxXmdYE8zfIKaSFsz4mA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LX+l6/ewFXAiYrViC+WWOIYf4er1R++tGF5JIzY1Obj1Ws1ojzEUDEf01iio6F2/b\n\t1YCMHHuwTsrrg4aNBN4cyNdplirUUbNOaibq7qjredO1LLSjMRsrJg+D3De/3lk0Aa\n\tlFlCgoUrKLlLEdeYX2sl9587YfQcbicAlO9LmZ4I=","Date":"Tue, 9 Feb 2021 03:50:53 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126173008.446321-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","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":15542,"web_url":"https://patchwork.libcamera.org/comment/15542/","msgid":"","date":"2021-03-08T23:47:01","subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Feb 09, 2021 at 03:50:55AM +0200, Laurent Pinchart wrote:\n> On Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:\n> > ---\n> > src/android/camera_device.cpp | 20 ++++++++++++++++++++\n> > 1 file changed, 20 insertions(+)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index e3d43bea4700..5a8072a8a007 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \t\tminFrameDurationUsec = frameDurationsInfo->second.min().get();\n> > \t\tmaxFrameDurationUsec = frameDurationsInfo->second.max().get();\n> > \n> > +\t\t/*\n> > +\t\t * Adjust the minimum frame duration to comply with Android\n> > +\t\t * requirements. The camera service mandates all preview/record\n> > +\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> > +\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> > +\t\t * implementation).\n> > +\t\t *\n> > +\t\t * If we're close enough (+- 500 useconds) to that value round\n> > +\t\t * the minimum frame duration of the camera to an accepted\n> > +\t\t * value.\n> > +\t\t */\n> > +\t\tstatic constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;\n> > +\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;\n> \n> You could write\n> \n> \t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;\n> \n> as MIN_PREVIEW_RECORD_FPS is only used here.\n> \n> > +\t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {\n> > +\t\t\tdouble frameDurationDelta = minFrameDurationUsec -\n> > +\t\t\t\t\t\t MAX_PREVIEW_RECORD_DURATION_US;\n> > +\t\t\tif (frameDurationDelta < 500)\n> > +\t\t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n> > +\t\t}\n> \n> Is the following more readable ?\n> \n> \t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&\n> \t\t minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {\n> \t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n> \n> This is of course a horrible hack, but if we have no choice...\n> \n> Reviewed-by: Laurent Pinchart \n> \n> What's the maximum FPS we otherwise report ? Is it really that the\n> sensor can't reach 30fps, or is there an issue somewhere else ?\n\nDo you have any data regarding this question, why can't we achieve 30fps\nwithout cheating ? I'm also curious, how does the Intel IPU3 HAL handle\nthis issue ?\n\n> > \t\t/*\n> > \t\t * The AE routine frame rate limits are computed using the frame\n> > \t\t * duration limits, as libcamera clips the AE routine to the","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 94BB5BD1F1\n\tfor ;\n\tMon, 8 Mar 2021 23:47:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5D6568A9F;\n\tTue, 9 Mar 2021 00:47:34 +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 74B146051F\n\tfor ;\n\tTue, 9 Mar 2021 00:47:33 +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 D1D9199;\n\tTue, 9 Mar 2021 00:47:32 +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=\"nhcSSa3r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615247253;\n\tbh=pGBhNT8nbJ7O+uOWiHD4xQ68PRpqvsZRgmyrOP0/MO4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nhcSSa3rWtY8Vhs9do9C4pnWKWK1cNczK0T2xkH/lHOc9G0ksyWfbA73Xa98xUne9\n\t9LTkeXxTA0pEFLJ59HlGwPBzsYQAlvk2Efzq9EKcC+JtnHUZ5OojfcEKjcEGD2H8dO\n\tUZ02vrtUeEVicb0iC/RnUIzcGchRAYlOvn8Z3Vt4=","Date":"Tue, 9 Mar 2021 01:47:01 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","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":15554,"web_url":"https://patchwork.libcamera.org/comment/15554/","msgid":"<20210309073054.uj7flhmtl6k6iflg@uno.localdomain>","date":"2021-03-09T07:30:54","subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Mar 09, 2021 at 01:47:01AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Feb 09, 2021 at 03:50:55AM +0200, Laurent Pinchart wrote:\n> > On Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:\n> > > ---\n> > > src/android/camera_device.cpp | 20 ++++++++++++++++++++\n> > > 1 file changed, 20 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index e3d43bea4700..5a8072a8a007 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \t\tminFrameDurationUsec = frameDurationsInfo->second.min().get();\n> > > \t\tmaxFrameDurationUsec = frameDurationsInfo->second.max().get();\n> > >\n> > > +\t\t/*\n> > > +\t\t * Adjust the minimum frame duration to comply with Android\n> > > +\t\t * requirements. The camera service mandates all preview/record\n> > > +\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> > > +\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> > > +\t\t * implementation).\n> > > +\t\t *\n> > > +\t\t * If we're close enough (+- 500 useconds) to that value round\n> > > +\t\t * the minimum frame duration of the camera to an accepted\n> > > +\t\t * value.\n> > > +\t\t */\n> > > +\t\tstatic constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;\n> > > +\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;\n> >\n> > You could write\n> >\n> > \t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;\n> >\n> > as MIN_PREVIEW_RECORD_FPS is only used here.\n> >\n> > > +\t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {\n> > > +\t\t\tdouble frameDurationDelta = minFrameDurationUsec -\n> > > +\t\t\t\t\t\t MAX_PREVIEW_RECORD_DURATION_US;\n> > > +\t\t\tif (frameDurationDelta < 500)\n> > > +\t\t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n> > > +\t\t}\n> >\n> > Is the following more readable ?\n> >\n> > \t\tif (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&\n> > \t\t minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {\n> > \t\t\tminFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;\n> >\n> > This is of course a horrible hack, but if we have no choice...\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > What's the maximum FPS we otherwise report ? Is it really that the\n> > sensor can't reach 30fps, or is there an issue somewhere else ?\n>\n> Do you have any data regarding this question, why can't we achieve 30fps\n\nI thought I reported it already, sorry about that. The sensor can\nachieve 29.94 FPS while android wants 29.97\n\n> without cheating ? I'm also curious, how does the Intel IPU3 HAL handle\n> this issue ?\n>\n\nAfaict from looking at the static metadata they report a fram duration\nof 33.33 milliseconds unconditionally (30.00.. FPS)\n\n\n> > > \t\t/*\n> > > \t\t * The AE routine frame rate limits are computed using the frame\n> > > \t\t * duration limits, as libcamera clips the AE routine to the\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 5E844BD1F1\n\tfor ;\n\tTue, 9 Mar 2021 07:30:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28E6F68A9C;\n\tTue, 9 Mar 2021 08:30:26 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A1C3602E4\n\tfor ;\n\tTue, 9 Mar 2021 08:30:25 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id B204D240009;\n\tTue, 9 Mar 2021 07:30:24 +0000 (UTC)"],"X-Originating-IP":"79.22.58.175","Date":"Tue, 9 Mar 2021 08:30:54 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210309073054.uj7flhmtl6k6iflg@uno.localdomain>","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-5-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] fixup! android: camera_device:\n\tCompute frame durations","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\" "}}]