[{"id":13489,"web_url":"https://patchwork.libcamera.org/comment/13489/","msgid":"<20201026184846.GB3756@pendragon.ideasonboard.com>","date":"2020-10-26T18:48:46","subject":"Re: [libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg:\n\tAllow encoding raw frame bytes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Oct 26, 2020 at 07:31:33PM +0530, Umang Jain wrote:\n> Allow encoding frames which are directly handed over to the encoder\n> via a span or vector i.e. a raw frame bytes. Introduce an overloaded\n> EncoderLibJpeg::encode() with libcamera::Span source parameter to\n> achieve this functionality. This makes the libjpeg-encoder a bit\n> flexible for use case such as compressing a thumbnail generated for\n> Exif.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/encoder_libjpeg.cpp | 18 ++++++++++++------\n> src/android/jpeg/encoder_libjpeg.h | 7 +++++--\n> 2 files changed, 17 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index cfa5332..129ca27 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> \treturn 0;\n> }\n> \n> -void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)\n> +void EncoderLibJpeg::compressRGB(Span frame)\n\nCan you make this a Span to express that the function\ndoesn't modify the contents of the frame ? It will likely require a few\nchanges within the function, making local variables const, and/or using\nconst_cast<> to cast away the const as some libjpeg functions take a\nnon-const pointer to data they never modify (an unfortunate API choice,\nbut that's the way it is).\n\n> {\n> -\tunsigned char *src = static_cast(frame->maps()[0].data());\n> +\tunsigned char *src = frame.data();\n> \t/* \\todo Stride information should come from buffer configuration. */\n> \tunsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> \n> @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)\n> * Compress the incoming buffer from a supported NV format.\n> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> */\n> -void EncoderLibJpeg::compressNV(const MappedBuffer *frame)\n> +void EncoderLibJpeg::compressNV(Span frame)\n\nSame here.\n\n> {\n> \tuint8_t tmprowbuf[compress_.image_width * 3];\n> \n> @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const MappedBuffer *frame)\n> \tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n> \tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n> \n> -\tconst unsigned char *src = static_cast(frame->maps()[0].data());\n> +\tconst unsigned char *src = frame.data();\n> \tconst unsigned char *src_c = src + y_stride * compress_.image_height;\n> \n> \tJSAMPROW row_pointer[1];\n> @@ -189,6 +189,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\treturn frame.error();\n> \t}\n> \n> +\treturn encode(frame.maps()[0], dest, exifData);\n> +}\n> +\n> +int EncoderLibJpeg::encode(Span src, Span dest,\n> +\t\t\t Span exifData)\n\nAnd same here for src (dest obviously needs to be mutable :-)).\n\nReviewed-by: Laurent Pinchart \n\n> +{\n> \tunsigned char *destination = dest.data();\n> \tunsigned long size = dest.size();\n> \n> @@ -214,9 +220,9 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\t\t << \"x\" << compress_.image_height;\n> \n> \tif (nv_)\n> -\t\tcompressNV(&frame);\n> +\t\tcompressNV(src);\n> \telse\n> -\t\tcompressRGB(&frame);\n> +\t\tcompressRGB(src);\n> \n> \tjpeg_finish_compress(&compress_);\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 40505dd..06f884b 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -24,10 +24,13 @@ public:\n> \tint encode(const libcamera::FrameBuffer &source,\n> \t\t libcamera::Span destination,\n> \t\t libcamera::Span exifData) override;\n> +\tint encode(libcamera::Span source,\n> +\t\t libcamera::Span destination,\n> +\t\t libcamera::Span exifData);\n> \n> private:\n> -\tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> -\tvoid compressNV(const libcamera::MappedBuffer *frame);\n> +\tvoid compressRGB(libcamera::Span frame);\n> +\tvoid compressNV(libcamera::Span frame);\n> \n> \tstruct jpeg_compress_struct compress_;\n> \tstruct jpeg_error_mgr jerr_;","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 3A214BDB1E\n\tfor ;\n\tMon, 26 Oct 2020 18:49:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 988A962057;\n\tMon, 26 Oct 2020 19:49:35 +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 437B962054\n\tfor ;\n\tMon, 26 Oct 2020 19:49:34 +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 A2E57527;\n\tMon, 26 Oct 2020 19:49:33 +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=\"hjR+BXcM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603738173;\n\tbh=+nFzP9ZBLxyyhXXJ+FDEUHPyb2lvxCRhgcjsjWC6/Xo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hjR+BXcMpi1g3CfnEntb66liZt5tvzPZ/XsDYc1twma992B9ZpgtfbaxR2lgp/nal\n\t59gQWk3Ha6Z3pqVXVGV53SHPnNAaaOIzpnNhLrVTm/eEiQn8I3CJhDbwm6PrQTZBRm\n\tABPiwdIVon/rBtI0nueM/D/aDr1ftN86LUhX/V9U=","Date":"Mon, 26 Oct 2020 20:48:46 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201026184846.GB3756@pendragon.ideasonboard.com>","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-3-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201026140134.44166-3-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg:\n\tAllow encoding raw frame bytes","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":13504,"web_url":"https://patchwork.libcamera.org/comment/13504/","msgid":"","date":"2020-10-27T04:34:37","subject":"Re: [libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg:\n\tAllow encoding raw frame bytes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Oct 27, 2020 at 3:49 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 26, 2020 at 07:31:33PM +0530, Umang Jain wrote:\n> > Allow encoding frames which are directly handed over to the encoder\n> > via a span or vector i.e. a raw frame bytes. Introduce an overloaded\n> > EncoderLibJpeg::encode() with libcamera::Span source parameter to\n> > achieve this functionality. This makes the libjpeg-encoder a bit\n> > flexible for use case such as compressing a thumbnail generated for\n> > Exif.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/jpeg/encoder_libjpeg.cpp | 18 ++++++++++++------\n> > src/android/jpeg/encoder_libjpeg.h | 7 +++++--\n> > 2 files changed, 17 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index cfa5332..129ca27 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -104,9 +104,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > return 0;\n> > }\n> >\n> > -void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)\n> > +void EncoderLibJpeg::compressRGB(Span frame)\n>\n> Can you make this a Span to express that the function\n> doesn't modify the contents of the frame ? It will likely require a few\n> changes within the function, making local variables const, and/or using\n> const_cast<> to cast away the const as some libjpeg functions take a\n> non-const pointer to data they never modify (an unfortunate API choice,\n> but that's the way it is).\n>\n> > {\n> > - unsigned char *src = static_cast(frame->maps()[0].data());\n> > + unsigned char *src = frame.data();\n> > /* \\todo Stride information should come from buffer configuration. */\n> > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> >\n> > @@ -122,7 +122,7 @@ void EncoderLibJpeg::compressRGB(const MappedBuffer *frame)\n> > * Compress the incoming buffer from a supported NV format.\n> > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> > */\n> > -void EncoderLibJpeg::compressNV(const MappedBuffer *frame)\n> > +void EncoderLibJpeg::compressNV(Span frame)\n>\n> Same here.\n>\n> > {\n> > uint8_t tmprowbuf[compress_.image_width * 3];\n> >\n> > @@ -144,7 +144,7 @@ void EncoderLibJpeg::compressNV(const MappedBuffer *frame)\n> > unsigned int cb_pos = nvSwap_ ? 1 : 0;\n> > unsigned int cr_pos = nvSwap_ ? 0 : 1;\n> >\n> > - const unsigned char *src = static_cast(frame->maps()[0].data());\n> > + const unsigned char *src = frame.data();\n> > const unsigned char *src_c = src + y_stride * compress_.image_height;\n> >\n> > JSAMPROW row_pointer[1];\n> > @@ -189,6 +189,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> > return frame.error();\n> > }\n> >\n> > + return encode(frame.maps()[0], dest, exifData);\n> > +}\n> > +\n> > +int EncoderLibJpeg::encode(Span src, Span dest,\n> > + Span exifData)\n>\n> And same here for src (dest obviously needs to be mutable :-)).\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +{\n> > unsigned char *destination = dest.data();\n> > unsigned long size = dest.size();\n> >\n> > @@ -214,9 +220,9 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> > << \"x\" << compress_.image_height;\n> >\n> > if (nv_)\n> > - compressNV(&frame);\n> > + compressNV(src);\n> > else\n> > - compressRGB(&frame);\n> > + compressRGB(src);\n> >\n> > jpeg_finish_compress(&compress_);\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 40505dd..06f884b 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -24,10 +24,13 @@ public:\n> > int encode(const libcamera::FrameBuffer &source,\n> > libcamera::Span destination,\n> > libcamera::Span exifData) override;\n> > + int encode(libcamera::Span source,\n> > + libcamera::Span destination,\n> > + libcamera::Span exifData);\n> >\n\nThe commit message says this is an overloaded function.\nBut it isn't actually. Because of that, this function needs to be\ninvoked with EncoderLibJpeg in PostProcessorJpeg.\nIf I remember correctly, we'all agree to expanding FrameBuffer for\ndmabuf in the future.\nSo the interface with FrameBuffer should be a solid single overloaded function.\nI think we would rather\n1.) makes the new function a specific function (which is done by this\npatch actually),\n2.) Use the FrameBuffer interface always.\nWe may want to think of the tradeoff between introducing the specific\nfunction and how simple the code becomes thanks to it.\n\nBest Regards,\n-Hiro\n\n\n\n> > private:\n> > - void compressRGB(const libcamera::MappedBuffer *frame);\n> > - void compressNV(const libcamera::MappedBuffer *frame);\n> > + void compressRGB(libcamera::Span frame);\n> > + void compressNV(libcamera::Span frame);\n> >\n> > struct jpeg_compress_struct compress_;\n> > struct jpeg_error_mgr jerr_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 EA56ABDB1E\n\tfor ;\n\tTue, 27 Oct 2020 04:34:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BDE66206C;\n\tTue, 27 Oct 2020 05:34:50 +0100 (CET)","from mail-ej1-x641.google.com (mail-ej1-x641.google.com\n\t[IPv6:2a00:1450:4864:20::641])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 875766034C\n\tfor ;\n\tTue, 27 Oct 2020 05:34:49 +0100 (CET)","by mail-ej1-x641.google.com with SMTP id t25so251166ejd.13\n\tfor ;\n\tMon, 26 Oct 2020 21:34:49 -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=\"aX2QYQjq\"; 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=EW6E9GRSkmR7+pof7UOf+JflAH0WcrkYWGog9iANTQA=;\n\tb=aX2QYQjq45A/P6vwZwazjBzCwPovxouLRc3Rx4+4gkIFTKxE0GsTfKvaEtkcYvqeAJ\n\tsvkPbac1JoklGU1tTtabn7N+2vGXS/PGcJ0Inw8GtkN7qEExZNlb1Ei4UNOAGu90z9Xm\n\t1G76NCQr04c/YbaY1RR7dqDt/T1BTBlSlH3jM=","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=EW6E9GRSkmR7+pof7UOf+JflAH0WcrkYWGog9iANTQA=;\n\tb=gF8NZuNvn7HcfK6oDJXvvzS6DIJ4U5XU5ecTXcxx2XdwKcGEdIqlrB2SC9QeC0qmmX\n\tMMYR5S09maUu+irtZx+1F9MPxD1tcmtmZBkeEhjpCLNCNi0nihtEXJpctcgy49ZBSysZ\n\tlZ/YTda63O3vd99AmLKrk8P6/qH3jLStQPIFWaQyWxZMW0DIxiTDtINqPm7wObvyMN0O\n\tAxO98+om1CZ/rOZCAbXtvrldTmg3t/VgsOae4At850bloi7DT+FWrbO7SkpQseewgrO8\n\t6wDEZVuzsNy7iY4whdADA1GqsAwIiT4t//qso5dN/F88LX+bNRYM/zf6OApA4Te5x0cZ\n\tAZbA==","X-Gm-Message-State":"AOAM531q5Uorf7VPhpJvRZgwuZbFkXQJP7NczUWCOWaafbJjNSEPIr/G\n\tUc9C1/xiAsLU0NUVDzr9riynid6CfT9A2QEH9SvwJx1oHgc=","X-Google-Smtp-Source":"ABdhPJxtfhAMB1Jdd/6HzXh4AYdNiX/nUB/eky5eU/x0MY1g6Ts++UtGY+3pCg8xZRpCKBsMgZEVYpz6F8L0NrnChwA=","X-Received":"by 2002:a17:907:2177:: with SMTP id\n\trl23mr544598ejb.243.1603773289090; \n\tMon, 26 Oct 2020 21:34:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026140134.44166-1-email@uajain.com>\n\t<20201026140134.44166-3-email@uajain.com>\n\t<20201026184846.GB3756@pendragon.ideasonboard.com>","In-Reply-To":"<20201026184846.GB3756@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 27 Oct 2020 13:34:37 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 2/3] android: jpeg: encoder_libjpeg:\n\tAllow encoding raw frame bytes","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\" "}}]