[{"id":18890,"web_url":"https://patchwork.libcamera.org/comment/18890/","msgid":"","date":"2021-08-18T01:59:16","subject":"Re: [libcamera-devel] [RFC PATCH 00/10] Add offset to\n\tFrameBuffer::Plane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Aug 16, 2021 at 01:31:28PM +0900, Hirokazu Honda wrote:\n> Since FrameBuffer::Plane doesn't have an offset variable, it is\n> impossible to map the FrameBuffer::Plane correctly. This series fixes\n> the issue by adding an offset and alignes some code of mapping and\n> creating FrameBuffer::Plane with the new offset.\n> \n> I probably miss some code of using FrameBuffer::Plane. To detect the\n> bug, I add the assertion to FrameBuffer.\n\nNice patch series ! Thanks a lot.\n\nOverall, I think this is good. I've made comments to individual patches,\nand none change the direction.\n\nThere's one part that makes me feel a bit uneasy though. With this\nseries applied, we're moving to better support of multi-planar formats.\nWe don't support all possible options, for instance, we don't support\nthe V4L2_PIX_FMT_NV12M format, but that's fine, there's no need to\nimplement everything in one go. However, it would be nice if you could\nsummarize somewhere, maybe in the cover letter, what is expected to be\nsupported (e.g. allocating FrameBuffer from a V4L2VideDevice for both\nthe single-planar and multi-planar V4L2 APIs, for the single V4L2 plane\nformats (i.e. not NV12M) only, or queuing to V4L2VideoDevice a\nmulti-planar FrameBuffer with all planes referring to the same dmabuf)\nand what isn't supported (e.g. queuing to a V4L2VideDevice a\nmulti-planar FrameBuffer with different dmabufs, or with non-default\noffsets). It would help reviewing the next version, to ensure the\nimplementation matches the expectations.\n\n> Hirokazu Honda (10):\n> libcamera: framebuffer: Add offset to FrameBuffer::Plane\n> libcamera: ipa_data_serializer: Modify (de)serialization for offset\n> libcamera: mapped_framebuffer: Return plane begin address by\n> MappedBuffer::maps()\n> cam: file_sink: Use offset in mapping FrameBuffer\n> ipa: rkisp1: Use offset in mapping IPABuffer\n> qcam: main_window: Use offset mapping FrameBuffer\n> gstreamer: gstlibcameraallocator: Use offset in creating a buffer\n> libcamera: v4l2_videodevice: Create color-format planes in\n> CreateBuffer()\n> android: camera_device: Fill offset and right length in\n> CreateFrameBuffer()\n> libcamera: framebuffer: Add assertion to detect offset is unfilled\n> \n> include/libcamera/framebuffer.h | 12 ++++-\n> .../libcamera/internal/mapped_framebuffer.h | 4 +-\n> include/libcamera/internal/v4l2_videodevice.h | 4 +-\n> src/android/camera_device.cpp | 38 +++++++-------\n> src/cam/file_sink.cpp | 5 +-\n> src/cam/file_sink.h | 1 +\n> src/gstreamer/gstlibcameraallocator.cpp | 3 +-\n> src/ipa/rkisp1/rkisp1.cpp | 4 +-\n> src/libcamera/framebuffer.cpp | 11 ++--\n> src/libcamera/ipa_data_serializer.cpp | 5 +-\n> src/libcamera/mapped_framebuffer.cpp | 51 +++++++++++++++----\n> src/libcamera/v4l2_videodevice.cpp | 28 +++++++++-\n> src/qcam/main_window.cpp | 15 ++++--\n> src/qcam/main_window.h | 1 +\n> 14 files changed, 136 insertions(+), 46 deletions(-)","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 0353ABD87D\n\tfor ;\n\tWed, 18 Aug 2021 01:59:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E43568894;\n\tWed, 18 Aug 2021 03:59:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01CBB6025D\n\tfor ;\n\tWed, 18 Aug 2021 03:59:23 +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 71471466;\n\tWed, 18 Aug 2021 03:59:23 +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=\"FWRn2XRU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629251963;\n\tbh=CjO9tI9UIkNCyXKXV9oDLAJ9cvZquLe3G6VCZMureo8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FWRn2XRUblpmfaKPnUp9RqAA06fk8QXiwWMmG96Q+oYoLnxIvp8N/6b86A85cUq33\n\tynD5cttUsjnJKJgYwwmltJhqvAbenI1heEujMwymGwRdJh7q+CSpTjQG9XmjCCNtJf\n\t6loqTjMLOkQAXraPchyaJS7Fy1HMcenk8uXfUF1s=","Date":"Wed, 18 Aug 2021 04:59:16 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210816043138.957984-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 00/10] Add offset to\n\tFrameBuffer::Plane","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]