[{"id":26252,"web_url":"https://patchwork.libcamera.org/comment/26252/","msgid":"","date":"2023-01-17T10:27:21","subject":"Re: [libcamera-devel] [PATCH v9 6/8] android: jpeg: Return an error\n\tcode from generateThumbnail()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thank you for the patch!\nOnly nits of the format.\n\nOn Mon, Jan 16, 2023 at 8:28 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> The usual way to signal errors in libcamera is to return an error code.\n> Change the Encoder::generateThumbnail() function to return an int\n> instead of signaling errors by not resizing the thumbnail vector. This\n> allows propagating error codes to the callers.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/jpeg/encoder.h | 8 +--\n> src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++-----------\n> src/android/jpeg/encoder_libjpeg.h | 8 +--\n> src/android/jpeg/post_processor_jpeg.cpp | 6 +-\n> 4 files changed, 48 insertions(+), 44 deletions(-)\n>\n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index 5f9ef890023a..d15f22e67830 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -22,8 +22,8 @@ public:\n> libcamera::Span destination,\n> libcamera::Span exifData,\n> unsigned int quality) = 0;\n> - virtual void generateThumbnail(const libcamera::FrameBuffer\n> &source,\n> - const libcamera::Size &targetSize,\n> - unsigned int quality,\n> - std::vector\n> *thumbnail) = 0;\n> + virtual int generateThumbnail(const libcamera::FrameBuffer &source,\n> + const libcamera::Size &targetSize,\n> + unsigned int quality,\n> + std::vector\n> *thumbnail) = 0;\n> };\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp\n> b/src/android/jpeg/encoder_libjpeg.cpp\n> index 9bbf1abbe09c..8f4df7899dfd 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -89,53 +89,57 @@ int EncoderLibJpeg::encode(const FrameBuffer &source,\n> Span dest,\n> return captureEncoder_.encode(frame.planes(), dest, exifData,\n> quality);\n> }\n>\n> -void EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer\n> &source,\n> - const libcamera::Size &targetSize,\n> - unsigned int quality,\n> - std::vector\n> *thumbnail)\n> +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer\n> &source,\n> + const libcamera::Size &targetSize,\n> + unsigned int quality,\n> + std::vector\n> *thumbnail)\n> {\n> /* Stores the raw scaled-down thumbnail bytes. */\n> std::vector rawThumbnail;\n>\n> thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> + if (rawThumbnail.empty())\n> + return -EINVAL;\n>\n> StreamConfiguration thCfg;\n> thCfg.size = targetSize;\n> thCfg.pixelFormat = thumbnailer_.pixelFormat();\n> int ret = thumbnailEncoder_.configure(thCfg);\n> + if (ret)\n> + return ret;\n>\n> - if (!rawThumbnail.empty() && !ret) {\n> - /*\n> - * \\todo Avoid value-initialization of all elements of the\n> - * vector.\n\n- */\n> - thumbnail->resize(rawThumbnail.size());\n> + /*\n> + * \\todo Avoid value-initialization of all elements of the\n> + * vector.\n>\n\nnit: I guess |vector| could be merged in the above line, as the line\nis not that long now? (Is the limit 80 characters?)\n\n\n> + */\n> + thumbnail->resize(rawThumbnail.size());\n>\n> - /*\n> - * Split planes manually as the encoder expects a vector of\n> - * planes.\n>\n\nditto\n\n\n> - *\n> - * \\todo Pass a vector of planes directly to\n> - * Thumbnailer::createThumbnailer above and remove the\n> manual\n> - * planes split from here.\n> - */\n> - std::vector> thumbnailPlanes;\n> - const PixelFormatInfo &formatNV12 =\n> - PixelFormatInfo::info(formats::NV12);\n> - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> - thumbnailPlanes.push_back({ rawThumbnail.data(),\n> yPlaneSize });\n> - thumbnailPlanes.push_back({ rawThumbnail.data() +\n> yPlaneSize,\n> - uvPlaneSize });\n> + /*\n> + * Split planes manually as the encoder expects a vector of\n> + * planes.\n> + *\n> + * \\todo Pass a vector of planes directly to\n> + * Thumbnailer::createThumbnailer above and remove the manual\n> + * planes split from here.\n> + */\n> + std::vector> thumbnailPlanes;\n> + const PixelFormatInfo &formatNV12 =\n> + PixelFormatInfo::info(formats::NV12);\n> + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0);\n> + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1);\n> + thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize });\n> + thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize,\n> + uvPlaneSize });\n>\n> - int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes,\n> *thumbnail,\n> - {}, quality);\n> - thumbnail->resize(jpegSize);\n> + int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes,\n> *thumbnail,\n> + {}, quality);\n> + thumbnail->resize(jpegSize);\n>\n> - LOG(JPEG, Debug)\n> - << \"Thumbnail compress returned \"\n> - << jpegSize << \" bytes\";\n> - }\n>\nditto\n\n> + LOG(JPEG, Debug)\n> + << \"Thumbnail compress returned \"\n> + << jpegSize << \" bytes\";\n> +\n> + return 0;\n> }\n>\n> EncoderLibJpeg::Encoder::Encoder()\n> diff --git a/src/android/jpeg/encoder_libjpeg.h\n> b/src/android/jpeg/encoder_libjpeg.h\n> index a022a72e02bf..9cff4f1b42e3 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -28,10 +28,10 @@ public:\n> libcamera::Span destination,\n> libcamera::Span exifData,\n> unsigned int quality) override;\n> - void generateThumbnail(const libcamera::FrameBuffer &source,\n> - const libcamera::Size &targetSize,\n> - unsigned int quality,\n> - std::vector *thumbnail)\n> override;\n> + int generateThumbnail(const libcamera::FrameBuffer &source,\n> + const libcamera::Size &targetSize,\n> + unsigned int quality,\n> + std::vector *thumbnail)\n> override;\n>\n> private:\n> class Encoder\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> b/src/android/jpeg/post_processor_jpeg.cpp\n> index 69b18a2e5945..5df89383e1af 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -115,9 +115,9 @@ void\n> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n>\n> if (thumbnailSize != Size(0, 0)) {\n> std::vector thumbnail;\n> - encoder_->generateThumbnail(source, thumbnailSize,\n> - quality, &thumbnail);\n> - if (!thumbnail.empty())\n> + ret = encoder_->generateThumbnail(source,\n> thumbnailSize,\n> + quality,\n> &thumbnail);\n> + if (!ret)\n> exif.setThumbnail(std::move(thumbnail),\n> Exif::Compression::JPEG);\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\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 471BFC3240\n\tfor ;\n\tTue, 17 Jan 2023 10:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95BA3625D8;\n\tTue, 17 Jan 2023 11:27:35 +0100 (CET)","from mail-vk1-xa30.google.com (mail-vk1-xa30.google.com\n\t[IPv6:2607:f8b0:4864:20::a30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA0A761EFE\n\tfor ;\n\tTue, 17 Jan 2023 11:27:33 +0100 (CET)","by mail-vk1-xa30.google.com with SMTP id q21so6303228vka.3\n\tfor ;\n\tTue, 17 Jan 2023 02:27:33 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673951255;\n\tbh=owQfkg1/LZTsqcrUJZxnQbSVifC6NBJblc36EKxx7P4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=a2LM8fnsGgrZ+9T/m3O+UoaREzsxjumNJChy0ns+O1CB7kSk5308P92njpWJz9/1a\n\tgyaucJ0eNg+2ZKtZnTe+/JOnbxq041tgDy3G8YchE12MF7f2qQnUqgq5FSuIpcyyhb\n\toHHnQp5a+u5kzZEdBWyQQGkKYe30RXBeg9gk9NlswtG+DSjxwD6zlOA0SzEITVT3IY\n\tL1Y75AeB9jO9Ovx66s58FIi0eaAqvDBGqP69Dc2rBbA7FW/uDXIRnyCF4CdrYE0diy\n\tTmU2CPAEBudp8tpzGolCOEoJ2F3UlMtwkfgeRzdalpEaiHy5mJfx8XmEmdfBkE6GzR\n\t5KNHDdP0tW9sQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=ERIOKIkhBCrhxp1wVYUUZu2K103Ei9/aY+RpNqBykvY=;\n\tb=ItsIbj4LZEs+CRJx5gDcSj0qAZs3DfVgpFdSL505bi1orpBhOOFZpraLtYqNtfRF0z\n\tkKBvTGnZufBOtpy2ax2jhnb5qhBQgZfsxUIPQ8biB6VVL9cUqKvUCnsVmR0qaqNOGP5S\n\tZj9W/Ocba+d4jSCIN+CUXzUvju8I+d1pAZMWY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"ItsIbj4L\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=ERIOKIkhBCrhxp1wVYUUZu2K103Ei9/aY+RpNqBykvY=;\n\tb=d8cV5CAWUgJZ5t5V3jNnhDI8WUYRtHCNAkut82TvIpADvc6XItU8r6YkXtLP6PT+7v\n\tFeEFdkY/rhuxpKUc7OyAOFkWIclKUhl+acWocVHd4NI0BTxUZ3/wBHRFVHzQX+6U7wC2\n\tYz0BFac/FvYaeV9zVEze0wafR1MN7x3jQtiKzCFo0yK/4DsskYmLwDOO/FUgfIStFXiZ\n\tMiOTredGRWwZ0+AuhAthXdgnex7X9BVLdz7znAuuH8JFva8WP1hDx8uzab+oaSI6nbxr\n\tsfd5BW9CaBJZvNIYUcCKsFUPDQFFekDLD3BzKU93/rCSHqAEyuw44sjDklioePKf3TIM\n\tyqEg==","X-Gm-Message-State":"AFqh2kpweS89pauD3bF9EO0bpllon2Do5ghimzBnmc6GgNq3mcSuvWhL\n\t06oPkxST0F9Uwo6Z+4HD3CPTLtV5n8IpE3IN/aOrWlCI+MGdWscy","X-Google-Smtp-Source":"AMrXdXv24URypCVodF/8IXk+1J1C3VgDDy6+Ygo1roSAA4qmRRNvj9xbjLrsPhIAbejAs4iK9JS3GRKob1qp/wDKHLc=","X-Received":"by 2002:a1f:a30c:0:b0:3e1:6f40:9ca2 with SMTP id\n\tm12-20020a1fa30c000000b003e16f409ca2mr312865vke.16.1673951252445;\n\tTue, 17 Jan 2023 02:27:32 -0800 (PST)","MIME-Version":"1.0","References":"<20230116002808.16014-1-laurent.pinchart@ideasonboard.com>\n\t<20230116002808.16014-7-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20230116002808.16014-7-laurent.pinchart@ideasonboard.com>","Date":"Tue, 17 Jan 2023 18:27:21 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000c6139505f273256e\"","Subject":"Re: [libcamera-devel] [PATCH v9 6/8] android: jpeg: Return an error\n\tcode from generateThumbnail()","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","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]