[{"id":23125,"web_url":"https://patchwork.libcamera.org/comment/23125/","msgid":"","date":"2022-05-22T10:36:28","subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:\n> This adds more portability to existing cam sinks. You can pass a\n> YUYV camera buffer and SDL will handle the pixel buffer conversion\n> to the display. This allows cam reference implementation to display\n> images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> reference implementation, to run as a desktop application in Wayland or\n> X11. SDL also has support for Android and ChromeOS which has not been\n> tested. Also tested on simpledrm Raspberry Pi 4 framebuffer\n> successfully where existing kms sink did not work. Can also be used as\n> kmsdrm sink. Only supports one camera stream at present.\n> \n> Signed-off-by: Eric Curtin \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> Tested-by: Jacopo Mondi \n> ---\n> src/cam/camera_session.cpp | 8 ++\n> src/cam/main.cpp | 4 +\n> src/cam/main.h | 1 +\n> src/cam/meson.build | 12 +++\n> src/cam/sdl_sink.cpp | 194 +++++++++++++++++++++++++++++++++++\n> src/cam/sdl_sink.h | 49 +++++++++\n> src/cam/sdl_texture.cpp | 37 +++++++\n> src/cam/sdl_texture.h | 29 ++++++\n> src/cam/sdl_texture_yuyv.cpp | 20 ++++\n> src/cam/sdl_texture_yuyv.h | 17 +++\n> 10 files changed, 371 insertions(+)\n> create mode 100644 src/cam/sdl_sink.cpp\n> create mode 100644 src/cam/sdl_sink.h\n> create mode 100644 src/cam/sdl_texture.cpp\n> create mode 100644 src/cam/sdl_texture.h\n> create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> create mode 100644 src/cam/sdl_texture_yuyv.h\n> \n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index efffafbf..336ae471 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -20,6 +20,9 @@\n> #include \"kms_sink.h\"\n> #endif\n> #include \"main.h\"\n> +#ifdef HAVE_SDL\n> +#include \"sdl_sink.h\"\n> +#endif\n> #include \"stream_options.h\"\n> \n> using namespace libcamera;\n> @@ -186,6 +189,11 @@ int CameraSession::start()\n> \t\tsink_ = std::make_unique(options_[OptDisplay].toString());\n> #endif\n> \n> +#ifdef HAVE_SDL\n> +\tif (options_.isSet(OptSDL))\n> +\t\tsink_ = std::make_unique();\n> +#endif\n> +\n> \tif (options_.isSet(OptFile)) {\n> \t\tif (!options_[OptFile].toString().empty())\n> \t\t\tsink_ = std::make_unique(streamNames_,\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index c7f664b9..962262a8 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \t\t\t \"The default file name is 'frame-#.bin'.\",\n> \t\t\t \"file\", ArgumentOptional, \"filename\", false,\n> \t\t\t OptCamera);\n> +#ifdef HAVE_SDL\n> +\tparser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> +\t\t\t \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> +#endif\n> \tparser.addOption(OptStream, &streamKeyValue,\n> \t\t\t \"Set configuration of a camera stream\", \"stream\", true,\n> \t\t\t OptCamera);\n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index 62f7bbc9..507a184f 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -17,6 +17,7 @@ enum {\n> \tOptList = 'l',\n> \tOptListProperties = 'p',\n> \tOptMonitor = 'm',\n> +\tOptSDL = 'S',\n> \tOptStream = 's',\n> \tOptListControls = 256,\n> \tOptStrictFormats = 257,\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 5bab8c9e..7d714152 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -32,12 +32,24 @@ if libdrm.found()\n> ])\n> endif\n> \n> +libsdl2 = dependency('SDL2', required : false)\n> +\n> +if libsdl2.found()\n> + cam_cpp_args += ['-DHAVE_SDL']\n> + cam_sources += files([\n> + 'sdl_sink.cpp',\n> + 'sdl_texture.cpp',\n> + 'sdl_texture_yuyv.cpp'\n> + ])\n> +endif\n> +\n> cam = executable('cam', cam_sources,\n> dependencies : [\n> libatomic,\n> libcamera_public,\n> libdrm,\n> libevent,\n> + libsdl2,\n> ],\n> cpp_args : cam_cpp_args,\n> install : true)\n> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> new file mode 100644\n> index 00000000..65430efb\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -0,0 +1,194 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_sink.h - SDL Sink\n> + */\n> +\n> +#include \"sdl_sink.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"event_loop.h\"\n> +#include \"image.h\"\n> +#include \"sdl_texture_yuyv.h\"\n> +\n> +using namespace libcamera;\n> +\n> +using namespace std::chrono_literals;\n> +\n> +SDLSink::SDLSink()\n> +\t: window_(nullptr), renderer_(nullptr), rect_({}),\n> +\t init_(false)\n> +{\n> +}\n> +\n> +SDLSink::~SDLSink()\n> +{\n> +\tstop();\n> +}\n> +\n> +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> +{\n> +\tconst int ret = FrameSink::configure(config);\n\nNo need to make ret const.\n\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tif (config.size() > 1) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> +\t\t\t<< std::endl;\n> +\t} else if (config.empty()) {\n> +\t\tstd::cerr << \"Require at least one camera stream to process\"\n> +\t\t\t << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> +\trect_.w = cfg.size.width;\n> +\trect_.h = cfg.size.height;\n> +\n> +\tswitch (cfg.pixelFormat) {\n> +\tcase libcamera::formats::YUYV:\n> +\t\ttexture_ = std::make_unique(rect_);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tstd::cerr << \"Unsupported pixel format \"\n> +\t\t\t << cfg.pixelFormat.toString() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t};\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SDLSink::start()\n> +{\n> +\tint ret = SDL_Init(SDL_INIT_VIDEO);\n> +\tif (ret) {\n> +\t\tstd::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> +\t\t\t << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tinit_ = true;\n> +\twindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> +\t\t\t\t SDL_WINDOWPOS_UNDEFINED, rect_.w,\n> +\t\t\t\t rect_.h,\n> +\t\t\t\t SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> +\tif (!window_) {\n> +\t\tstd::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> +\t\t\t << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\trenderer_ = SDL_CreateRenderer(window_, -1, 0);\n> +\tif (!renderer_) {\n> +\t\tstd::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> +\t\t\t << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);\n> +\tif (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> +\t\tstd::cerr << \"Failed to set SDL render logical size: \"\n> +\t\t\t << SDL_GetError() << std::endl;\n> +\t}\n\n\t/*\n\t * Not critical to set, don't return in this case, set for scaling\n\t * purposes.\n\t */\n\tif (ret)\n\t\tstd::cerr << \"Failed to set SDL render logical size: \"\n\t\t\t << SDL_GetError() << std::endl;\n\n> +\n> +\tret = texture_->create(renderer_);\n> +\tif (ret) {\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* \\todo Make the event cancellable to support stop/start cycles. */\n> +\tEventLoop::instance()->addTimerEvent(\n> +\t\t10ms, std::bind(&SDLSink::processSDLEvents, this));\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SDLSink::stop()\n> +{\n> +\tif (texture_) {\n> +\t\ttexture_.reset();\n> +\t}\n\nThis can be written as\n\n\ttexture_.reset();\n\nor\n\n\ttexture_ = nullptr;\n\n> +\n> +\tif (renderer_) {\n> +\t\tSDL_DestroyRenderer(renderer_);\n> +\t\trenderer_ = nullptr;\n> +\t}\n> +\n> +\tif (window_) {\n> +\t\tSDL_DestroyWindow(window_);\n> +\t\twindow_ = nullptr;\n> +\t}\n> +\n> +\tif (init_) {\n> +\t\tSDL_Quit();\n> +\t\tinit_ = false;\n> +\t}\n> +\n> +\treturn FrameSink::stop();\n> +}\n> +\n> +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> +{\n> +\tstd::unique_ptr image =\n> +\t\tImage::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> +\tassert(image != nullptr);\n> +\n> +\tmappedBuffers_[buffer] = std::move(image);\n> +}\n> +\n> +bool SDLSink::processRequest(Request *request)\n> +{\n> +\tfor (auto [stream, buffer] : request->buffers()) {\n> +\t\trenderBuffer(buffer);\n> +\t\tbreak; /* to be expanded to launch SDL window per buffer */\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +/*\n> + * Process SDL events, required for things like window resize and quit button\n> + */\n> +void SDLSink::processSDLEvents()\n> +{\n> +\tfor (SDL_Event e; SDL_PollEvent(&e);) {\n> +\t\tif (e.type == SDL_QUIT) {\n> +\t\t\t/* Click close icon then quit */\n> +\t\t\tEventLoop::instance()->exit(0);\n> +\t\t}\n> +\t}\n> +}\n> +\n> +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> +{\n> +\tImage *image = mappedBuffers_[buffer].get();\n> +\n> +\t/* \\todo Implement support for multi-planar formats. */\n> +\tconst FrameMetadata::Plane &meta =\n> +\t\tbuffer->metadata().planes()[0];\n\nThis holds on a single line.\n\n> +\n> +\tSpan data = image->data(0);\n> +\tif (meta.bytesused > data.size())\n> +\t\tstd::cerr << \"payload size \" << meta.bytesused\n> +\t\t\t << \" larger than plane size \" << data.size()\n> +\t\t\t << std::endl;\n> +\n> +\ttexture_->update(data);\n> +\n> +\tSDL_RenderClear(renderer_);\n> +\tSDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);\n> +\tSDL_RenderPresent(renderer_);\n> +}\n> diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> new file mode 100644\n> index 00000000..83171cca\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.h\n> @@ -0,0 +1,49 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_sink.h - SDL Sink\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"frame_sink.h\"\n> +\n> +class Image;\n> +class SDLTexture;\n> +\n> +class SDLSink : public FrameSink\n> +{\n> +public:\n> +\tSDLSink();\n> +\t~SDLSink();\n> +\n> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> +\tint start() override;\n> +\tint stop() override;\n> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tbool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> +\tvoid renderBuffer(libcamera::FrameBuffer *buffer);\n> +\tvoid processSDLEvents();\n> +\n> +\tstd::map>\n> +\t\tmappedBuffers_;\n> +\n> +\tstd::unique_ptr texture_;\n> +\n> +\tSDL_Window *window_;\n> +\tSDL_Renderer *renderer_;\n> +\tSDL_Rect rect_;\n> +\tSDL_PixelFormatEnum pixelFormat_;\n> +\tbool init_;\n> +};\n> diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> new file mode 100644\n> index 00000000..ac355a97\n> --- /dev/null\n> +++ b/src/cam/sdl_texture.cpp\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_texture.cpp - SDL Texture\n> + */\n> +\n> +#include \"sdl_texture.h\"\n> +\n> +#include \n> +\n> +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> +\t\t const int pitch)\n> +\t: ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)\n> +{\n> +}\n> +\n> +SDLTexture::~SDLTexture()\n> +{\n> +\tif (ptr_) {\n> +\t\tSDL_DestroyTexture(ptr_);\n> +\t}\n\nNo need for curly braces.\n\n> +}\n> +\n> +int SDLTexture::create(SDL_Renderer *renderer_)\n> +{\n> +\tptr_ = SDL_CreateTexture(renderer_, pixelFormat_,\n> +\t\t\t\t SDL_TEXTUREACCESS_STREAMING, rect_.w,\n> +\t\t\t\t rect_.h);\n> +\tif (!ptr_) {\n> +\t\tstd::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> +\t\t\t << std::endl;\n> +\t\treturn -ENOMEM;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> new file mode 100644\n> index 00000000..b04eece0\n> --- /dev/null\n> +++ b/src/cam/sdl_texture.h\n> @@ -0,0 +1,29 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_texture.h - SDL Texture\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"image.h\"\n> +\n> +class SDLTexture\n> +{\n> +public:\n> +\tSDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> +\t\t const int pitch);\n> +\tvirtual ~SDLTexture();\n> +\tint create(SDL_Renderer *renderer_);\n\ns/renderer_/renderer/\n\n> +\tvirtual void update(const libcamera::Span &data) = 0;\n> +\tSDL_Texture *get() const { return ptr_; }\n> +\n> +protected:\n> +\tSDL_Texture *ptr_;\n> +\tconst SDL_Rect &rect_;\n\nThis should be a copy, not a reference, as the caller shouldn't need to\nguarantee that the rectangle passed to the constructor will not be\ndestroyed as long as the texture exists.\n\nThese are small issues, I can fix them when applying.\n\n> +\tSDL_PixelFormatEnum pixelFormat_;\n> +\tconst int pitch_;\n> +};\n> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> new file mode 100644\n> index 00000000..a219097b\n> --- /dev/null\n> +++ b/src/cam/sdl_texture_yuyv.cpp\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_texture_yuyv.cpp - SDL Texture YUYV\n> + */\n> +\n> +#include \"sdl_texture_yuyv.h\"\n> +\n> +using namespace libcamera;\n> +\n> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> +\t: SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> +{\n> +}\n> +\n> +void SDLTextureYUYV::update(const Span &data)\n> +{\n> +\tSDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);\n> +}\n> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> new file mode 100644\n> index 00000000..38c51c74\n> --- /dev/null\n> +++ b/src/cam/sdl_texture_yuyv.h\n> @@ -0,0 +1,17 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas on Board Oy\n> + *\n> + * sdl_texture_yuyv.h - SDL Texture YUYV\n> + */\n> +\n> +#pragma once\n> +\n> +#include \"sdl_texture.h\"\n> +\n> +class SDLTextureYUYV : public SDLTexture\n> +{\n> +public:\n> +\tSDLTextureYUYV(const SDL_Rect &sdlRect);\n> +\tvoid update(const libcamera::Span &data) override;\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 0A191BD160\n\tfor ;\n\tSun, 22 May 2022 10:37:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AA2265663;\n\tSun, 22 May 2022 12:37:40 +0200 (CEST)","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 B0E2D60419\n\tfor ;\n\tSun, 22 May 2022 12:37:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(static-11-127-145-212.ipcom.comunitel.net [212.145.127.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE6FC563;\n\tSun, 22 May 2022 12:37:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653215860;\n\tbh=z7pwVsoHxu79OOMzRf7/LqDEeXX8uNGTmyLWhbxLAp8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=V0b6eV4Qxkut+YU2blx0OyEnD96uUVqqppoC5nsCiVFzN+6p8tFoaDUP+CDqd3fjv\n\tPbXF1YwLv04iIP9ida7n1paWgXYXX14mKc/SQ2zbJ1d4lPWGpp/nHmvS5up82EiQff\n\tC+fqmcfILsILs+JgBb/8jPc3GQz9nZqdEaMMCUiUaMS3BWqt07Mj/h7j273d2ZWpLV\n\tJqn3rGP/PGd+qIcKykZUPBpkS43zyp6RgykctVw6eH9Ot3GWlXME/HpsBDY+mtqPXV\n\t375g2TJeonkuYIzF6P/EZ3U3EL5BUdL7Mp5BF4pDC1LHgejHVB5FmFtDpH71TZ3Ktz\n\tZ6ekz6q0IosKg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653215858;\n\tbh=z7pwVsoHxu79OOMzRf7/LqDEeXX8uNGTmyLWhbxLAp8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qjw3cbhIhILBeP+OjMl9qZo880Rb0t6kTqDaJRMWGXac+f68SJGOhpYLcMXZVQITy\n\t64xI8gjM9pi7nNtSEVAf6Z9+Lfn2B2pF6rTtWY9aNXBG9OCS4SgXzHGn4dvqhdf2pz\n\t/3GE8WA2PlDcxpgWFO73khAFTtgNqzqmTSjv9pME="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qjw3cbhI\"; dkim-atps=neutral","Date":"Sun, 22 May 2022 13:36:28 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-4-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220520190106.425386-4-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23127,"web_url":"https://patchwork.libcamera.org/comment/23127/","msgid":"","date":"2022-05-22T17:31:51","subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Another small comment.\n\nOn Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:\n> > This adds more portability to existing cam sinks. You can pass a\n> > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > to the display. This allows cam reference implementation to display\n> > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > reference implementation, to run as a desktop application in Wayland or\n> > X11. SDL also has support for Android and ChromeOS which has not been\n> > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer\n> > successfully where existing kms sink did not work. Can also be used as\n> > kmsdrm sink. Only supports one camera stream at present.\n> > \n> > Signed-off-by: Eric Curtin \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > Tested-by: Jacopo Mondi \n> > ---\n> > src/cam/camera_session.cpp | 8 ++\n> > src/cam/main.cpp | 4 +\n> > src/cam/main.h | 1 +\n> > src/cam/meson.build | 12 +++\n> > src/cam/sdl_sink.cpp | 194 +++++++++++++++++++++++++++++++++++\n> > src/cam/sdl_sink.h | 49 +++++++++\n> > src/cam/sdl_texture.cpp | 37 +++++++\n> > src/cam/sdl_texture.h | 29 ++++++\n> > src/cam/sdl_texture_yuyv.cpp | 20 ++++\n> > src/cam/sdl_texture_yuyv.h | 17 +++\n> > 10 files changed, 371 insertions(+)\n> > create mode 100644 src/cam/sdl_sink.cpp\n> > create mode 100644 src/cam/sdl_sink.h\n> > create mode 100644 src/cam/sdl_texture.cpp\n> > create mode 100644 src/cam/sdl_texture.h\n> > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > \n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index efffafbf..336ae471 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -20,6 +20,9 @@\n> > #include \"kms_sink.h\"\n> > #endif\n> > #include \"main.h\"\n> > +#ifdef HAVE_SDL\n> > +#include \"sdl_sink.h\"\n> > +#endif\n> > #include \"stream_options.h\"\n> > \n> > using namespace libcamera;\n> > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > \t\tsink_ = std::make_unique(options_[OptDisplay].toString());\n> > #endif\n> > \n> > +#ifdef HAVE_SDL\n> > +\tif (options_.isSet(OptSDL))\n> > +\t\tsink_ = std::make_unique();\n> > +#endif\n> > +\n> > \tif (options_.isSet(OptFile)) {\n> > \t\tif (!options_[OptFile].toString().empty())\n> > \t\t\tsink_ = std::make_unique(streamNames_,\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index c7f664b9..962262a8 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \t\t\t \"The default file name is 'frame-#.bin'.\",\n> > \t\t\t \"file\", ArgumentOptional, \"filename\", false,\n> > \t\t\t OptCamera);\n> > +#ifdef HAVE_SDL\n> > +\tparser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > +\t\t\t \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > +#endif\n> > \tparser.addOption(OptStream, &streamKeyValue,\n> > \t\t\t \"Set configuration of a camera stream\", \"stream\", true,\n> > \t\t\t OptCamera);\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index 62f7bbc9..507a184f 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -17,6 +17,7 @@ enum {\n> > \tOptList = 'l',\n> > \tOptListProperties = 'p',\n> > \tOptMonitor = 'm',\n> > +\tOptSDL = 'S',\n> > \tOptStream = 's',\n> > \tOptListControls = 256,\n> > \tOptStrictFormats = 257,\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 5bab8c9e..7d714152 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -32,12 +32,24 @@ if libdrm.found()\n> > ])\n> > endif\n> > \n> > +libsdl2 = dependency('SDL2', required : false)\n> > +\n> > +if libsdl2.found()\n> > + cam_cpp_args += ['-DHAVE_SDL']\n> > + cam_sources += files([\n> > + 'sdl_sink.cpp',\n> > + 'sdl_texture.cpp',\n> > + 'sdl_texture_yuyv.cpp'\n> > + ])\n> > +endif\n> > +\n> > cam = executable('cam', cam_sources,\n> > dependencies : [\n> > libatomic,\n> > libcamera_public,\n> > libdrm,\n> > libevent,\n> > + libsdl2,\n> > ],\n> > cpp_args : cam_cpp_args,\n> > install : true)\n> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > new file mode 100644\n> > index 00000000..65430efb\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -0,0 +1,194 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_sink.h - SDL Sink\n> > + */\n> > +\n> > +#include \"sdl_sink.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"event_loop.h\"\n> > +#include \"image.h\"\n> > +#include \"sdl_texture_yuyv.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +using namespace std::chrono_literals;\n> > +\n> > +SDLSink::SDLSink()\n> > +\t: window_(nullptr), renderer_(nullptr), rect_({}),\n> > +\t init_(false)\n> > +{\n> > +}\n> > +\n> > +SDLSink::~SDLSink()\n> > +{\n> > +\tstop();\n> > +}\n> > +\n> > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > +{\n> > +\tconst int ret = FrameSink::configure(config);\n> \n> No need to make ret const.\n> \n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tif (config.size() > 1) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > +\t\t\t<< std::endl;\n> > +\t} else if (config.empty()) {\n> > +\t\tstd::cerr << \"Require at least one camera stream to process\"\n> > +\t\t\t << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> > +\trect_.w = cfg.size.width;\n> > +\trect_.h = cfg.size.height;\n> > +\n> > +\tswitch (cfg.pixelFormat) {\n> > +\tcase libcamera::formats::YUYV:\n> > +\t\ttexture_ = std::make_unique(rect_);\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tstd::cerr << \"Unsupported pixel format \"\n> > +\t\t\t << cfg.pixelFormat.toString() << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t};\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int SDLSink::start()\n> > +{\n> > +\tint ret = SDL_Init(SDL_INIT_VIDEO);\n> > +\tif (ret) {\n> > +\t\tstd::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > +\t\t\t << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tinit_ = true;\n> > +\twindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > +\t\t\t\t SDL_WINDOWPOS_UNDEFINED, rect_.w,\n> > +\t\t\t\t rect_.h,\n> > +\t\t\t\t SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > +\tif (!window_) {\n> > +\t\tstd::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > +\t\t\t << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\trenderer_ = SDL_CreateRenderer(window_, -1, 0);\n> > +\tif (!renderer_) {\n> > +\t\tstd::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > +\t\t\t << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);\n> > +\tif (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > +\t\tstd::cerr << \"Failed to set SDL render logical size: \"\n> > +\t\t\t << SDL_GetError() << std::endl;\n> > +\t}\n> \n> \t/*\n> \t * Not critical to set, don't return in this case, set for scaling\n> \t * purposes.\n> \t */\n> \tif (ret)\n> \t\tstd::cerr << \"Failed to set SDL render logical size: \"\n> \t\t\t << SDL_GetError() << std::endl;\n> \n> > +\n> > +\tret = texture_->create(renderer_);\n> > +\tif (ret) {\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* \\todo Make the event cancellable to support stop/start cycles. */\n> > +\tEventLoop::instance()->addTimerEvent(\n> > +\t\t10ms, std::bind(&SDLSink::processSDLEvents, this));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int SDLSink::stop()\n> > +{\n> > +\tif (texture_) {\n> > +\t\ttexture_.reset();\n> > +\t}\n> \n> This can be written as\n> \n> \ttexture_.reset();\n> \n> or\n> \n> \ttexture_ = nullptr;\n> \n> > +\n> > +\tif (renderer_) {\n> > +\t\tSDL_DestroyRenderer(renderer_);\n> > +\t\trenderer_ = nullptr;\n> > +\t}\n> > +\n> > +\tif (window_) {\n> > +\t\tSDL_DestroyWindow(window_);\n> > +\t\twindow_ = nullptr;\n> > +\t}\n> > +\n> > +\tif (init_) {\n> > +\t\tSDL_Quit();\n> > +\t\tinit_ = false;\n> > +\t}\n> > +\n> > +\treturn FrameSink::stop();\n> > +}\n> > +\n> > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tstd::unique_ptr image =\n> > +\t\tImage::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > +\tassert(image != nullptr);\n> > +\n> > +\tmappedBuffers_[buffer] = std::move(image);\n> > +}\n> > +\n> > +bool SDLSink::processRequest(Request *request)\n> > +{\n> > +\tfor (auto [stream, buffer] : request->buffers()) {\n> > +\t\trenderBuffer(buffer);\n> > +\t\tbreak; /* to be expanded to launch SDL window per buffer */\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/*\n> > + * Process SDL events, required for things like window resize and quit button\n> > + */\n> > +void SDLSink::processSDLEvents()\n> > +{\n> > +\tfor (SDL_Event e; SDL_PollEvent(&e);) {\n> > +\t\tif (e.type == SDL_QUIT) {\n> > +\t\t\t/* Click close icon then quit */\n> > +\t\t\tEventLoop::instance()->exit(0);\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tImage *image = mappedBuffers_[buffer].get();\n> > +\n> > +\t/* \\todo Implement support for multi-planar formats. */\n> > +\tconst FrameMetadata::Plane &meta =\n> > +\t\tbuffer->metadata().planes()[0];\n> \n> This holds on a single line.\n> \n> > +\n> > +\tSpan data = image->data(0);\n> > +\tif (meta.bytesused > data.size())\n> > +\t\tstd::cerr << \"payload size \" << meta.bytesused\n> > +\t\t\t << \" larger than plane size \" << data.size()\n> > +\t\t\t << std::endl;\n> > +\n> > +\ttexture_->update(data);\n> > +\n> > +\tSDL_RenderClear(renderer_);\n> > +\tSDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);\n> > +\tSDL_RenderPresent(renderer_);\n> > +}\n> > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > new file mode 100644\n> > index 00000000..83171cca\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.h\n> > @@ -0,0 +1,49 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_sink.h - SDL Sink\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"frame_sink.h\"\n> > +\n> > +class Image;\n> > +class SDLTexture;\n> > +\n> > +class SDLSink : public FrameSink\n> > +{\n> > +public:\n> > +\tSDLSink();\n> > +\t~SDLSink();\n> > +\n> > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > +\tint start() override;\n> > +\tint stop() override;\n> > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > +\tbool processRequest(libcamera::Request *request) override;\n> > +\n> > +private:\n> > +\tvoid renderBuffer(libcamera::FrameBuffer *buffer);\n> > +\tvoid processSDLEvents();\n> > +\n> > +\tstd::map>\n> > +\t\tmappedBuffers_;\n> > +\n> > +\tstd::unique_ptr texture_;\n> > +\n> > +\tSDL_Window *window_;\n> > +\tSDL_Renderer *renderer_;\n> > +\tSDL_Rect rect_;\n> > +\tSDL_PixelFormatEnum pixelFormat_;\n\nThis isn't used and can be dropped.\n\n> > +\tbool init_;\n> > +};\n> > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > new file mode 100644\n> > index 00000000..ac355a97\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture.cpp\n> > @@ -0,0 +1,37 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_texture.cpp - SDL Texture\n> > + */\n> > +\n> > +#include \"sdl_texture.h\"\n> > +\n> > +#include \n> > +\n> > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > +\t\t const int pitch)\n> > +\t: ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > +{\n> > +}\n> > +\n> > +SDLTexture::~SDLTexture()\n> > +{\n> > +\tif (ptr_) {\n> > +\t\tSDL_DestroyTexture(ptr_);\n> > +\t}\n> \n> No need for curly braces.\n> \n> > +}\n> > +\n> > +int SDLTexture::create(SDL_Renderer *renderer_)\n> > +{\n> > +\tptr_ = SDL_CreateTexture(renderer_, pixelFormat_,\n> > +\t\t\t\t SDL_TEXTUREACCESS_STREAMING, rect_.w,\n> > +\t\t\t\t rect_.h);\n> > +\tif (!ptr_) {\n> > +\t\tstd::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > +\t\t\t << std::endl;\n> > +\t\treturn -ENOMEM;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > new file mode 100644\n> > index 00000000..b04eece0\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture.h\n> > @@ -0,0 +1,29 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_texture.h - SDL Texture\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"image.h\"\n> > +\n> > +class SDLTexture\n> > +{\n> > +public:\n> > +\tSDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > +\t\t const int pitch);\n> > +\tvirtual ~SDLTexture();\n> > +\tint create(SDL_Renderer *renderer_);\n> \n> s/renderer_/renderer/\n> \n> > +\tvirtual void update(const libcamera::Span &data) = 0;\n> > +\tSDL_Texture *get() const { return ptr_; }\n> > +\n> > +protected:\n> > +\tSDL_Texture *ptr_;\n> > +\tconst SDL_Rect &rect_;\n> \n> This should be a copy, not a reference, as the caller shouldn't need to\n> guarantee that the rectangle passed to the constructor will not be\n> destroyed as long as the texture exists.\n> \n> These are small issues, I can fix them when applying.\n> \n> > +\tSDL_PixelFormatEnum pixelFormat_;\n> > +\tconst int pitch_;\n> > +};\n> > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > new file mode 100644\n> > index 00000000..a219097b\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > @@ -0,0 +1,20 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_texture_yuyv.cpp - SDL Texture YUYV\n> > + */\n> > +\n> > +#include \"sdl_texture_yuyv.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > +\t: SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > +{\n> > +}\n> > +\n> > +void SDLTextureYUYV::update(const Span &data)\n> > +{\n> > +\tSDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);\n> > +}\n> > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > new file mode 100644\n> > index 00000000..38c51c74\n> > --- /dev/null\n> > +++ b/src/cam/sdl_texture_yuyv.h\n> > @@ -0,0 +1,17 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Ideas on Board Oy\n> > + *\n> > + * sdl_texture_yuyv.h - SDL Texture YUYV\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \"sdl_texture.h\"\n> > +\n> > +class SDLTextureYUYV : public SDLTexture\n> > +{\n> > +public:\n> > +\tSDLTextureYUYV(const SDL_Rect &sdlRect);\n> > +\tvoid update(const libcamera::Span &data) override;\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 68C90BD160\n\tfor ;\n\tSun, 22 May 2022 17:32:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8669865663;\n\tSun, 22 May 2022 19:32:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23A2760419\n\tfor ;\n\tSun, 22 May 2022 19:32:00 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [86.8.200.222])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B51345F;\n\tSun, 22 May 2022 19:31:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653240721;\n\tbh=n6vVhYFAnAyokGSOw0Cg9divnbU4wkPX9nc2oaTheMg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=BR9yadBxOovbszeLQr5IhF/L/eR7GCGqYSnibY1ibZS4KHZ2Ht+djY+onqEM+X1sR\n\tfvG8WmwduJXYt4k3PfqwmOnlQp0Y6hrBgPLFfAsphkeCWelpW8R2V+u5d/PFQAZ7ye\n\teAoNoq9AA7lX2pZGAy11Zw1Hk6jzv6De7fByImy3Qm74tn6kVN3wsEbxOj7Sz5jBwm\n\tRa6iiCYUenijPLFTA5c6aVpBbdJ4sLS404850xGYpc5+tGvdZF9bg/Bjogj3/NtAaI\n\tXLA0Ig0LW2EILiOYv3mt1R/sFmOmn4B2j1D3DDqfpQ0CSjovd7wRKVF+4ap1koqDh0\n\tL97IMP9EaF9ww==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653240719;\n\tbh=n6vVhYFAnAyokGSOw0Cg9divnbU4wkPX9nc2oaTheMg=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=tbbaSno7W+H1jvQDq6f6RyPO8fLcXAj3/kXoYxg6xxZxwzxoX0sj14P5pU4yaPZYn\n\tlNe4vXz9+I/SuscU9zEuec6u96eTQBrY4xa6bSHaBA9iW7ktapSgLB+IGn4ziXHrK6\n\tsfkNO5tyhZJYzvSY+L0oc3PNYZK+fV11u8yD9cLk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tbbaSno7\"; dkim-atps=neutral","Date":"Sun, 22 May 2022 20:31:51 +0300","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-4-ecurtin@redhat.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23131,"web_url":"https://patchwork.libcamera.org/comment/23131/","msgid":"","date":"2022-05-22T20:08:09","subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Sun, 22 May 2022 at 18:32, Laurent Pinchart\n wrote:\n>\n> Another small comment.\n>\n> On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:\n> > > This adds more portability to existing cam sinks. You can pass a\n> > > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > > to the display. This allows cam reference implementation to display\n> > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > > reference implementation, to run as a desktop application in Wayland or\n> > > X11. SDL also has support for Android and ChromeOS which has not been\n> > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer\n> > > successfully where existing kms sink did not work. Can also be used as\n> > > kmsdrm sink. Only supports one camera stream at present.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > Reviewed-by: Laurent Pinchart \n> > > Reviewed-by: Kieran Bingham \n> > > Tested-by: Jacopo Mondi \n> > > ---\n> > > src/cam/camera_session.cpp | 8 ++\n> > > src/cam/main.cpp | 4 +\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 12 +++\n> > > src/cam/sdl_sink.cpp | 194 +++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 49 +++++++++\n> > > src/cam/sdl_texture.cpp | 37 +++++++\n> > > src/cam/sdl_texture.h | 29 ++++++\n> > > src/cam/sdl_texture_yuyv.cpp | 20 ++++\n> > > src/cam/sdl_texture_yuyv.h | 17 +++\n> > > 10 files changed, 371 insertions(+)\n> > > create mode 100644 src/cam/sdl_sink.cpp\n> > > create mode 100644 src/cam/sdl_sink.h\n> > > create mode 100644 src/cam/sdl_texture.cpp\n> > > create mode 100644 src/cam/sdl_texture.h\n> > > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > >\n> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > index efffafbf..336ae471 100644\n> > > --- a/src/cam/camera_session.cpp\n> > > +++ b/src/cam/camera_session.cpp\n> > > @@ -20,6 +20,9 @@\n> > > #include \"kms_sink.h\"\n> > > #endif\n> > > #include \"main.h\"\n> > > +#ifdef HAVE_SDL\n> > > +#include \"sdl_sink.h\"\n> > > +#endif\n> > > #include \"stream_options.h\"\n> > >\n> > > using namespace libcamera;\n> > > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > > #endif\n> > >\n> > > +#ifdef HAVE_SDL\n> > > + if (options_.isSet(OptSDL))\n> > > + sink_ = std::make_unique();\n> > > +#endif\n> > > +\n> > > if (options_.isSet(OptFile)) {\n> > > if (!options_[OptFile].toString().empty())\n> > > sink_ = std::make_unique(streamNames_,\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index c7f664b9..962262a8 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"The default file name is 'frame-#.bin'.\",\n> > > \"file\", ArgumentOptional, \"filename\", false,\n> > > OptCamera);\n> > > +#ifdef HAVE_SDL\n> > > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > > +#endif\n> > > parser.addOption(OptStream, &streamKeyValue,\n> > > \"Set configuration of a camera stream\", \"stream\", true,\n> > > OptCamera);\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index 62f7bbc9..507a184f 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -17,6 +17,7 @@ enum {\n> > > OptList = 'l',\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > + OptSDL = 'S',\n> > > OptStream = 's',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index 5bab8c9e..7d714152 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -32,12 +32,24 @@ if libdrm.found()\n> > > ])\n> > > endif\n> > >\n> > > +libsdl2 = dependency('SDL2', required : false)\n> > > +\n> > > +if libsdl2.found()\n> > > + cam_cpp_args += ['-DHAVE_SDL']\n> > > + cam_sources += files([\n> > > + 'sdl_sink.cpp',\n> > > + 'sdl_texture.cpp',\n> > > + 'sdl_texture_yuyv.cpp'\n> > > + ])\n> > > +endif\n> > > +\n> > > cam = executable('cam', cam_sources,\n> > > dependencies : [\n> > > libatomic,\n> > > libcamera_public,\n> > > libdrm,\n> > > libevent,\n> > > + libsdl2,\n> > > ],\n> > > cpp_args : cam_cpp_args,\n> > > install : true)\n> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > new file mode 100644\n> > > index 00000000..65430efb\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,194 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_sink.h - SDL Sink\n> > > + */\n> > > +\n> > > +#include \"sdl_sink.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"event_loop.h\"\n> > > +#include \"image.h\"\n> > > +#include \"sdl_texture_yuyv.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +using namespace std::chrono_literals;\n> > > +\n> > > +SDLSink::SDLSink()\n> > > + : window_(nullptr), renderer_(nullptr), rect_({}),\n> > > + init_(false)\n> > > +{\n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + stop();\n> > > +}\n> > > +\n> > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > +{\n> > > + const int ret = FrameSink::configure(config);\n> >\n> > No need to make ret const.\n> >\n> > > + if (ret < 0)\n> > > + return ret;\n> > > +\n> > > + if (config.size() > 1) {\n> > > + std::cerr\n> > > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > + << std::endl;\n> > > + } else if (config.empty()) {\n> > > + std::cerr << \"Require at least one camera stream to process\"\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > + rect_.w = cfg.size.width;\n> > > + rect_.h = cfg.size.height;\n> > > +\n> > > + switch (cfg.pixelFormat) {\n> > > + case libcamera::formats::YUYV:\n> > > + texture_ = std::make_unique(rect_);\n> > > + break;\n> > > + default:\n> > > + std::cerr << \"Unsupported pixel format \"\n> > > + << cfg.pixelFormat.toString() << std::endl;\n> > > + return -EINVAL;\n> > > + };\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::start()\n> > > +{\n> > > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > > + if (ret) {\n> > > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return ret;\n> > > + }\n> > > +\n> > > + init_ = true;\n> > > + window_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > + SDL_WINDOWPOS_UNDEFINED, rect_.w,\n> > > + rect_.h,\n> > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > + if (!window_) {\n> > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + renderer_ = SDL_CreateRenderer(window_, -1, 0);\n> > > + if (!renderer_) {\n> > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);\n> > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > > + std::cerr << \"Failed to set SDL render logical size: \"\n> > > + << SDL_GetError() << std::endl;\n> > > + }\n> >\n> > /*\n> > * Not critical to set, don't return in this case, set for scaling\n> > * purposes.\n> > */\n> > if (ret)\n> > std::cerr << \"Failed to set SDL render logical size: \"\n> > << SDL_GetError() << std::endl;\n> >\n> > > +\n> > > + ret = texture_->create(renderer_);\n> > > + if (ret) {\n> > > + return ret;\n> > > + }\n> > > +\n> > > + /* \\todo Make the event cancellable to support stop/start cycles. */\n> > > + EventLoop::instance()->addTimerEvent(\n> > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::stop()\n> > > +{\n> > > + if (texture_) {\n> > > + texture_.reset();\n> > > + }\n> >\n> > This can be written as\n> >\n> > texture_.reset();\n> >\n> > or\n> >\n> > texture_ = nullptr;\n> >\n> > > +\n> > > + if (renderer_) {\n> > > + SDL_DestroyRenderer(renderer_);\n> > > + renderer_ = nullptr;\n> > > + }\n> > > +\n> > > + if (window_) {\n> > > + SDL_DestroyWindow(window_);\n> > > + window_ = nullptr;\n> > > + }\n> > > +\n> > > + if (init_) {\n> > > + SDL_Quit();\n> > > + init_ = false;\n> > > + }\n> > > +\n> > > + return FrameSink::stop();\n> > > +}\n> > > +\n> > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + std::unique_ptr image =\n> > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > > + assert(image != nullptr);\n> > > +\n> > > + mappedBuffers_[buffer] = std::move(image);\n> > > +}\n> > > +\n> > > +bool SDLSink::processRequest(Request *request)\n> > > +{\n> > > + for (auto [stream, buffer] : request->buffers()) {\n> > > + renderBuffer(buffer);\n> > > + break; /* to be expanded to launch SDL window per buffer */\n> > > + }\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Process SDL events, required for things like window resize and quit button\n> > > + */\n> > > +void SDLSink::processSDLEvents()\n> > > +{\n> > > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > > + if (e.type == SDL_QUIT) {\n> > > + /* Click close icon then quit */\n> > > + EventLoop::instance()->exit(0);\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + Image *image = mappedBuffers_[buffer].get();\n> > > +\n> > > + /* \\todo Implement support for multi-planar formats. */\n> > > + const FrameMetadata::Plane &meta =\n> > > + buffer->metadata().planes()[0];\n> >\n> > This holds on a single line.\n> >\n> > > +\n> > > + Span data = image->data(0);\n> > > + if (meta.bytesused > data.size())\n> > > + std::cerr << \"payload size \" << meta.bytesused\n> > > + << \" larger than plane size \" << data.size()\n> > > + << std::endl;\n> > > +\n> > > + texture_->update(data);\n> > > +\n> > > + SDL_RenderClear(renderer_);\n> > > + SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);\n> > > + SDL_RenderPresent(renderer_);\n> > > +}\n> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > new file mode 100644\n> > > index 00000000..83171cca\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,49 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_sink.h - SDL Sink\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"frame_sink.h\"\n> > > +\n> > > +class Image;\n> > > +class SDLTexture;\n> > > +\n> > > +class SDLSink : public FrameSink\n> > > +{\n> > > +public:\n> > > + SDLSink();\n> > > + ~SDLSink();\n> > > +\n> > > + int configure(const libcamera::CameraConfiguration &config) override;\n> > > + int start() override;\n> > > + int stop() override;\n> > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > +\n> > > + bool processRequest(libcamera::Request *request) override;\n> > > +\n> > > +private:\n> > > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > > + void processSDLEvents();\n> > > +\n> > > + std::map>\n> > > + mappedBuffers_;\n> > > +\n> > > + std::unique_ptr texture_;\n> > > +\n> > > + SDL_Window *window_;\n> > > + SDL_Renderer *renderer_;\n> > > + SDL_Rect rect_;\n> > > + SDL_PixelFormatEnum pixelFormat_;\n>\n> This isn't used and can be dropped.\n>\n> > > + bool init_;\n> > > +};\n> > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > > new file mode 100644\n> > > index 00000000..ac355a97\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture.cpp\n> > > @@ -0,0 +1,37 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_texture.cpp - SDL Texture\n> > > + */\n> > > +\n> > > +#include \"sdl_texture.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > + const int pitch)\n> > > + : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > > +{\n> > > +}\n> > > +\n> > > +SDLTexture::~SDLTexture()\n> > > +{\n> > > + if (ptr_) {\n> > > + SDL_DestroyTexture(ptr_);\n> > > + }\n> >\n> > No need for curly braces.\n> >\n> > > +}\n> > > +\n> > > +int SDLTexture::create(SDL_Renderer *renderer_)\n> > > +{\n> > > + ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,\n> > > + SDL_TEXTUREACCESS_STREAMING, rect_.w,\n> > > + rect_.h);\n> > > + if (!ptr_) {\n> > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > > + << std::endl;\n> > > + return -ENOMEM;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > > new file mode 100644\n> > > index 00000000..b04eece0\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture.h\n> > > @@ -0,0 +1,29 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_texture.h - SDL Texture\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"image.h\"\n> > > +\n> > > +class SDLTexture\n> > > +{\n> > > +public:\n> > > + SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > + const int pitch);\n> > > + virtual ~SDLTexture();\n> > > + int create(SDL_Renderer *renderer_);\n> >\n> > s/renderer_/renderer/\n> >\n> > > + virtual void update(const libcamera::Span &data) = 0;\n> > > + SDL_Texture *get() const { return ptr_; }\n> > > +\n> > > +protected:\n> > > + SDL_Texture *ptr_;\n> > > + const SDL_Rect &rect_;\n> >\n> > This should be a copy, not a reference, as the caller shouldn't need to\n> > guarantee that the rectangle passed to the constructor will not be\n> > destroyed as long as the texture exists.\n> >\n> > These are small issues, I can fix them when applying.\n> >\n> > > + SDL_PixelFormatEnum pixelFormat_;\n> > > + const int pitch_;\n> > > +};\n> > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > > new file mode 100644\n> > > index 00000000..a219097b\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > > @@ -0,0 +1,20 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV\n> > > + */\n> > > +\n> > > +#include \"sdl_texture_yuyv.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > > +{\n> > > +}\n> > > +\n> > > +void SDLTextureYUYV::update(const Span &data)\n> > > +{\n> > > + SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);\n> > > +}\n> > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > > new file mode 100644\n> > > index 00000000..38c51c74\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_texture_yuyv.h\n> > > @@ -0,0 +1,17 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > + *\n> > > + * sdl_texture_yuyv.h - SDL Texture YUYV\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \"sdl_texture.h\"\n> > > +\n> > > +class SDLTextureYUYV : public SDLTexture\n> > > +{\n> > > +public:\n> > > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > > + void update(const libcamera::Span &data) override;\n> > > +};\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nThere is an extra \"SDL_PixelFormatEnum pixelFormat_;\" sorry, if you'd\nlike to make changes no problem on my end. The main thing for me is to\nhave this merged and working for YUYV and MJPG and build on it from\nthere.","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 776C3BD160\n\tfor ;\n\tSun, 22 May 2022 20:08:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C22AB65664;\n\tSun, 22 May 2022 22:08:29 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4A266041B\n\tfor ;\n\tSun, 22 May 2022 22:08:28 +0200 (CEST)","from mail-qk1-f199.google.com (mail-qk1-f199.google.com\n\t[209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-371-85RtbSeKPyetV3bqJz-1nw-1; Sun, 22 May 2022 16:08:26 -0400","by mail-qk1-f199.google.com with SMTP id\n\tq5-20020a05620a0d8500b004738c1b48beso10199183qkl.7\n\tfor ;\n\tSun, 22 May 2022 13:08:26 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653250109;\n\tbh=mhr7K7oKrRfljruxpzmtOTPwesKw1zrq7lKexvo5gAM=;\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=dWFwRFxpUTw1LrblnM11DHljS54WdYsPGyZu0Ons4i1uNlvJrN3q1OAG4QN7XgOlt\n\tRW6xsTd0qnGgT9PkctKrWsuLQ57IINqizle7u1qJR5PcbMtkHI9xjXLtQmeqLIaS/A\n\tm3hSSUaBhO9/TzF6LEHzfa5byYK/fRgufoxfwj0IQ4W3agVBTNGhKm5i2jKXemDVPX\n\txPzPv5g8T9hSUVtf+MxYbudtZ2x9TL1YFGOnlC6i3Sat1I+Wt116acyN5rNJPQfV+s\n\tqVjnFQNT1+J4+QuawNL0pFBa3UectdRVqeYW0oyA3eu3tQx+g0EmeoGS0mcy6Jf7Sv\n\tiX65JLnivMm7w==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1653250107;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=cs2zkCZXfQ4eL0rWTimsq5nOhtO7ioNIuOuQwptUZgQ=;\n\tb=jPKRP9GlqqwG1Abig/STrVndAh45wNV9AYbA7QiirtW+pBNHyL3Nm7pr+XWaht4aUH44k4\n\t7p5FiXSICe0Kr9rm3X+H4HdH0wYo3F5t67mPBYTr9oZe3VEpeShcHvmvPdCsyQ3/NN6iLQ\n\twFMds6V1yyqU/WjaCB4JrnoLPQ6lglk="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"jPKRP9Gl\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"85RtbSeKPyetV3bqJz-1nw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=cs2zkCZXfQ4eL0rWTimsq5nOhtO7ioNIuOuQwptUZgQ=;\n\tb=JaNx2DMp53bzZNnxsYcWEVovuLxoeyogSXo/brJfArfOEcJaD6g9Ey+xzWKIxiKKd7\n\tcxXV57KQweth8wrSvXD10bYf3ngI+bDih1yjr8vjcp/7x9MWN4CTD2MpLAiElkBOKyRb\n\tAyQnn9TEAzRAdE/sYRZ7ImnXjBNXFIHfah72LesO+lUxVxbntg1RtFsxhMcGcH2cwoTt\n\tmD7ND8WUXKKW5UUyFzZXkFpvCwNUMAiaHPl6tvZaC8xHiK3nNujx3jzUAyIVgKdN/Umm\n\txs7R0wpUqv3tIoZ1BUZYkiZeKe62Z/myDBVgrmNfW8Y/0jrrnlhWWZvqCd5IUF9HA4LP\n\tXyqw==","X-Gm-Message-State":"AOAM532FexIhdQWXnGVzMDGh9gLpZ8uh6ubSHQOVS3xnJq1KIwJRmE9g\n\tM8FH8YBK4WuK/6iVWqXdQj6vc+PE1FreTzMDKFMYNWMcFBwSp3Q0ocgYWXibSa61Dfwiq3ThLUV\n\tu8wdtdcIUd9fqZJezpq/PbJnGcddsBOH4LMCmPcfTflj+gAkcxQ==","X-Received":["by 2002:a05:6214:27c8:b0:456:3800:7b6e with SMTP id\n\tge8-20020a05621427c800b0045638007b6emr14613005qvb.83.1653250105548; \n\tSun, 22 May 2022 13:08:25 -0700 (PDT)","by 2002:a05:6214:27c8:b0:456:3800:7b6e with SMTP id\n\tge8-20020a05621427c800b0045638007b6emr14612992qvb.83.1653250105220;\n\tSun, 22 May 2022 13:08:25 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJzo4c6LK+JWWl9LEjuOCqJbtisFjOxXIY6/j3+JsE6A75zwMRM9/ACKvacxdEXuaoHUDNEAsM0zqb37Fkv8iOE=","MIME-Version":"1.0","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-4-ecurtin@redhat.com>\n\t\n\t","In-Reply-To":"","Date":"Sun, 22 May 2022 21:08:09 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","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":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23135,"web_url":"https://patchwork.libcamera.org/comment/23135/","msgid":"","date":"2022-05-23T08:25:20","subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Sun, May 22, 2022 at 09:08:09PM +0100, Eric Curtin wrote:\n> On Sun, 22 May 2022 at 18:32, Laurent Pinchart wrote:\n> >\n> > Another small comment.\n> >\n> > On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:\n> > > > This adds more portability to existing cam sinks. You can pass a\n> > > > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > > > to the display. This allows cam reference implementation to display\n> > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > > > reference implementation, to run as a desktop application in Wayland or\n> > > > X11. SDL also has support for Android and ChromeOS which has not been\n> > > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer\n> > > > successfully where existing kms sink did not work. Can also be used as\n> > > > kmsdrm sink. Only supports one camera stream at present.\n> > > >\n> > > > Signed-off-by: Eric Curtin \n> > > > Reviewed-by: Laurent Pinchart \n> > > > Reviewed-by: Kieran Bingham \n> > > > Tested-by: Jacopo Mondi \n> > > > ---\n> > > > src/cam/camera_session.cpp | 8 ++\n> > > > src/cam/main.cpp | 4 +\n> > > > src/cam/main.h | 1 +\n> > > > src/cam/meson.build | 12 +++\n> > > > src/cam/sdl_sink.cpp | 194 +++++++++++++++++++++++++++++++++++\n> > > > src/cam/sdl_sink.h | 49 +++++++++\n> > > > src/cam/sdl_texture.cpp | 37 +++++++\n> > > > src/cam/sdl_texture.h | 29 ++++++\n> > > > src/cam/sdl_texture_yuyv.cpp | 20 ++++\n> > > > src/cam/sdl_texture_yuyv.h | 17 +++\n> > > > 10 files changed, 371 insertions(+)\n> > > > create mode 100644 src/cam/sdl_sink.cpp\n> > > > create mode 100644 src/cam/sdl_sink.h\n> > > > create mode 100644 src/cam/sdl_texture.cpp\n> > > > create mode 100644 src/cam/sdl_texture.h\n> > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > > > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > > >\n> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > > index efffafbf..336ae471 100644\n> > > > --- a/src/cam/camera_session.cpp\n> > > > +++ b/src/cam/camera_session.cpp\n> > > > @@ -20,6 +20,9 @@\n> > > > #include \"kms_sink.h\"\n> > > > #endif\n> > > > #include \"main.h\"\n> > > > +#ifdef HAVE_SDL\n> > > > +#include \"sdl_sink.h\"\n> > > > +#endif\n> > > > #include \"stream_options.h\"\n> > > >\n> > > > using namespace libcamera;\n> > > > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > > > #endif\n> > > >\n> > > > +#ifdef HAVE_SDL\n> > > > + if (options_.isSet(OptSDL))\n> > > > + sink_ = std::make_unique();\n> > > > +#endif\n> > > > +\n> > > > if (options_.isSet(OptFile)) {\n> > > > if (!options_[OptFile].toString().empty())\n> > > > sink_ = std::make_unique(streamNames_,\n> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > > index c7f664b9..962262a8 100644\n> > > > --- a/src/cam/main.cpp\n> > > > +++ b/src/cam/main.cpp\n> > > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > > \"The default file name is 'frame-#.bin'.\",\n> > > > \"file\", ArgumentOptional, \"filename\", false,\n> > > > OptCamera);\n> > > > +#ifdef HAVE_SDL\n> > > > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > > > +#endif\n> > > > parser.addOption(OptStream, &streamKeyValue,\n> > > > \"Set configuration of a camera stream\", \"stream\", true,\n> > > > OptCamera);\n> > > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > > index 62f7bbc9..507a184f 100644\n> > > > --- a/src/cam/main.h\n> > > > +++ b/src/cam/main.h\n> > > > @@ -17,6 +17,7 @@ enum {\n> > > > OptList = 'l',\n> > > > OptListProperties = 'p',\n> > > > OptMonitor = 'm',\n> > > > + OptSDL = 'S',\n> > > > OptStream = 's',\n> > > > OptListControls = 256,\n> > > > OptStrictFormats = 257,\n> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > index 5bab8c9e..7d714152 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -32,12 +32,24 @@ if libdrm.found()\n> > > > ])\n> > > > endif\n> > > >\n> > > > +libsdl2 = dependency('SDL2', required : false)\n> > > > +\n> > > > +if libsdl2.found()\n> > > > + cam_cpp_args += ['-DHAVE_SDL']\n> > > > + cam_sources += files([\n> > > > + 'sdl_sink.cpp',\n> > > > + 'sdl_texture.cpp',\n> > > > + 'sdl_texture_yuyv.cpp'\n> > > > + ])\n> > > > +endif\n> > > > +\n> > > > cam = executable('cam', cam_sources,\n> > > > dependencies : [\n> > > > libatomic,\n> > > > libcamera_public,\n> > > > libdrm,\n> > > > libevent,\n> > > > + libsdl2,\n> > > > ],\n> > > > cpp_args : cam_cpp_args,\n> > > > install : true)\n> > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > > new file mode 100644\n> > > > index 00000000..65430efb\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.cpp\n> > > > @@ -0,0 +1,194 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_sink.h - SDL Sink\n> > > > + */\n> > > > +\n> > > > +#include \"sdl_sink.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"event_loop.h\"\n> > > > +#include \"image.h\"\n> > > > +#include \"sdl_texture_yuyv.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +using namespace std::chrono_literals;\n> > > > +\n> > > > +SDLSink::SDLSink()\n> > > > + : window_(nullptr), renderer_(nullptr), rect_({}),\n> > > > + init_(false)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SDLSink::~SDLSink()\n> > > > +{\n> > > > + stop();\n> > > > +}\n> > > > +\n> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > > +{\n> > > > + const int ret = FrameSink::configure(config);\n> > >\n> > > No need to make ret const.\n> > >\n> > > > + if (ret < 0)\n> > > > + return ret;\n> > > > +\n> > > > + if (config.size() > 1) {\n> > > > + std::cerr\n> > > > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > > + << std::endl;\n> > > > + } else if (config.empty()) {\n> > > > + std::cerr << \"Require at least one camera stream to process\"\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > > + rect_.w = cfg.size.width;\n> > > > + rect_.h = cfg.size.height;\n> > > > +\n> > > > + switch (cfg.pixelFormat) {\n> > > > + case libcamera::formats::YUYV:\n> > > > + texture_ = std::make_unique(rect_);\n> > > > + break;\n> > > > + default:\n> > > > + std::cerr << \"Unsupported pixel format \"\n> > > > + << cfg.pixelFormat.toString() << std::endl;\n> > > > + return -EINVAL;\n> > > > + };\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int SDLSink::start()\n> > > > +{\n> > > > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > > > + if (ret) {\n> > > > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + init_ = true;\n> > > > + window_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > > + SDL_WINDOWPOS_UNDEFINED, rect_.w,\n> > > > + rect_.h,\n> > > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > > + if (!window_) {\n> > > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + renderer_ = SDL_CreateRenderer(window_, -1, 0);\n> > > > + if (!renderer_) {\n> > > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);\n> > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > > > + std::cerr << \"Failed to set SDL render logical size: \"\n> > > > + << SDL_GetError() << std::endl;\n> > > > + }\n> > >\n> > > /*\n> > > * Not critical to set, don't return in this case, set for scaling\n> > > * purposes.\n> > > */\n> > > if (ret)\n> > > std::cerr << \"Failed to set SDL render logical size: \"\n> > > << SDL_GetError() << std::endl;\n> > >\n> > > > +\n> > > > + ret = texture_->create(renderer_);\n> > > > + if (ret) {\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + /* \\todo Make the event cancellable to support stop/start cycles. */\n> > > > + EventLoop::instance()->addTimerEvent(\n> > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int SDLSink::stop()\n> > > > +{\n> > > > + if (texture_) {\n> > > > + texture_.reset();\n> > > > + }\n> > >\n> > > This can be written as\n> > >\n> > > texture_.reset();\n> > >\n> > > or\n> > >\n> > > texture_ = nullptr;\n> > >\n> > > > +\n> > > > + if (renderer_) {\n> > > > + SDL_DestroyRenderer(renderer_);\n> > > > + renderer_ = nullptr;\n> > > > + }\n> > > > +\n> > > > + if (window_) {\n> > > > + SDL_DestroyWindow(window_);\n> > > > + window_ = nullptr;\n> > > > + }\n> > > > +\n> > > > + if (init_) {\n> > > > + SDL_Quit();\n> > > > + init_ = false;\n> > > > + }\n> > > > +\n> > > > + return FrameSink::stop();\n> > > > +}\n> > > > +\n> > > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + std::unique_ptr image =\n> > > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > > > + assert(image != nullptr);\n> > > > +\n> > > > + mappedBuffers_[buffer] = std::move(image);\n> > > > +}\n> > > > +\n> > > > +bool SDLSink::processRequest(Request *request)\n> > > > +{\n> > > > + for (auto [stream, buffer] : request->buffers()) {\n> > > > + renderBuffer(buffer);\n> > > > + break; /* to be expanded to launch SDL window per buffer */\n> > > > + }\n> > > > +\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > +/*\n> > > > + * Process SDL events, required for things like window resize and quit button\n> > > > + */\n> > > > +void SDLSink::processSDLEvents()\n> > > > +{\n> > > > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > > > + if (e.type == SDL_QUIT) {\n> > > > + /* Click close icon then quit */\n> > > > + EventLoop::instance()->exit(0);\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + Image *image = mappedBuffers_[buffer].get();\n> > > > +\n> > > > + /* \\todo Implement support for multi-planar formats. */\n> > > > + const FrameMetadata::Plane &meta =\n> > > > + buffer->metadata().planes()[0];\n> > >\n> > > This holds on a single line.\n> > >\n> > > > +\n> > > > + Span data = image->data(0);\n> > > > + if (meta.bytesused > data.size())\n> > > > + std::cerr << \"payload size \" << meta.bytesused\n> > > > + << \" larger than plane size \" << data.size()\n> > > > + << std::endl;\n> > > > +\n> > > > + texture_->update(data);\n> > > > +\n> > > > + SDL_RenderClear(renderer_);\n> > > > + SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);\n> > > > + SDL_RenderPresent(renderer_);\n> > > > +}\n> > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > > new file mode 100644\n> > > > index 00000000..83171cca\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.h\n> > > > @@ -0,0 +1,49 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_sink.h - SDL Sink\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"frame_sink.h\"\n> > > > +\n> > > > +class Image;\n> > > > +class SDLTexture;\n> > > > +\n> > > > +class SDLSink : public FrameSink\n> > > > +{\n> > > > +public:\n> > > > + SDLSink();\n> > > > + ~SDLSink();\n> > > > +\n> > > > + int configure(const libcamera::CameraConfiguration &config) override;\n> > > > + int start() override;\n> > > > + int stop() override;\n> > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > +\n> > > > + bool processRequest(libcamera::Request *request) override;\n> > > > +\n> > > > +private:\n> > > > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > > > + void processSDLEvents();\n> > > > +\n> > > > + std::map>\n> > > > + mappedBuffers_;\n> > > > +\n> > > > + std::unique_ptr texture_;\n> > > > +\n> > > > + SDL_Window *window_;\n> > > > + SDL_Renderer *renderer_;\n> > > > + SDL_Rect rect_;\n> > > > + SDL_PixelFormatEnum pixelFormat_;\n> >\n> > This isn't used and can be dropped.\n> >\n> > > > + bool init_;\n> > > > +};\n> > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > > > new file mode 100644\n> > > > index 00000000..ac355a97\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture.cpp\n> > > > @@ -0,0 +1,37 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_texture.cpp - SDL Texture\n> > > > + */\n> > > > +\n> > > > +#include \"sdl_texture.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > > + const int pitch)\n> > > > + : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SDLTexture::~SDLTexture()\n> > > > +{\n> > > > + if (ptr_) {\n> > > > + SDL_DestroyTexture(ptr_);\n> > > > + }\n> > >\n> > > No need for curly braces.\n> > >\n> > > > +}\n> > > > +\n> > > > +int SDLTexture::create(SDL_Renderer *renderer_)\n> > > > +{\n> > > > + ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,\n> > > > + SDL_TEXTUREACCESS_STREAMING, rect_.w,\n> > > > + rect_.h);\n> > > > + if (!ptr_) {\n> > > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > > > + << std::endl;\n> > > > + return -ENOMEM;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > > > new file mode 100644\n> > > > index 00000000..b04eece0\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture.h\n> > > > @@ -0,0 +1,29 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_texture.h - SDL Texture\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"image.h\"\n> > > > +\n> > > > +class SDLTexture\n> > > > +{\n> > > > +public:\n> > > > + SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > > + const int pitch);\n> > > > + virtual ~SDLTexture();\n> > > > + int create(SDL_Renderer *renderer_);\n> > >\n> > > s/renderer_/renderer/\n> > >\n> > > > + virtual void update(const libcamera::Span &data) = 0;\n> > > > + SDL_Texture *get() const { return ptr_; }\n> > > > +\n> > > > +protected:\n> > > > + SDL_Texture *ptr_;\n> > > > + const SDL_Rect &rect_;\n> > >\n> > > This should be a copy, not a reference, as the caller shouldn't need to\n> > > guarantee that the rectangle passed to the constructor will not be\n> > > destroyed as long as the texture exists.\n> > >\n> > > These are small issues, I can fix them when applying.\n> > >\n> > > > + SDL_PixelFormatEnum pixelFormat_;\n> > > > + const int pitch_;\n> > > > +};\n> > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > > > new file mode 100644\n> > > > index 00000000..a219097b\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > > > @@ -0,0 +1,20 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV\n> > > > + */\n> > > > +\n> > > > +#include \"sdl_texture_yuyv.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +void SDLTextureYUYV::update(const Span &data)\n> > > > +{\n> > > > + SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);\n> > > > +}\n> > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > > > new file mode 100644\n> > > > index 00000000..38c51c74\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_texture_yuyv.h\n> > > > @@ -0,0 +1,17 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > + *\n> > > > + * sdl_texture_yuyv.h - SDL Texture YUYV\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \"sdl_texture.h\"\n> > > > +\n> > > > +class SDLTextureYUYV : public SDLTexture\n> > > > +{\n> > > > +public:\n> > > > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > > > + void update(const libcamera::Span &data) override;\n> > > > +};\n> \n> There is an extra \"SDL_PixelFormatEnum pixelFormat_;\" sorry, if you'd\n> like to make changes no problem on my end. The main thing for me is to\n> have this merged and working for YUYV and MJPG and build on it from\n> there.\n\nOK. Could you reply to the question about the race condition fix ?\nThat's the only outstanding issue, I can then merge the patches.","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 466C3BD161\n\tfor ;\n\tMon, 23 May 2022 08:25:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09D5165663;\n\tMon, 23 May 2022 10:25:28 +0200 (CEST)","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 71BC96565E\n\tfor ;\n\tMon, 23 May 2022 10:25:26 +0200 (CEST)","from pendragon.ideasonboard.com (ip-109-40-241-50.web.vodafone.de\n\t[109.40.241.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F124B45F;\n\tMon, 23 May 2022 10:25:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653294328;\n\tbh=N838FrVdmbKqSALKFz7IO1WmrVTusQdzq9wZjCwdZvA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0cVR92iR9QLbQ/hmsENjnZ5Ldp2TidEEQGccVtz6u0RaRAOs7RDbBB+y2laBSHQrN\n\tel6yBTPvOe5hVVFc3iB0fneSp8AOE5UEL67Urart3sNgqtkxhL+LfwhVMdY8NMGI0O\n\tpaGaQ4ihWt/uV2y2yHvemMv6XXRl+4Bz5a+52PnYxDOsLgLg9jkZJg3Bkkphyzjhec\n\trroP2DQpkS7xAfN9T0H3hythLRk1KEKlTNSX9m/PPHh7zXSIrv0pBl0jJrTBUuBHGp\n\tG7+EkQhyVsqq/NqD1zu6Cus7S7AixqXPKdJfAa1MfXqAX9YVJVbTSRQc3ATSI+3X4H\n\tU7c6ybkZd0gvQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653294326;\n\tbh=N838FrVdmbKqSALKFz7IO1WmrVTusQdzq9wZjCwdZvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aczgexy+if9jVqShWch1w0N4NJtZYu/Jy7YIA8cN0rlTiuO1S3/DeMBhgRcOm2xth\n\tml/XBovFNbCK5dARtlfClnDzN6eTHgpwqjRmS8BXeVvQO5Py1hkLwTHqLku/ZCd3mK\n\tLG55QyOim6wGb5LXEdIrpfoFreXkQ+Ti/cwcPtvY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aczgexy+\"; dkim-atps=neutral","Date":"Mon, 23 May 2022 11:25:20 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-4-ecurtin@redhat.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23137,"web_url":"https://patchwork.libcamera.org/comment/23137/","msgid":"","date":"2022-05-23T09:41:28","subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Mon, 23 May 2022 at 09:25, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Sun, May 22, 2022 at 09:08:09PM +0100, Eric Curtin wrote:\n> > On Sun, 22 May 2022 at 18:32, Laurent Pinchart wrote:\n> > >\n> > > Another small comment.\n> > >\n> > > On Sun, May 22, 2022 at 01:36:28PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > On Fri, May 20, 2022 at 08:01:05PM +0100, Eric Curtin wrote:\n> > > > > This adds more portability to existing cam sinks. You can pass a\n> > > > > YUYV camera buffer and SDL will handle the pixel buffer conversion\n> > > > > to the display. This allows cam reference implementation to display\n> > > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam\n> > > > > reference implementation, to run as a desktop application in Wayland or\n> > > > > X11. SDL also has support for Android and ChromeOS which has not been\n> > > > > tested. Also tested on simpledrm Raspberry Pi 4 framebuffer\n> > > > > successfully where existing kms sink did not work. Can also be used as\n> > > > > kmsdrm sink. Only supports one camera stream at present.\n> > > > >\n> > > > > Signed-off-by: Eric Curtin \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Reviewed-by: Kieran Bingham \n> > > > > Tested-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/cam/camera_session.cpp | 8 ++\n> > > > > src/cam/main.cpp | 4 +\n> > > > > src/cam/main.h | 1 +\n> > > > > src/cam/meson.build | 12 +++\n> > > > > src/cam/sdl_sink.cpp | 194 +++++++++++++++++++++++++++++++++++\n> > > > > src/cam/sdl_sink.h | 49 +++++++++\n> > > > > src/cam/sdl_texture.cpp | 37 +++++++\n> > > > > src/cam/sdl_texture.h | 29 ++++++\n> > > > > src/cam/sdl_texture_yuyv.cpp | 20 ++++\n> > > > > src/cam/sdl_texture_yuyv.h | 17 +++\n> > > > > 10 files changed, 371 insertions(+)\n> > > > > create mode 100644 src/cam/sdl_sink.cpp\n> > > > > create mode 100644 src/cam/sdl_sink.h\n> > > > > create mode 100644 src/cam/sdl_texture.cpp\n> > > > > create mode 100644 src/cam/sdl_texture.h\n> > > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp\n> > > > > create mode 100644 src/cam/sdl_texture_yuyv.h\n> > > > >\n> > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > > > index efffafbf..336ae471 100644\n> > > > > --- a/src/cam/camera_session.cpp\n> > > > > +++ b/src/cam/camera_session.cpp\n> > > > > @@ -20,6 +20,9 @@\n> > > > > #include \"kms_sink.h\"\n> > > > > #endif\n> > > > > #include \"main.h\"\n> > > > > +#ifdef HAVE_SDL\n> > > > > +#include \"sdl_sink.h\"\n> > > > > +#endif\n> > > > > #include \"stream_options.h\"\n> > > > >\n> > > > > using namespace libcamera;\n> > > > > @@ -186,6 +189,11 @@ int CameraSession::start()\n> > > > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> > > > > #endif\n> > > > >\n> > > > > +#ifdef HAVE_SDL\n> > > > > + if (options_.isSet(OptSDL))\n> > > > > + sink_ = std::make_unique();\n> > > > > +#endif\n> > > > > +\n> > > > > if (options_.isSet(OptFile)) {\n> > > > > if (!options_[OptFile].toString().empty())\n> > > > > sink_ = std::make_unique(streamNames_,\n> > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > > > index c7f664b9..962262a8 100644\n> > > > > --- a/src/cam/main.cpp\n> > > > > +++ b/src/cam/main.cpp\n> > > > > @@ -147,6 +147,10 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > > > \"The default file name is 'frame-#.bin'.\",\n> > > > > \"file\", ArgumentOptional, \"filename\", false,\n> > > > > OptCamera);\n> > > > > +#ifdef HAVE_SDL\n> > > > > + parser.addOption(OptSDL, OptionNone, \"Display viewfinder through SDL\",\n> > > > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> > > > > +#endif\n> > > > > parser.addOption(OptStream, &streamKeyValue,\n> > > > > \"Set configuration of a camera stream\", \"stream\", true,\n> > > > > OptCamera);\n> > > > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > > > index 62f7bbc9..507a184f 100644\n> > > > > --- a/src/cam/main.h\n> > > > > +++ b/src/cam/main.h\n> > > > > @@ -17,6 +17,7 @@ enum {\n> > > > > OptList = 'l',\n> > > > > OptListProperties = 'p',\n> > > > > OptMonitor = 'm',\n> > > > > + OptSDL = 'S',\n> > > > > OptStream = 's',\n> > > > > OptListControls = 256,\n> > > > > OptStrictFormats = 257,\n> > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > > index 5bab8c9e..7d714152 100644\n> > > > > --- a/src/cam/meson.build\n> > > > > +++ b/src/cam/meson.build\n> > > > > @@ -32,12 +32,24 @@ if libdrm.found()\n> > > > > ])\n> > > > > endif\n> > > > >\n> > > > > +libsdl2 = dependency('SDL2', required : false)\n> > > > > +\n> > > > > +if libsdl2.found()\n> > > > > + cam_cpp_args += ['-DHAVE_SDL']\n> > > > > + cam_sources += files([\n> > > > > + 'sdl_sink.cpp',\n> > > > > + 'sdl_texture.cpp',\n> > > > > + 'sdl_texture_yuyv.cpp'\n> > > > > + ])\n> > > > > +endif\n> > > > > +\n> > > > > cam = executable('cam', cam_sources,\n> > > > > dependencies : [\n> > > > > libatomic,\n> > > > > libcamera_public,\n> > > > > libdrm,\n> > > > > libevent,\n> > > > > + libsdl2,\n> > > > > ],\n> > > > > cpp_args : cam_cpp_args,\n> > > > > install : true)\n> > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..65430efb\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_sink.cpp\n> > > > > @@ -0,0 +1,194 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_sink.h - SDL Sink\n> > > > > + */\n> > > > > +\n> > > > > +#include \"sdl_sink.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"event_loop.h\"\n> > > > > +#include \"image.h\"\n> > > > > +#include \"sdl_texture_yuyv.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +using namespace std::chrono_literals;\n> > > > > +\n> > > > > +SDLSink::SDLSink()\n> > > > > + : window_(nullptr), renderer_(nullptr), rect_({}),\n> > > > > + init_(false)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +SDLSink::~SDLSink()\n> > > > > +{\n> > > > > + stop();\n> > > > > +}\n> > > > > +\n> > > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config)\n> > > > > +{\n> > > > > + const int ret = FrameSink::configure(config);\n> > > >\n> > > > No need to make ret const.\n> > > >\n> > > > > + if (ret < 0)\n> > > > > + return ret;\n> > > > > +\n> > > > > + if (config.size() > 1) {\n> > > > > + std::cerr\n> > > > > + << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > > > + << std::endl;\n> > > > > + } else if (config.empty()) {\n> > > > > + std::cerr << \"Require at least one camera stream to process\"\n> > > > > + << std::endl;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + const libcamera::StreamConfiguration &cfg = config.at(0);\n> > > > > + rect_.w = cfg.size.width;\n> > > > > + rect_.h = cfg.size.height;\n> > > > > +\n> > > > > + switch (cfg.pixelFormat) {\n> > > > > + case libcamera::formats::YUYV:\n> > > > > + texture_ = std::make_unique(rect_);\n> > > > > + break;\n> > > > > + default:\n> > > > > + std::cerr << \"Unsupported pixel format \"\n> > > > > + << cfg.pixelFormat.toString() << std::endl;\n> > > > > + return -EINVAL;\n> > > > > + };\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int SDLSink::start()\n> > > > > +{\n> > > > > + int ret = SDL_Init(SDL_INIT_VIDEO);\n> > > > > + if (ret) {\n> > > > > + std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > > > + << std::endl;\n> > > > > + return ret;\n> > > > > + }\n> > > > > +\n> > > > > + init_ = true;\n> > > > > + window_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > > > + SDL_WINDOWPOS_UNDEFINED, rect_.w,\n> > > > > + rect_.h,\n> > > > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > > > + if (!window_) {\n> > > > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError()\n> > > > > + << std::endl;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + renderer_ = SDL_CreateRenderer(window_, -1, 0);\n> > > > > + if (!renderer_) {\n> > > > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError()\n> > > > > + << std::endl;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + ret = SDL_RenderSetLogicalSize(renderer_, rect_.w, rect_.h);\n> > > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> > > > > + std::cerr << \"Failed to set SDL render logical size: \"\n> > > > > + << SDL_GetError() << std::endl;\n> > > > > + }\n> > > >\n> > > > /*\n> > > > * Not critical to set, don't return in this case, set for scaling\n> > > > * purposes.\n> > > > */\n> > > > if (ret)\n> > > > std::cerr << \"Failed to set SDL render logical size: \"\n> > > > << SDL_GetError() << std::endl;\n> > > >\n> > > > > +\n> > > > > + ret = texture_->create(renderer_);\n> > > > > + if (ret) {\n> > > > > + return ret;\n> > > > > + }\n> > > > > +\n> > > > > + /* \\todo Make the event cancellable to support stop/start cycles. */\n> > > > > + EventLoop::instance()->addTimerEvent(\n> > > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this));\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int SDLSink::stop()\n> > > > > +{\n> > > > > + if (texture_) {\n> > > > > + texture_.reset();\n> > > > > + }\n> > > >\n> > > > This can be written as\n> > > >\n> > > > texture_.reset();\n> > > >\n> > > > or\n> > > >\n> > > > texture_ = nullptr;\n> > > >\n> > > > > +\n> > > > > + if (renderer_) {\n> > > > > + SDL_DestroyRenderer(renderer_);\n> > > > > + renderer_ = nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + if (window_) {\n> > > > > + SDL_DestroyWindow(window_);\n> > > > > + window_ = nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + if (init_) {\n> > > > > + SDL_Quit();\n> > > > > + init_ = false;\n> > > > > + }\n> > > > > +\n> > > > > + return FrameSink::stop();\n> > > > > +}\n> > > > > +\n> > > > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > > > +{\n> > > > > + std::unique_ptr image =\n> > > > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);\n> > > > > + assert(image != nullptr);\n> > > > > +\n> > > > > + mappedBuffers_[buffer] = std::move(image);\n> > > > > +}\n> > > > > +\n> > > > > +bool SDLSink::processRequest(Request *request)\n> > > > > +{\n> > > > > + for (auto [stream, buffer] : request->buffers()) {\n> > > > > + renderBuffer(buffer);\n> > > > > + break; /* to be expanded to launch SDL window per buffer */\n> > > > > + }\n> > > > > +\n> > > > > + return true;\n> > > > > +}\n> > > > > +\n> > > > > +/*\n> > > > > + * Process SDL events, required for things like window resize and quit button\n> > > > > + */\n> > > > > +void SDLSink::processSDLEvents()\n> > > > > +{\n> > > > > + for (SDL_Event e; SDL_PollEvent(&e);) {\n> > > > > + if (e.type == SDL_QUIT) {\n> > > > > + /* Click close icon then quit */\n> > > > > + EventLoop::instance()->exit(0);\n> > > > > + }\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > > > +{\n> > > > > + Image *image = mappedBuffers_[buffer].get();\n> > > > > +\n> > > > > + /* \\todo Implement support for multi-planar formats. */\n> > > > > + const FrameMetadata::Plane &meta =\n> > > > > + buffer->metadata().planes()[0];\n> > > >\n> > > > This holds on a single line.\n> > > >\n> > > > > +\n> > > > > + Span data = image->data(0);\n> > > > > + if (meta.bytesused > data.size())\n> > > > > + std::cerr << \"payload size \" << meta.bytesused\n> > > > > + << \" larger than plane size \" << data.size()\n> > > > > + << std::endl;\n> > > > > +\n> > > > > + texture_->update(data);\n> > > > > +\n> > > > > + SDL_RenderClear(renderer_);\n> > > > > + SDL_RenderCopy(renderer_, texture_->get(), nullptr, nullptr);\n> > > > > + SDL_RenderPresent(renderer_);\n> > > > > +}\n> > > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > > > new file mode 100644\n> > > > > index 00000000..83171cca\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_sink.h\n> > > > > @@ -0,0 +1,49 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_sink.h - SDL Sink\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"frame_sink.h\"\n> > > > > +\n> > > > > +class Image;\n> > > > > +class SDLTexture;\n> > > > > +\n> > > > > +class SDLSink : public FrameSink\n> > > > > +{\n> > > > > +public:\n> > > > > + SDLSink();\n> > > > > + ~SDLSink();\n> > > > > +\n> > > > > + int configure(const libcamera::CameraConfiguration &config) override;\n> > > > > + int start() override;\n> > > > > + int stop() override;\n> > > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > > +\n> > > > > + bool processRequest(libcamera::Request *request) override;\n> > > > > +\n> > > > > +private:\n> > > > > + void renderBuffer(libcamera::FrameBuffer *buffer);\n> > > > > + void processSDLEvents();\n> > > > > +\n> > > > > + std::map>\n> > > > > + mappedBuffers_;\n> > > > > +\n> > > > > + std::unique_ptr texture_;\n> > > > > +\n> > > > > + SDL_Window *window_;\n> > > > > + SDL_Renderer *renderer_;\n> > > > > + SDL_Rect rect_;\n> > > > > + SDL_PixelFormatEnum pixelFormat_;\n> > >\n> > > This isn't used and can be dropped.\n> > >\n> > > > > + bool init_;\n> > > > > +};\n> > > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..ac355a97\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_texture.cpp\n> > > > > @@ -0,0 +1,37 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_texture.cpp - SDL Texture\n> > > > > + */\n> > > > > +\n> > > > > +#include \"sdl_texture.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > > > + const int pitch)\n> > > > > + : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +SDLTexture::~SDLTexture()\n> > > > > +{\n> > > > > + if (ptr_) {\n> > > > > + SDL_DestroyTexture(ptr_);\n> > > > > + }\n> > > >\n> > > > No need for curly braces.\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +int SDLTexture::create(SDL_Renderer *renderer_)\n> > > > > +{\n> > > > > + ptr_ = SDL_CreateTexture(renderer_, pixelFormat_,\n> > > > > + SDL_TEXTUREACCESS_STREAMING, rect_.w,\n> > > > > + rect_.h);\n> > > > > + if (!ptr_) {\n> > > > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError()\n> > > > > + << std::endl;\n> > > > > + return -ENOMEM;\n> > > > > + }\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h\n> > > > > new file mode 100644\n> > > > > index 00000000..b04eece0\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_texture.h\n> > > > > @@ -0,0 +1,29 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_texture.h - SDL Texture\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"image.h\"\n> > > > > +\n> > > > > +class SDLTexture\n> > > > > +{\n> > > > > +public:\n> > > > > + SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,\n> > > > > + const int pitch);\n> > > > > + virtual ~SDLTexture();\n> > > > > + int create(SDL_Renderer *renderer_);\n> > > >\n> > > > s/renderer_/renderer/\n> > > >\n> > > > > + virtual void update(const libcamera::Span &data) = 0;\n> > > > > + SDL_Texture *get() const { return ptr_; }\n> > > > > +\n> > > > > +protected:\n> > > > > + SDL_Texture *ptr_;\n> > > > > + const SDL_Rect &rect_;\n> > > >\n> > > > This should be a copy, not a reference, as the caller shouldn't need to\n> > > > guarantee that the rectangle passed to the constructor will not be\n> > > > destroyed as long as the texture exists.\n> > > >\n> > > > These are small issues, I can fix them when applying.\n> > > >\n> > > > > + SDL_PixelFormatEnum pixelFormat_;\n> > > > > + const int pitch_;\n> > > > > +};\n> > > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..a219097b\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_texture_yuyv.cpp\n> > > > > @@ -0,0 +1,20 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_texture_yuyv.cpp - SDL Texture YUYV\n> > > > > + */\n> > > > > +\n> > > > > +#include \"sdl_texture_yuyv.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect)\n> > > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2))\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +void SDLTextureYUYV::update(const Span &data)\n> > > > > +{\n> > > > > + SDL_UpdateTexture(ptr_, &rect_, data.data(), pitch_);\n> > > > > +}\n> > > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h\n> > > > > new file mode 100644\n> > > > > index 00000000..38c51c74\n> > > > > --- /dev/null\n> > > > > +++ b/src/cam/sdl_texture_yuyv.h\n> > > > > @@ -0,0 +1,17 @@\n> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Ideas on Board Oy\n> > > > > + *\n> > > > > + * sdl_texture_yuyv.h - SDL Texture YUYV\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \"sdl_texture.h\"\n> > > > > +\n> > > > > +class SDLTextureYUYV : public SDLTexture\n> > > > > +{\n> > > > > +public:\n> > > > > + SDLTextureYUYV(const SDL_Rect &sdlRect);\n> > > > > + void update(const libcamera::Span &data) override;\n> > > > > +};\n> >\n> > There is an extra \"SDL_PixelFormatEnum pixelFormat_;\" sorry, if you'd\n> > like to make changes no problem on my end. The main thing for me is to\n> > have this merged and working for YUYV and MJPG and build on it from\n> > there.\n>\n> OK. Could you reply to the question about the race condition fix ?\n> That's the only outstanding issue, I can then merge the patches.\n\nThere was no identified race condition, you could remove that.\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 B4E67BD160\n\tfor ;\n\tMon, 23 May 2022 09:41:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CCFD65666;\n\tMon, 23 May 2022 11:41:49 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9944C6565E\n\tfor ;\n\tMon, 23 May 2022 11:41:47 +0200 (CEST)","from mail-qt1-f200.google.com (mail-qt1-f200.google.com\n\t[209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-28-lLsi76MiNrCyxbczG9M_HA-1; Mon, 23 May 2022 05:41:45 -0400","by mail-qt1-f200.google.com with SMTP id\n\tm6-20020ac866c6000000b002f52f9fb4edso11118342qtp.19\n\tfor ;\n\tMon, 23 May 2022 02:41:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653298909;\n\tbh=nhYgAlOALIG3a6ApcuIuWzxh+q7p38qhAWO/1Sca+24=;\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=xacoCiAcPexg1n3lmdITa7puO8aeSwGm8XRtIOoPuEh7Th1v3MjQHQiZlj5iBdd00\n\twJhycjo1qgDPeOAB8n+v1ZM2UX/X6GtEt8AYit/8TDkdTchXV6Z8Qis6esy2glmOt8\n\tEprfcSh7tGnQL/jhFLFWfOYxm9cVlTTiD7jt/2v9a68LVk2gpXB+IGnHFc8LZcwGJK\n\tp/L+bHAtm6GMeS513956Is40z3fFiN1uZiGa+oGNftb3BA3A++JlvLg/f2j1x0+SEL\n\tdQ+f4r1LOUp0TGQKL9l2OmUSZIiZihlEbuWo7OCh7FB5+OM9qmFPTDpV8UXOFy4OTC\n\tHadr9m0N3/36w==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1653298906;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=uyqQFB8A6YaZ+4RysL2MmfppCN5vggpJje2LpBUaKAc=;\n\tb=QtfDcdD4XX5y5Y4Gdz288YKSQx3hwvBvE1d0YQZO8ZQf+oCtb1q5YD/rQA3q0rpNT5MTix\n\tcEU3dekdb16KUIG5ATNNNFmmiJRvjY7O0c37hybcqB12dCRMNKTtgM78Bt62fU68T723xt\n\tBtOAhMy15259/JedRWiH22UnEOE+/gg="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"QtfDcdD4\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"lLsi76MiNrCyxbczG9M_HA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=uyqQFB8A6YaZ+4RysL2MmfppCN5vggpJje2LpBUaKAc=;\n\tb=C1k3XXTOn6Gw8EdS2B7RRFYmbal7KOcksBybBrOm7fEchMyBFHuzs7CayjF8kOUdhQ\n\tcSPMhvSdSsBO6vUTmNp1y3Cp3VIzFabJHtLKBabtUisIB1yZcs19MgbKipiIPMRgtpTJ\n\ta4zBNuRuf8PfO3sdNL7ijbloqsOtbep60pJII8Oc1dr4UDrrwto6m4JDc301rhrJ3NHd\n\tt6EzaS2ort2X6snDu+iIL4M0CO/INSukbzfy2qcNG3gwz/sG8eAljBN6kvvziMkviqIL\n\tJ5mr/1KHPvteovtLDqIDhsT0KUJHhi7+Co1Zb+k98gV1VWfTiW+U52EIQdEm4TWhuMDh\n\tY3FQ==","X-Gm-Message-State":"AOAM531BxAcl9M/xGFVgc/br5HvOGAQ1UFV8NszaF/Ms/vs6Q8MokgBL\n\tz1xc7cJnM5KPezd45nhLIPV4mKkN130Z4VKltwS3L1fkyHamN2tDhZFVWXla0pfRIJzsBTok90e\n\ttDT0L8itOiDoNsMCtDW145IiNayVy4z0IwVEs+V29g9Z4QS7Yzg==","X-Received":["by 2002:a05:620a:1a87:b0:6a3:76d1:19c7 with SMTP id\n\tbl7-20020a05620a1a8700b006a376d119c7mr3632116qkb.532.1653298904487; \n\tMon, 23 May 2022 02:41:44 -0700 (PDT)","by 2002:a05:620a:1a87:b0:6a3:76d1:19c7 with SMTP id\n\tbl7-20020a05620a1a8700b006a376d119c7mr3632113qkb.532.1653298904120;\n\tMon, 23 May 2022 02:41:44 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJwYwgEycFByi+zKuLqNK7DgblWZgpoWM5sSNOAenp0+ZXrvXIH2xkljxegM9Ln6F2aiCgAKpl9CpXIGNXcqhGA=","MIME-Version":"1.0","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-4-ecurtin@redhat.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 23 May 2022 10:41:28 +0100","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v9 3/4] cam: sdl_sink: Add SDL sink\n\twith initial YUYV support","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":"Eric Curtin via libcamera-devel ","Reply-To":"Eric Curtin ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]