[{"id":3415,"web_url":"https://patchwork.libcamera.org/comment/3415/","msgid":"<20200111015222.GR4859@pendragon.ideasonboard.com>","date":"2020-01-11T01:52:22","subject":"Re: [libcamera-devel] [PATCH v3 00/33] libcamera: Rework buffer API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patches.\n\nOn Fri, Jan 10, 2020 at 08:37:35PM +0100, Niklas Söderlund wrote:\n> Hi,\n> \n> This series reworks the buffer API across the whole library. The two\n> main reasons for the rework is\n> \n> - The current buffer API is cumbersome to work with as the variations\n> between internally allocated buffers (from a V4L2 video device) and\n> externally (from other source in the system or other V4l2 video\n> device) is slightly different.\n> \n> - V4L2 concepts such as buffer index have \"leaked\" into the application\n> facing interface which makes the API less intuitive to work with as\n> one needs to know more about V4L2 and its limitations to use.\n> \n> As changing the buffer API touches most parts of the library this series\n> is unfortunately quiet large and some patches are also quiet large. I\n\ns/quiet/quite/\n\n> have really tried to break things apart as best I could.\n\nYou've done a great job there, it made the series much easier to review.\n\n> The series starts by adding the new FrameBuffer interface building\n> blocks and then slowly proceeds to replace the existing API with the new\n> one. The series is tested on all upstream pipelines and IPAs without any\n> regressions.\n> \n> One note on 18/33, there is a temporary hack which is introduced in this\n> patch that allows both the Buffer and FrameBuffer API to function side\n> by side. That is i the V4L2VideoDevice is setup using the Buffer API\n> slots related to Buffer will be emitted while if it is setup with the\n> FrameBuffer API slots related to FrameBuffers are emitted. This hack is\n> removed later in 27/33 where the Buffer API is completely removed.\n> \n> I have incorporated all prototype patches published by Laurent on top of \n> this series. Either in their original form or squashed if the code is \n> introduced in this patch-set (FileDescriptor). Big thanks to Laurent for \n> this work.\n\nYou're more than welcome.\n\nI've reviewed the whole series, and I think v4 will be fully reviewed.\nThere's however one part I still dislike: the API towards pipeline\nhandlers. We can probably refactor this on top, or squash the changes if\nthey're ready in time. I'll try to give it a go.\n\n> Laurent Pinchart (2):\n> v4l2: camera_proxy: Call V4L2Camera::getBufferFd() directly\n> libcamera: v4l2_videodevice: Use FileDescriptor where appropriate\n> \n> Niklas Söderlund (31):\n> libcamera: Add FileDescriptor to help pass numerical fds around\n> test: file_descriptor: Add test\n> libcamera: utils: Add exchange()\n> v4l2: Rename FrameMetadata to V4L2FrameMetadata\n> v4l2: camera: Handle memory mapping of buffers directly\n> libcamera: buffer: Add FrameMetadata container for metadata\n> information\n> libcamera: buffer: Add FrameBuffer interface\n> ipa: Switch to FrameBuffer interface\n> libcamera: buffer: Switch from Plane to FrameBuffer::Plane\n> libcamera: buffers: Remove Plane class\n> libcamera: buffer: Drop private function setRequest()\n> libcamera: v4l2_videodevice: Align which type variable is used in\n> queueBuffer()\n> libcamera: v4l2_videodevice: Extract exportDmabufFd()\n> libcamera: request: In addBuffer() do not fetch stream from Buffer\n> libcamera: buffer: Move captured metadata to FrameMetadata\n> libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index\n> mapping\n> libcamera: v4l2_videodevice: Add FrameBuffer interface\n> test: v4l2_videodevice: Switch to FrameBuffer interface\n> test: camera: buffer_import: Update to FrameBuffer restrictions\n> libcamera: pipeline: rkisp1: Destroy frame information before\n> completing request\n> libcamera: pipeline: rkisp1: Switch to FrameBuffer interface for stat\n> and param\n> libcamera: pipeline: ipu3: Switch to FrameBuffer interface for cio2\n> and stat\n> libcamera: pipeline: Add FrameBuffer handlers\n> libcamera: allocator: Add FrameBufferAllocator to help applications\n> allocate buffers\n> libcamera: Switch to FrameBuffer interface\n> libcamera: v4l2_videodevice: Remove Buffer interface\n> libcamera: Remove dead code after switch to FrameBuffer\n> cam: Cache buffer memory mapping\n> qcam: Cache buffer memory mapping\n> libcamera: pipeline: Remove explicit buffer handling\n> libcamera: camera: Remove the prepared state\n> \n> include/ipa/ipa_interface.h | 2 +-\n> include/libcamera/buffer.h | 111 ++---\n> include/libcamera/camera.h | 13 +-\n> include/libcamera/file_descriptor.h | 46 ++\n> include/libcamera/framebuffer_allocator.h | 45 ++\n> include/libcamera/meson.build | 2 +\n> include/libcamera/request.h | 14 +-\n> include/libcamera/stream.h | 23 -\n> src/android/camera_device.cpp | 43 +-\n> src/cam/buffer_writer.cpp | 33 +-\n> src/cam/buffer_writer.h | 8 +-\n> src/cam/capture.cpp | 64 +--\n> src/cam/capture.h | 5 +-\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 9 +-\n> src/ipa/rkisp1/rkisp1.cpp | 40 +-\n> src/libcamera/buffer.cpp | 422 +++++------------\n> src/libcamera/camera.cpp | 173 +++----\n> src/libcamera/file_descriptor.cpp | 170 +++++++\n> src/libcamera/framebuffer_allocator.cpp | 218 +++++++++\n> src/libcamera/include/pipeline_handler.h | 13 +-\n> src/libcamera/include/utils.h | 9 +\n> src/libcamera/include/v4l2_videodevice.h | 65 ++-\n> src/libcamera/ipa_context_wrapper.cpp | 8 +-\n> src/libcamera/ipa_interface.cpp | 7 +-\n> src/libcamera/meson.build | 2 +\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 277 +++++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 192 ++++----\n> src/libcamera/pipeline/uvcvideo.cpp | 40 +-\n> src/libcamera/pipeline/vimc.cpp | 40 +-\n> src/libcamera/pipeline_handler.cpp | 70 ++-\n> src/libcamera/request.cpp | 38 +-\n> src/libcamera/stream.cpp | 248 +---------\n> src/libcamera/utils.cpp | 8 +\n> src/libcamera/v4l2_videodevice.cpp | 424 ++++++++++++------\n> src/qcam/main_window.cpp | 86 ++--\n> src/qcam/main_window.h | 7 +-\n> src/v4l2/v4l2_camera.cpp | 69 +--\n> src/v4l2/v4l2_camera.h | 43 +-\n> src/v4l2/v4l2_camera_proxy.cpp | 42 +-\n> src/v4l2/v4l2_camera_proxy.h | 2 +-\n> src/v4l2/v4l2_compat_manager.cpp | 2 +-\n> test/camera/buffer_import.cpp | 382 ++++------------\n> test/camera/capture.cpp | 48 +-\n> test/camera/statemachine.cpp | 89 +---\n> test/file-descriptor.cpp | 208 +++++++++\n> test/ipa/ipa_wrappers_test.cpp | 41 +-\n> test/meson.build | 1 +\n> test/v4l2_videodevice/buffer_sharing.cpp | 34 +-\n> test/v4l2_videodevice/capture_async.cpp | 18 +-\n> test/v4l2_videodevice/request_buffers.cpp | 11 +-\n> test/v4l2_videodevice/stream_on_off.cpp | 6 +-\n> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 40 +-\n> test/v4l2_videodevice/v4l2_videodevice_test.h | 2 +-\n> 53 files changed, 2086 insertions(+), 1927 deletions(-)\n> create mode 100644 include/libcamera/file_descriptor.h\n> create mode 100644 include/libcamera/framebuffer_allocator.h\n> create mode 100644 src/libcamera/file_descriptor.cpp\n> create mode 100644 src/libcamera/framebuffer_allocator.cpp\n> create mode 100644 test/file-descriptor.cpp","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10754606AC\n\tfor ;\n\tSat, 11 Jan 2020 02:52:36 +0100 (CET)","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 7A16A9B1;\n\tSat, 11 Jan 2020 02:52:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578707555;\n\tbh=Q954HHFOdOAQsJl/MRNrSgOeElI1aB1sag34D7assR8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hyhIFNcp2D2Deb6Pu9McyygXuEUXWHAuFyFqjhmESywQ52M+W9z8BPVbUkDiiDswW\n\tGf+tD3igDwGqizg2wW8MwO6JNAZp7CAT09oEccDQRIJ1Ovd1kZs3gn1gH/sRCYL+TU\n\txZw+lOkAmAde8nD1++R1zNlOyezy5G/9yzdwXUbI=","Date":"Sat, 11 Jan 2020 03:52:22 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200111015222.GR4859@pendragon.ideasonboard.com>","References":"<20200110193808.2266294-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200110193808.2266294-1-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 00/33] libcamera: Rework buffer API","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","X-List-Received-Date":"Sat, 11 Jan 2020 01:52:36 -0000"}}]