[{"id":11711,"web_url":"https://patchwork.libcamera.org/comment/11711/","msgid":"<20200729140809.GB6183@pendragon.ideasonboard.com>","date":"2020-07-29T14:08:09","subject":"Re: [libcamera-devel] [PATCH 0/5] Transform implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Jul 29, 2020 at 10:31:49AM +0100, David Plowman wrote:\n> Hi everyone\n> \n> This patch set isn't complete, however, I thought it was worth giving\n> people the chance to have a look, and this also allows me to ask a few\n> more questions!\n> \n> There are 5 patches.\n> \n> 1. The first one merely adds the Transform class and inserts a\n> Transform field into the CameraConfiguration. This is the only\n> non-Raspberry Pi code.\n> \n> 2. Handles the transform in the RPi pipeline handler so that the\n> correct flips are applied to the camera. Also passes the transform to\n> the IPA.\n> \n> 3. Plumbs the transform through to all the separate control algorithms\n> via the SwitchMode method (but does nothing further to handle the\n> transform).\n> \n> 4. Handles the transform correctly in ALSC.\n> \n> 5. As part of patch 4, I noticed it wasn't dealing correctly with the\n> luminance tables, irrespective of the transform. This fixes that and\n> actually improves the handling of camera mode switches in general.\n> \n> Comments/Questions:\n> \n> * In patch #1, transform.h is labelled as \"Copyright Google\". Or\n> should I put Raspberry Pi? Does it matter?\n\nYou're the author of the code, why should the copyright be assigned to\nGoogle ? :-) I expect copyright Raspberry Pi Trading.\n\n> * Patch #1 needs documentation. It looks like I need to write a\n> transform.cpp file in the manner of (for example) geometry.cpp. Is\n> that right, or is there anything else? (Do we mind if this file has\n> nothing but comments?) How would I test the generated documentation\n> is correct?\n\nThat's correct. We have other .cpp files that only have comments, so\nit's fine, but there are a couple of functions that are a bit larger and\nwould make sense to move to the .cpp file. I'll comment on that\nspecifically when reviewing 1/5.\n\n> * Patch #2 has an ugly cast...\n\nI'll comment on that separately too :-)\n\n> I think that's everything for now. Suggestions welcome as ever...!\n> \n> David Plowman (5):\n> libcamera: Add Transform class\n> libcamera: raspberrypi: Apply transform and pass through to IPA\n> libcamera: raspberrypi: Plumb user transform through to individual\n> IPAs\n> libcamera: raspberrypi: ALSC: Handle transform in colour tables\n> libcamera: raspberrypi: ALSC: Fix crop/transform of luminance table\n> \n> include/libcamera/camera.h | 3 +\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> include/libcamera/meson.build | 1 +\n> include/libcamera/transform.h | 193 ++++++++++++++++++\n> src/ipa/raspberrypi/controller/algorithm.cpp | 4 +-\n> src/ipa/raspberrypi/controller/algorithm.hpp | 3 +-\n> src/ipa/raspberrypi/controller/controller.cpp | 5 +-\n> src/ipa/raspberrypi/controller/controller.hpp | 5 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 3 +-\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 +-\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 108 +++++++---\n> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 9 +-\n> src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 +-\n> src/ipa/raspberrypi/controller/rpi/noise.hpp | 3 +-\n> .../raspberrypi/controller/rpi/sharpen.cpp | 3 +-\n> .../raspberrypi/controller/rpi/sharpen.hpp | 3 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 44 ++--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 27 ++-\n> 18 files changed, 356 insertions(+), 66 deletions(-)\n> create mode 100644 include/libcamera/transform.h","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 43092BD86F\n\tfor ;\n\tWed, 29 Jul 2020 14:08:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAF3C613C6;\n\tWed, 29 Jul 2020 16:08:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20A1E6039D\n\tfor ;\n\tWed, 29 Jul 2020 16:08:19 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 852C131F;\n\tWed, 29 Jul 2020 16:08:18 +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=\"AtLKAuAs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596031698;\n\tbh=+MdjkkMf9xIbZhTH0Shw8q4KtAeChro7e57dH/ersFA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AtLKAuAs1nqcOkFmWROLIIfwhiceHv1LCfET5AnfdWUEAtm5oMkX2lppAbz2UZ5hP\n\tiCl2/dy9bFxLhpv9uItlc6qPZ03IVVfcuRDLsn0ry51Rgh6GwQRbQka1diNTfykvcb\n\tx3D1oAjzzuk3lBwofxZNg519aTNrLT/BD++fSj0U=","Date":"Wed, 29 Jul 2020 17:08:09 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200729140809.GB6183@pendragon.ideasonboard.com>","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729093154.12296-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 0/5] Transform implementation","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\" "}}]