[{"id":12314,"web_url":"https://patchwork.libcamera.org/comment/12314/","msgid":"<20200905174901.GE6319@pendragon.ideasonboard.com>","date":"2020-09-05T17:49:01","subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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 Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:\n> When producing the list of image resolution to claim as supported by the\n\ns/resolution/resolutions/\n\n> camera HAL, the JPEG stream was assumed to be 'always valid' as, at the\n> time, there was no JPEG support in place at all.\n> \n> With the introduction of support for JPEG compression, reporting\n> non-valid sizes as supported obviously causes troubles.\n> \n> In order to avoid reporting non-supported resolutions as supported,\n> produce the list of available JPEG sizes by using the ones supported\n> by the YCbCr_420_888 format, from which the JPEG stream is encoded.\n\nDown the line we'll likely need to scale down images for JPEG\ncompression, but for now this looks fine to me.\n\n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------\n> 1 file changed, 28 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ad0d7fd15d90..8a8072123961 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()\n> \t\tconst std::vector &libcameraFormats =\n> \t\t\tcamera3Format.libcameraFormats;\n> \n> +\t\t/*\n> +\t\t * Fixed format mapping for JPEG.\n> +\t\t *\n> +\t\t * The list of supported JPEG resolutions is generated\n> +\t\t * from the list of resolutions supported by\n> +\t\t * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.\n\nThis doesn seem to correspond to the code below. Should it read\n\n\t\t/*\n\t\t * JPEG is always supported, either produced directly by the\n\t\t * camera, or encoded in the HAL.\n\t\t */\n\n?\n\n> +\t\t *\n> +\t\t * \\todo Wire the JPEG encoder interface to query the list\n> +\t\t * of supported resolutions.\n\nIs this still valid ?\n\n> +\t\t */\n> +\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> +\t\t\tformatsMap_[androidFormat] = formats::MJPEG;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\t/*\n> \t\t * Test the libcamera formats that can produce images\n> \t\t * compatible with the format defined by Android.\n> \t\t */\n> \t\tPixelFormat mappedFormat;\n> \t\tfor (const PixelFormat &pixelFormat : libcameraFormats) {\n> -\t\t\t/* \\todo Fixed mapping for JPEG. */\n> -\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> -\t\t\t\tmappedFormat = formats::MJPEG;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> \n> \t\t\t/*\n> \t\t\t * The stream configuration size can be adjusted,\n> @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()\n> \t\t\tcfg.size = res;\n> \n> \t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> -\t\t\t/*\n> -\t\t\t * Unconditionally report we can produce JPEG.\n> -\t\t\t *\n> -\t\t\t * \\todo The JPEG stream will be implemented as an\n> -\t\t\t * HAL-only stream, but some cameras can produce it\n> -\t\t\t * directly. As of now, claim support for JPEG without\n> -\t\t\t * inspecting where the JPEG stream is produced.\n> -\t\t\t */\n> -\t\t\tif (androidFormat != HAL_PIXEL_FORMAT_BLOB &&\n> -\t\t\t status != CameraConfiguration::Valid)\n> +\t\t\tif (status != CameraConfiguration::Valid)\n> \t\t\t\tcontinue;\n> \n> \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> +\n> +\t\t\t/*\n> +\t\t\t * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888\n> +\t\t\t * from which JPEG is produced, add an entry for\n> +\t\t\t * the JPEG stream.\n> +\t\t\t *\n> +\t\t\t * \\todo Wire the JPEG encoder to query the supported\n> +\t\t\t * sizes provided a list of formats it can encode.\n\nWe shouldn't do this if the device can provide JPEG natively. Is this\nsomething you'd like to address already ? Otherwise we should add a\n\\todo item for this too.\n\n> +\t\t\t */\n> +\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)\n> +\t\t\t\tstreamConfigurations_.push_back(\n> +\t\t\t\t\t{ res, HAL_PIXEL_FORMAT_BLOB });\n> \t\t}\n> \t}\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 307E0BDB1D\n\tfor ;\n\tSat, 5 Sep 2020 17:49:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B763C62B5B;\n\tSat, 5 Sep 2020 19:49:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1ECD62901\n\tfor ;\n\tSat, 5 Sep 2020 19:49:28 +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 230A5335;\n\tSat, 5 Sep 2020 19:49:25 +0200 (CEST)"],"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=\"tOH1q1Ag\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599328165;\n\tbh=5rh24PlZG3KztJ3w43HdpRJBZKMQIOVgyEtU3jcebns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tOH1q1AgEZm/cb1piWGctFfeFX2s0C3DD/zjsE2mXiyvXdDG2ZhveU22rSEHG0G40\n\turo27nof5BEDT/hGMgN0WvvqvG4o2qXcW4oCYXjNX6KT1p+lBV5yJbEj+klTAVlIle\n\ttFfr3miOPFcpLHacDGyzxxtVjL1NkDpswayekaR8=","Date":"Sat, 5 Sep 2020 20:49:01 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200905174901.GE6319@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200902152236.69770-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, hiroh@google.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12342,"web_url":"https://patchwork.libcamera.org/comment/12342/","msgid":"","date":"2020-09-07T07:58:28","subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi,\n\nOn Sun, Sep 6, 2020 at 2:49 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:\n> > When producing the list of image resolution to claim as supported by the\n>\n> s/resolution/resolutions/\n>\n> > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the\n> > time, there was no JPEG support in place at all.\n> >\n> > With the introduction of support for JPEG compression, reporting\n> > non-valid sizes as supported obviously causes troubles.\n> >\n> > In order to avoid reporting non-supported resolutions as supported,\n> > produce the list of available JPEG sizes by using the ones supported\n> > by the YCbCr_420_888 format, from which the JPEG stream is encoded.\n>\n> Down the line we'll likely need to scale down images for JPEG\n> compression, but for now this looks fine to me.\n>\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------\n> > 1 file changed, 28 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ad0d7fd15d90..8a8072123961 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()\n> > const std::vector &libcameraFormats =\n> > camera3Format.libcameraFormats;\n> >\n> > + /*\n> > + * Fixed format mapping for JPEG.\n> > + *\n> > + * The list of supported JPEG resolutions is generated\n> > + * from the list of resolutions supported by\n> > + * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.\n>\n> This doesn seem to correspond to the code below. Should it read\n>\n> /*\n> * JPEG is always supported, either produced directly by the\n> * camera, or encoded in the HAL.\n> */\n>\n> ?\n>\n> > + *\n> > + * \\todo Wire the JPEG encoder interface to query the list\n> > + * of supported resolutions.\n>\n> Is this still valid ?\n>\n> > + */\n> > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > + formatsMap_[androidFormat] = formats::MJPEG;\n> > + continue;\n> > + }\n> > +\n> > /*\n> > * Test the libcamera formats that can produce images\n> > * compatible with the format defined by Android.\n> > */\n> > PixelFormat mappedFormat;\n> > for (const PixelFormat &pixelFormat : libcameraFormats) {\n> > - /* \\todo Fixed mapping for JPEG. */\n> > - if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > - mappedFormat = formats::MJPEG;\n> > - break;\n> > - }\n> >\n> > /*\n> > * The stream configuration size can be adjusted,\n> > @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()\n> > cfg.size = res;\n> >\n> > CameraConfiguration::Status status = cameraConfig->validate();\n> > - /*\n> > - * Unconditionally report we can produce JPEG.\n> > - *\n> > - * \\todo The JPEG stream will be implemented as an\n> > - * HAL-only stream, but some cameras can produce it\n> > - * directly. As of now, claim support for JPEG without\n> > - * inspecting where the JPEG stream is produced.\n> > - */\n> > - if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&\n> > - status != CameraConfiguration::Valid)\n> > + if (status != CameraConfiguration::Valid)\n> > continue;\n> >\n> > streamConfigurations_.push_back({ res, androidFormat });\n\nThis comment is probably the same as Laurent's one.\nThe change looks to allow using native jpeg output streams.\n\n> > +\n> > + /*\n> > + * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888\n> > + * from which JPEG is produced, add an entry for\n> > + * the JPEG stream.\n> > + *\n> > + * \\todo Wire the JPEG encoder to query the supported\n> > + * sizes provided a list of formats it can encode.\n>\n> We shouldn't do this if the device can provide JPEG natively. Is this\n> something you'd like to address already ? Otherwise we should add a\n> \\todo item for this too.\n>\n> > + */\n> > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)\n> > + streamConfigurations_.push_back(\n> > + { res, HAL_PIXEL_FORMAT_BLOB });\n> > }\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nBest Regards,\n-Hiro\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD212BDB1D\n\tfor ;\n\tMon, 7 Sep 2020 07:58:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A07162B98;\n\tMon, 7 Sep 2020 09:58:42 +0200 (CEST)","from mail-ed1-x543.google.com (mail-ed1-x543.google.com\n\t[IPv6:2a00:1450:4864:20::543])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC7E4629B6\n\tfor ;\n\tMon, 7 Sep 2020 09:58:40 +0200 (CEST)","by mail-ed1-x543.google.com with SMTP id l63so11803553edl.9\n\tfor ;\n\tMon, 07 Sep 2020 00:58:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ea8msjsA\"; 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=xr4pcJ65dGuWPoOvU7XBVDWGNH+pXJE+OlRwgWphkPM=;\n\tb=ea8msjsArxQq0M+vJCAsTY/XoCkbq+3h6J+aImOD0BhKizg/PU0Eik6c7QTVTRnpmS\n\tUPtQvhuOP/6iEvQQYraDDg4tcEteTZ7GmRawwcCvSqwCc6hB4yvpIOlMt2dEi7J8anaB\n\tq9Kg9rIEtJq2xE6vsvWzAG035TqbRNH4jdUK0=","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=xr4pcJ65dGuWPoOvU7XBVDWGNH+pXJE+OlRwgWphkPM=;\n\tb=aKfYpC7X1h12hrpxH3mkCRVcFSebBNiqzVcdmGzZ+kNGFtLykRCqRPI6JdEgZd6NGJ\n\t0HTQg/Wh65UTBVg0KIeAKvITFkw8b6dshOWHbMXJU6UrRH73NB1zShMdTcWXJiRLpTdP\n\tdXGSBHIuX0XUzDglfWRU/LuTjferGwoTMSixD7NolzdkEXJA8OX79k7mZI4GTKYr/+qF\n\tc3vaL7uQ7dbLtdso4Q7JONNkB4vn7y+CfH3P4+ZuMp79nMDPwc95tscv60CA2Jz6Xtbz\n\tvucZrbJZxDv5Bg+YR5wBMCeQNQtkbtAhd1lq7bs+zyLcZ1/tl3JxgSsZMBncrVd6av0q\n\tES1Q==","X-Gm-Message-State":"AOAM531MxM2lvtvughCXAUwy80FQfoURTc3LDDIxjCYHlzexYi84Ze4X\n\tj/RgXY4MQun/ICkKHBG7ZGjUcUXFRPoyh8ubuW3Gmg==","X-Google-Smtp-Source":"ABdhPJxcEVTwqCJxuUZCL9q9Pms/T8xzBZXkaDl6kKIo5ES0opXjuF6oA37krI9CX7HEeKVPP4fPxx4Mnf5rhwt0lqo=","X-Received":"by 2002:aa7:d296:: with SMTP id\n\tw22mr20507388edq.327.1599465520455; \n\tMon, 07 Sep 2020 00:58:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-3-jacopo@jmondi.org>\n\t<20200905174901.GE6319@pendragon.ideasonboard.com>","In-Reply-To":"<20200905174901.GE6319@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 7 Sep 2020 16:58:28 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12344,"web_url":"https://patchwork.libcamera.org/comment/12344/","msgid":"<20200907082952.rncrleok7tkx2s4i@uno.localdomain>","date":"2020-09-07T08:29:52","subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Sep 05, 2020 at 08:49:01PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:\n> > When producing the list of image resolution to claim as supported by the\n>\n> s/resolution/resolutions/\n>\n> > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the\n> > time, there was no JPEG support in place at all.\n> >\n> > With the introduction of support for JPEG compression, reporting\n> > non-valid sizes as supported obviously causes troubles.\n> >\n> > In order to avoid reporting non-supported resolutions as supported,\n> > produce the list of available JPEG sizes by using the ones supported\n> > by the YCbCr_420_888 format, from which the JPEG stream is encoded.\n>\n> Down the line we'll likely need to scale down images for JPEG\n> compression, but for now this looks fine to me.\n>\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------\n> > 1 file changed, 28 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ad0d7fd15d90..8a8072123961 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()\n> > \t\tconst std::vector &libcameraFormats =\n> > \t\t\tcamera3Format.libcameraFormats;\n> >\n> > +\t\t/*\n> > +\t\t * Fixed format mapping for JPEG.\n> > +\t\t *\n> > +\t\t * The list of supported JPEG resolutions is generated\n> > +\t\t * from the list of resolutions supported by\n> > +\t\t * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.\n>\n> This doesn seem to correspond to the code below. Should it read\n\ndoesn't it ? JPEG is generated from YCbCr... but this comment is\nactually a repetition of another one here below, so I guess I can\nreplace it.\n\n>\n> \t\t/*\n> \t\t * JPEG is always supported, either produced directly by the\n> \t\t * camera, or encoded in the HAL.\n> \t\t */\n>\n> ?\n>\n> > +\t\t *\n> > +\t\t * \\todo Wire the JPEG encoder interface to query the list\n> > +\t\t * of supported resolutions.\n>\n> Is this still valid ?\n>\n\nI think so. As explained to Kieran in v1 I think all the\nimplementation of the Encoder interface should be able to return the\nlist of supported resolution to take into account things like, say,\nalignment.\n\n> > +\t\t */\n> > +\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > +\t\t\tformatsMap_[androidFormat] = formats::MJPEG;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > \t\t/*\n> > \t\t * Test the libcamera formats that can produce images\n> > \t\t * compatible with the format defined by Android.\n> > \t\t */\n> > \t\tPixelFormat mappedFormat;\n> > \t\tfor (const PixelFormat &pixelFormat : libcameraFormats) {\n> > -\t\t\t/* \\todo Fixed mapping for JPEG. */\n> > -\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > -\t\t\t\tmappedFormat = formats::MJPEG;\n> > -\t\t\t\tbreak;\n> > -\t\t\t}\n> >\n> > \t\t\t/*\n> > \t\t\t * The stream configuration size can be adjusted,\n> > @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()\n> > \t\t\tcfg.size = res;\n> >\n> > \t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > -\t\t\t/*\n> > -\t\t\t * Unconditionally report we can produce JPEG.\n> > -\t\t\t *\n> > -\t\t\t * \\todo The JPEG stream will be implemented as an\n> > -\t\t\t * HAL-only stream, but some cameras can produce it\n> > -\t\t\t * directly. As of now, claim support for JPEG without\n> > -\t\t\t * inspecting where the JPEG stream is produced.\n> > -\t\t\t */\n> > -\t\t\tif (androidFormat != HAL_PIXEL_FORMAT_BLOB &&\n> > -\t\t\t status != CameraConfiguration::Valid)\n> > +\t\t\tif (status != CameraConfiguration::Valid)\n> > \t\t\t\tcontinue;\n> >\n> > \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888\n> > +\t\t\t * from which JPEG is produced, add an entry for\n> > +\t\t\t * the JPEG stream.\n> > +\t\t\t *\n> > +\t\t\t * \\todo Wire the JPEG encoder to query the supported\n> > +\t\t\t * sizes provided a list of formats it can encode.\n>\n> We shouldn't do this if the device can provide JPEG natively. Is this\n> something you'd like to address already ? Otherwise we should add a\n> \\todo item for this too.\n\nThis isn't different from what we had before. I we want to support\nJPEG natively produced from the camera, there's some work to do to\nidentify that at the beginning of this function.\n\nA todo item is indeed appropriate.\n\nThanks\n j\n\n>\n> > +\t\t\t */\n> > +\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)\n> > +\t\t\t\tstreamConfigurations_.push_back(\n> > +\t\t\t\t\t{ res, HAL_PIXEL_FORMAT_BLOB });\n> > \t\t}\n> > \t}\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 4DBF8BDB1D\n\tfor ;\n\tMon, 7 Sep 2020 08:26:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E99E862B82;\n\tMon, 7 Sep 2020 10:26:05 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0B13629B6\n\tfor ;\n\tMon, 7 Sep 2020 10:26:05 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id AE7D9C0010;\n\tMon, 7 Sep 2020 08:26:04 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Sep 2020 10:29:52 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200907082952.rncrleok7tkx2s4i@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-3-jacopo@jmondi.org>\n\t<20200905174901.GE6319@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200905174901.GE6319@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, hiroh@google.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12355,"web_url":"https://patchwork.libcamera.org/comment/12355/","msgid":"<20200907131554.GB6047@pendragon.ideasonboard.com>","date":"2020-09-07T13:15:54","subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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, Sep 07, 2020 at 10:29:52AM +0200, Jacopo Mondi wrote:\n> On Sat, Sep 05, 2020 at 08:49:01PM +0300, Laurent Pinchart wrote:\n> > On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:\n> > > When producing the list of image resolution to claim as supported by the\n> >\n> > s/resolution/resolutions/\n> >\n> > > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the\n> > > time, there was no JPEG support in place at all.\n> > >\n> > > With the introduction of support for JPEG compression, reporting\n> > > non-valid sizes as supported obviously causes troubles.\n> > >\n> > > In order to avoid reporting non-supported resolutions as supported,\n> > > produce the list of available JPEG sizes by using the ones supported\n> > > by the YCbCr_420_888 format, from which the JPEG stream is encoded.\n> >\n> > Down the line we'll likely need to scale down images for JPEG\n> > compression, but for now this looks fine to me.\n> >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------\n> > > 1 file changed, 28 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ad0d7fd15d90..8a8072123961 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()\n> > > \t\tconst std::vector &libcameraFormats =\n> > > \t\t\tcamera3Format.libcameraFormats;\n> > >\n> > > +\t\t/*\n> > > +\t\t * Fixed format mapping for JPEG.\n> > > +\t\t *\n> > > +\t\t * The list of supported JPEG resolutions is generated\n> > > +\t\t * from the list of resolutions supported by\n> > > +\t\t * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.\n> >\n> > This doesn seem to correspond to the code below. Should it read\n> \n> doesn't it ? JPEG is generated from YCbCr... but this comment is\n> actually a repetition of another one here below, so I guess I can\n> replace it.\n\nI'm not saying the comment is a lie :-) Just that it doesn't correspond\nto the 4 lines directly after it.\n\n> >\n> > \t\t/*\n> > \t\t * JPEG is always supported, either produced directly by the\n> > \t\t * camera, or encoded in the HAL.\n> > \t\t */\n> >\n> > ?\n> >\n> > > +\t\t *\n> > > +\t\t * \\todo Wire the JPEG encoder interface to query the list\n> > > +\t\t * of supported resolutions.\n> >\n> > Is this still valid ?\n> \n> I think so. As explained to Kieran in v1 I think all the\n> implementation of the Encoder interface should be able to return the\n> list of supported resolution to take into account things like, say,\n> alignment.\n> \n> > > +\t\t */\n> > > +\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > > +\t\t\tformatsMap_[androidFormat] = formats::MJPEG;\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > \t\t/*\n> > > \t\t * Test the libcamera formats that can produce images\n> > > \t\t * compatible with the format defined by Android.\n> > > \t\t */\n> > > \t\tPixelFormat mappedFormat;\n> > > \t\tfor (const PixelFormat &pixelFormat : libcameraFormats) {\n> > > -\t\t\t/* \\todo Fixed mapping for JPEG. */\n> > > -\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_BLOB) {\n> > > -\t\t\t\tmappedFormat = formats::MJPEG;\n> > > -\t\t\t\tbreak;\n> > > -\t\t\t}\n> > >\n> > > \t\t\t/*\n> > > \t\t\t * The stream configuration size can be adjusted,\n> > > @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()\n> > > \t\t\tcfg.size = res;\n> > >\n> > > \t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > > -\t\t\t/*\n> > > -\t\t\t * Unconditionally report we can produce JPEG.\n> > > -\t\t\t *\n> > > -\t\t\t * \\todo The JPEG stream will be implemented as an\n> > > -\t\t\t * HAL-only stream, but some cameras can produce it\n> > > -\t\t\t * directly. As of now, claim support for JPEG without\n> > > -\t\t\t * inspecting where the JPEG stream is produced.\n> > > -\t\t\t */\n> > > -\t\t\tif (androidFormat != HAL_PIXEL_FORMAT_BLOB &&\n> > > -\t\t\t status != CameraConfiguration::Valid)\n> > > +\t\t\tif (status != CameraConfiguration::Valid)\n> > > \t\t\t\tcontinue;\n> > >\n> > > \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888\n> > > +\t\t\t * from which JPEG is produced, add an entry for\n> > > +\t\t\t * the JPEG stream.\n> > > +\t\t\t *\n> > > +\t\t\t * \\todo Wire the JPEG encoder to query the supported\n> > > +\t\t\t * sizes provided a list of formats it can encode.\n> >\n> > We shouldn't do this if the device can provide JPEG natively. Is this\n> > something you'd like to address already ? Otherwise we should add a\n> > \\todo item for this too.\n> \n> This isn't different from what we had before. I we want to support\n> JPEG natively produced from the camera, there's some work to do to\n> identify that at the beginning of this function.\n\nI'm not saying it's a regression :-) A todo is all that we need here I\nthink.\n\n> A todo item is indeed appropriate.\n> \n> > > +\t\t\t */\n> > > +\t\t\tif (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)\n> > > +\t\t\t\tstreamConfigurations_.push_back(\n> > > +\t\t\t\t\t{ res, HAL_PIXEL_FORMAT_BLOB });\n> > > \t\t}\n> > > \t}\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 B648FBDB1C\n\tfor ;\n\tMon, 7 Sep 2020 13:16:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D03960371;\n\tMon, 7 Sep 2020 15:16:22 +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 600AA6036E\n\tfor ;\n\tMon, 7 Sep 2020 15:16:20 +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 BDDC33E;\n\tMon, 7 Sep 2020 15:16:19 +0200 (CEST)"],"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=\"f5i8L/Cz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599484579;\n\tbh=NyGUvHwL5YE2cZZbZ16nnm0YC6w2D8FqmPQ7gOGDUnc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f5i8L/CzE+Ey0gXeZg/fL61xt1Tf2P0qtxHqNJmcD5c2EfCWO28dVNCLsvQCqv7ix\n\tquST4atisl1Hz8/DoAVpuqaIArxofXCJEW/0QCndh3PCpR3fVeBvfUAGqH4Ub4GWSm\n\ts9JFWQY7mqIPmoSEmUwBQrhCZ5L1zQT5GupD11e4=","Date":"Mon, 7 Sep 2020 16:15:54 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200907131554.GB6047@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-3-jacopo@jmondi.org>\n\t<20200905174901.GE6319@pendragon.ideasonboard.com>\n\t<20200907082952.rncrleok7tkx2s4i@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200907082952.rncrleok7tkx2s4i@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/12] android: camera_device:\n\tGenerate JPEG sizes","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, hiroh@google.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]