[{"id":22729,"web_url":"https://patchwork.libcamera.org/comment/22729/","msgid":"","date":"2022-04-15T17:13:11","subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote:\n> This adds more portability to existing cam sinks. You can pass a\n> YUYV camera buffer for example and SDL will handle the pixel buffer\n> conversion, although SDL does not support decompression for pixelformats\n> like MJPEG. This allows cam reference implementation to display images\n> on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference\n> implementation, to run as a desktop application in wayland or x11.\n> SDL also has support for Android and ChromeOS which has not been tested.\n> Also tested on simpledrm raspberry pi 4 framebuffer successfully where\n> existing kms sink did not work. Can also be used as kmsdrm sink. Only\n> supports one camera stream at present.\n> \n> Signed-off-by: Eric Curtin \n> ---\n> src/cam/camera_session.cpp | 8 ++\n> src/cam/main.cpp | 5 +\n> src/cam/main.h | 1 +\n> src/cam/meson.build | 10 ++\n> src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++\n> src/cam/sdl_sink.h | 46 +++++++++\n> 6 files changed, 268 insertions(+)\n> create mode 100644 src/cam/sdl_sink.cpp\n> create mode 100644 src/cam/sdl_sink.h\n> \n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 0428b538..30162dbd 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -19,6 +19,9 @@\n> #ifdef HAVE_KMS\n> #include \"kms_sink.h\"\n> #endif\n> +#ifdef HAVE_SDL\n> +#include \"sdl_sink.h\"\n> +#endif\n> #include \"main.h\"\n> #include \"stream_options.h\"\n> \n> @@ -187,6 +190,11 @@ int CameraSession::start()\n> \t\tsink_ = std::make_unique(options_[OptDisplay].toString());\n\nSpace before tab. There are several occurrences.\n\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..1d62a64a 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \t\t\t \"Display viewfinder through DRM/KMS on specified connector\",\n> \t\t\t \"display\", ArgumentOptional, \"connector\", false,\n> \t\t\t OptCamera);\n> +#endif\n> +#ifdef HAVE_SDL\n> +\tparser.addOption(OptSDL, OptionNone,\n> +\t\t\t \"Display viewfinder through SDL\",\n> +\t\t\t \"sdl\", ArgumentNone, \"\", false, OptCamera);\n\nI'm tempted to refactor the -D option to specify the backend as an\nargument, given that the KMS and SDL sinks are mutually exclusive. I\nthink this can be done later though.\n\n> #endif\n> \tparser.addOption(OptFile, OptionString,\n> \t\t\t \"Write captured frames to disk\\n\"\n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index 62f7bbc9..2b285808 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -18,6 +18,7 @@ enum {\n> \tOptListProperties = 'p',\n> \tOptMonitor = 'm',\n> \tOptStream = 's',\n> +\tOptSDL = 'S',\n> \tOptListControls = 256,\n> \tOptStrictFormats = 257,\n> \tOptMetadata = 258,\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 5bab8c9e..59787741 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -32,11 +32,21 @@ 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> + ])\n> +endif\n> +\n> cam = executable('cam', cam_sources,\n> dependencies : [\n> libatomic,\n> libcamera_public,\n> libdrm,\n> + libsdl2,\n> libevent,\n> ],\n> cpp_args : cam_cpp_args,\n> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> new file mode 100644\n> index 00000000..03511974\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -0,0 +1,198 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * sdl_sink.cpp - 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> +\n> +using namespace libcamera;\n> +\n> +SDLSink::SDLSink()\n> +\t: sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false)\n\nLine wrap please. Same in quite a few locations below.\n\n> +{\n> +}\n> +\n> +SDLSink::~SDLSink()\n> +{\n> +\tstop();\n> +}\n> +\n> +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)\n> +{\n> +\tint ret = FrameSink::configure(cfg);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tif (cfg.size() > 1) {\n> +\t\tstd::cerr << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> +\t\t\t << std::endl;\n> +\t} else if (cfg.empty()) {\n> +\t\tstd::cerr << \"Require at least one camera stream to process\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst libcamera::StreamConfiguration &sCfg = cfg.at(0);\n\nYou could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match\nKMSSink::configure().\n\n> +\tsdlRect_.w = sCfg.size.width;\n> +\tsdlRect_.h = sCfg.size.height;\n\nPlease add a blank line here.\n\n> +\tswitch (sCfg.pixelFormat) {\n> +\tcase libcamera::formats::YUYV:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_YUY2;\n> +\t\tpitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> +\t\tbreak;\n> +\n> +\t/* From here down the fourcc values are identical between SDL, drm, libcamera */\n\nThey're always identical between DRM and libcamera, that's by design :-)\nYou can write\n\n\t/* From here down the fourcc values are identical between SDL and libcamera */\n\nor simply drop the message, as I don't think it's really relevant. In\nthat case I'd move the YVYU and UYVY formats just fter YUYV as they are\nrelated.\n\n> +\tcase libcamera::formats::NV21:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_NV21;\n> +\t\tpitch_ = sdlRect_.w;\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV12:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_NV12;\n> +\t\tpitch_ = sdlRect_.w;\n> +\t\tbreak;\n> +\tcase libcamera::formats::YVU420:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_YV12;\n> +\t\tpitch_ = sdlRect_.w;\n> +\t\tbreak;\n> +\tcase libcamera::formats::YVYU:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_YVYU;\n> +\t\tpitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> +\t\tbreak;\n> +\tcase libcamera::formats::UYVY:\n> +\t\tpixelFormat_ = SDL_PIXELFORMAT_UYVY;\n> +\t\tpitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> +\t\tbreak;\n\nFhr the same of completeness, does SDL have VYUY support ? NV16 and\nNV61 would also be nice if they are supported. If not that's fine.\n\n> +\tdefault:\n> +\t\tstd::cerr << sCfg.pixelFormat.toString() << \" not present in libcamera <-> SDL map\"\n> +\t\t\t << std::endl;\n\n\t\tstd::cerr << \"Unsupported pixel format \"\n\t\t\t << sCfg.pixelFormat.toString() << std::endl;\n\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() << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tsdlInit_ = true;\n> +\tsdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> +\tif (!sdlWindow_) {\n> +\t\tstd::cerr << \"Failed to create SDL window: \" << SDL_GetError() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tsdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> +\tif (!sdlRenderer_) {\n> +\t\tstd::cerr << \"Failed to create SDL renderer: \" << SDL_GetError() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> +\tif (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n\nPlease move the comment to the next line.\n\n> +\t\tstd::cerr << \"Failed to set SDL render logical size: \" << SDL_GetError() << std::endl;\n> +\t}\n> +\n> +\tsdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h);\n> +\tif (!sdlTexture_) {\n> +\t\tstd::cerr << \"Failed to create SDL texture: \" << SDL_GetError() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tEventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this));\n\nPlease use std::chrono types for the addTimerEvent duration parameter.\n\nUnless I'm mistaken, the event will keep being triggered indefinitely, which means\nthat SDLSink::processSDLEvents() could call SDL_PollEvent() even after\nstop() calls SDL_Quit(). That seems dangerous to me, I think we need a\nway to cancel periodic timer events.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SDLSink::stop()\n> +{\n> +\tif (sdlTexture_) {\n> +\t\tSDL_DestroyTexture(sdlTexture_);\n> +\t\tsdlTexture_ = nullptr;\n> +\t}\n> +\n> +\tif (sdlRenderer_) {\n> +\t\tSDL_DestroyRenderer(sdlRenderer_);\n> +\t\tsdlRenderer_ = nullptr;\n> +\t}\n> +\n> +\tif (sdlWindow_) {\n> +\t\tSDL_DestroyWindow(sdlWindow_);\n> +\t\tsdlWindow_ = nullptr;\n> +\t}\n> +\n> +\tif (sdlInit_) {\n> +\t\tSDL_Quit();\n> +\t\tsdlInit_ = false;\n> +\t}\n> +\n> +\treturn FrameSink::stop();\n> +}\n> +\n> +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> +{\n> +\tstd::unique_ptr image = Image::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) { /* click close icon then quit */\n\nPlease move the comment to the next line, and s/click/Click/\n\nActually, maybe a switch/case would be better, to prepare for adding\nsupport for more events ? Up to you.\n\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> +\tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> +\t\tconst FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> +\n> +\t\tSpan data = image->data(i);\n> +\t\tif (meta.bytesused > data.size())\n> +\t\t\tstd::cerr << \"payload size \" << meta.bytesused\n> +\t\t\t\t << \" larger than plane size \" << data.size()\n> +\t\t\t\t << std::endl;\n> +\n> +\t\tSDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_);\n\nAccording to https://wiki.libsdl.org/SDL_UpdateTexture,\n\n\"This is a fairly slow function, intended for use with static textures\nthat do not change often.\n\nIf the texture is intended to be updated often, it is preferred to\ncreate the texture as streaming and use the locking functions referenced\nbelow. While this function will work with streaming textures, for\noptimization reasons you may not get the pixels back if you lock the\ntexture afterward.\"\n\nHave you considered this ? I don't know what difference it makes when\nupdating the full texture though, maybe the added complexity isn't worth\nit.\n\n> +\t\tSDL_RenderClear(sdlRenderer_);\n> +\t\tSDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr);\n> +\t\tSDL_RenderPresent(sdlRenderer_);\n\nDoes this really have to be repeated for each plane ? That sounds\nstrange.\n\n> +\t}\n> +}\n> diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> new file mode 100644\n> index 00000000..c9b0ab8e\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * sdl_sink.h - SDL Sink\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"frame_sink.h\"\n> +\n> +class Image;\n> +\n> +class SDLSink : public FrameSink\n> +{\n> +public:\n> +\tSDLSink();\n> +\t~SDLSink();\n> +\n> +\tint configure(const libcamera::CameraConfiguration &cfg) 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> mappedBuffers_;\n> +\n> +\tSDL_Window *sdlWindow_;\n> +\tSDL_Renderer *sdlRenderer_;\n> +\tSDL_Texture *sdlTexture_;\n> +\tSDL_Rect sdlRect_;\n> +\tSDL_PixelFormatEnum pixelFormat_;\n> +\tbool sdlInit_;\n> +\tint pitch_;\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 E0CD2C0F1B\n\tfor ;\n\tFri, 15 Apr 2022 17:13:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 343C0604B3;\n\tFri, 15 Apr 2022 19:13:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB23660206\n\tfor ;\n\tFri, 15 Apr 2022 19:13:15 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-64-137-nat.elisa-mobile.fi\n\t[85.76.64.137])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 662B4482;\n\tFri, 15 Apr 2022 19:13:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650042797;\n\tbh=VrQySnl0TozJIl9YZBVXrubMathtactHs9xrVBVJLrE=;\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=bfdRLQg3zVuUd8foPe6jnz2ZcvfVsquwIGl5gkYFszKNYt8cgQaFfdCxiapnNwnHo\n\tR6i3Fq+9Dm1ucLK6Kae621IDgMQ9BI/jct3SQd1v6Xz7q5ziQP1XGWd2MBiZ8Xbhk1\n\tiTmFNZU6ds4+d6uqOoYQEbvuqS/h3l9nEqmpjBMuNgD0733s+G6uwcJ5xKIWZWFxjV\n\tr6/1rhr1HsJGlFzO+AkXbM2IXQiiylL22TmiH//YRfR2FNc6MdVx5erMlA4Lw4yja0\n\tZTcyuI70B6XDJaOzBXoIjYWqh06SC/kfehNloo2HthjRFWadILUQoZgMLSqnOt3RkY\n\tBAX9LtM0Fvfzg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650042795;\n\tbh=VrQySnl0TozJIl9YZBVXrubMathtactHs9xrVBVJLrE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nd5vALkc1tzq6IcS55I0uMW74tbIfvGSs8T5L19/BVifUqNEiw7W2KWZstbVkoHza\n\tGOIkONUIdvtf69/wxUOEiQN/yt0DRH6HsdjaJeYFvylxbLA2jsGdluj8Y5MJZbHQLk\n\tinmnXM96FBDMofLxmM0dqB5JGTeQ0yTMy0kHmiO0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Nd5vALkc\"; dkim-atps=neutral","Date":"Fri, 15 Apr 2022 20:13:11 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220408160015.33612-1-ecurtin@redhat.com>\n\t<20220408160015.33612-2-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220408160015.33612-2-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","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":22740,"web_url":"https://patchwork.libcamera.org/comment/22740/","msgid":"","date":"2022-04-19T14:47:17","subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","submitter":{"id":101,"url":"https://patchwork.libcamera.org/api/people/101/","name":"Eric Curtin","email":"ecurtin@redhat.com"},"content":"On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote:\n> > This adds more portability to existing cam sinks. You can pass a\n> > YUYV camera buffer for example and SDL will handle the pixel buffer\n> > conversion, although SDL does not support decompression for pixelformats\n> > like MJPEG. This allows cam reference implementation to display images\n> > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference\n> > implementation, to run as a desktop application in wayland or x11.\n> > SDL also has support for Android and ChromeOS which has not been tested.\n> > Also tested on simpledrm raspberry pi 4 framebuffer successfully where\n> > existing kms sink did not work. Can also be used as kmsdrm sink. Only\n> > supports one camera stream at present.\n> >\n> > Signed-off-by: Eric Curtin \n> > ---\n> > src/cam/camera_session.cpp | 8 ++\n> > src/cam/main.cpp | 5 +\n> > src/cam/main.h | 1 +\n> > src/cam/meson.build | 10 ++\n> > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++\n> > src/cam/sdl_sink.h | 46 +++++++++\n> > 6 files changed, 268 insertions(+)\n> > create mode 100644 src/cam/sdl_sink.cpp\n> > create mode 100644 src/cam/sdl_sink.h\n> >\n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 0428b538..30162dbd 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -19,6 +19,9 @@\n> > #ifdef HAVE_KMS\n> > #include \"kms_sink.h\"\n> > #endif\n> > +#ifdef HAVE_SDL\n> > +#include \"sdl_sink.h\"\n> > +#endif\n> > #include \"main.h\"\n> > #include \"stream_options.h\"\n> >\n> > @@ -187,6 +190,11 @@ int CameraSession::start()\n> > sink_ = std::make_unique(options_[OptDisplay].toString());\n>\n> Space before tab. There are several occurrences.\n>\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..1d62a64a 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \"Display viewfinder through DRM/KMS on specified connector\",\n> > \"display\", ArgumentOptional, \"connector\", false,\n> > OptCamera);\n> > +#endif\n> > +#ifdef HAVE_SDL\n> > + parser.addOption(OptSDL, OptionNone,\n> > + \"Display viewfinder through SDL\",\n> > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n>\n> I'm tempted to refactor the -D option to specify the backend as an\n> argument, given that the KMS and SDL sinks are mutually exclusive. I\n> think this can be done later though.\n>\n> > #endif\n> > parser.addOption(OptFile, OptionString,\n> > \"Write captured frames to disk\\n\"\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index 62f7bbc9..2b285808 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -18,6 +18,7 @@ enum {\n> > OptListProperties = 'p',\n> > OptMonitor = 'm',\n> > OptStream = 's',\n> > + OptSDL = 'S',\n> > OptListControls = 256,\n> > OptStrictFormats = 257,\n> > OptMetadata = 258,\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 5bab8c9e..59787741 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -32,11 +32,21 @@ 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> > + ])\n> > +endif\n> > +\n> > cam = executable('cam', cam_sources,\n> > dependencies : [\n> > libatomic,\n> > libcamera_public,\n> > libdrm,\n> > + libsdl2,\n> > libevent,\n> > ],\n> > cpp_args : cam_cpp_args,\n> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > new file mode 100644\n> > index 00000000..03511974\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -0,0 +1,198 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * sdl_sink.cpp - 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> > +\n> > +using namespace libcamera;\n> > +\n> > +SDLSink::SDLSink()\n> > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false)\n>\n> Line wrap please. Same in quite a few locations below.\n\nOk.\n\n>\n> > +{\n> > +}\n> > +\n> > +SDLSink::~SDLSink()\n> > +{\n> > + stop();\n> > +}\n> > +\n> > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)\n> > +{\n> > + int ret = FrameSink::configure(cfg);\n> > + if (ret < 0)\n> > + return ret;\n> > +\n> > + if (cfg.size() > 1) {\n> > + std::cerr << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > + << std::endl;\n> > + } else if (cfg.empty()) {\n> > + std::cerr << \"Require at least one camera stream to process\" << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n>\n> You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match\n> KMSSink::configure().\n\nOk.\n\n>\n> > + sdlRect_.w = sCfg.size.width;\n> > + sdlRect_.h = sCfg.size.height;\n>\n> Please add a blank line here.\n\nOk.\n\n>\n> > + switch (sCfg.pixelFormat) {\n> > + case libcamera::formats::YUYV:\n> > + pixelFormat_ = SDL_PIXELFORMAT_YUY2;\n> > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > + break;\n> > +\n> > + /* From here down the fourcc values are identical between SDL, drm, libcamera */\n>\n> They're always identical between DRM and libcamera, that's by design :-)\n> You can write\n>\n> /* From here down the fourcc values are identical between SDL and libcamera */\n>\n> or simply drop the message, as I don't think it's really relevant. In\n> that case I'd move the YVYU and UYVY formats just fter YUYV as they are\n> related.\n\nOk.\n\n>\n> > + case libcamera::formats::NV21:\n> > + pixelFormat_ = SDL_PIXELFORMAT_NV21;\n> > + pitch_ = sdlRect_.w;\n> > + break;\n> > + case libcamera::formats::NV12:\n> > + pixelFormat_ = SDL_PIXELFORMAT_NV12;\n> > + pitch_ = sdlRect_.w;\n> > + break;\n> > + case libcamera::formats::YVU420:\n> > + pixelFormat_ = SDL_PIXELFORMAT_YV12;\n> > + pitch_ = sdlRect_.w;\n> > + break;\n> > + case libcamera::formats::YVYU:\n> > + pixelFormat_ = SDL_PIXELFORMAT_YVYU;\n> > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > + break;\n> > + case libcamera::formats::UYVY:\n> > + pixelFormat_ = SDL_PIXELFORMAT_UYVY;\n> > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > + break;\n>\n> Fhr the same of completeness, does SDL have VYUY support ? NV16 and\n> NV61 would also be nice if they are supported. If not that's fine.\n\nOk, I'll add if they are supported.\n\n>\n> > + default:\n> > + std::cerr << sCfg.pixelFormat.toString() << \" not present in libcamera <-> SDL map\"\n> > + << std::endl;\n>\n> std::cerr << \"Unsupported pixel format \"\n> << sCfg.pixelFormat.toString() << std::endl;\n>\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() << std::endl;\n> > + return ret;\n> > + }\n> > +\n> > + sdlInit_ = true;\n> > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > + if (!sdlWindow_) {\n> > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError() << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > + if (!sdlRenderer_) {\n> > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError() << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n>\n> Please move the comment to the next line.\n\nOk.\n\n>\n> > + std::cerr << \"Failed to set SDL render logical size: \" << SDL_GetError() << std::endl;\n> > + }\n> > +\n> > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h);\n> > + if (!sdlTexture_) {\n> > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError() << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this));\n>\n> Please use std::chrono types for the addTimerEvent duration parameter.\n\nAre we sure we want to do this, just for the sake of using the C++\nversion? You end up having to cast because it's not the actual type\nlibevent requires, libevent requires a C timeval to be passed in.\n\n../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t'\n(aka 'long') from incompatible type 'const std::chrono::microseconds'\n(aka 'const duration>')\n tv.tv_usec = period;\n\n>\n> Unless I'm mistaken, the event will keep being triggered indefinitely, which means\n> that SDLSink::processSDLEvents() could call SDL_PollEvent() even after\n> stop() calls SDL_Quit(). That seems dangerous to me, I think we need a\n> way to cancel periodic timer events.\n\nWill look into this, this is all ran through valgrind, address\nsantizers, etc. didn't see anything like that pop up. But will check\nagain.\n\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int SDLSink::stop()\n> > +{\n> > + if (sdlTexture_) {\n> > + SDL_DestroyTexture(sdlTexture_);\n> > + sdlTexture_ = nullptr;\n> > + }\n> > +\n> > + if (sdlRenderer_) {\n> > + SDL_DestroyRenderer(sdlRenderer_);\n> > + sdlRenderer_ = nullptr;\n> > + }\n> > +\n> > + if (sdlWindow_) {\n> > + SDL_DestroyWindow(sdlWindow_);\n> > + sdlWindow_ = nullptr;\n> > + }\n> > +\n> > + if (sdlInit_) {\n> > + SDL_Quit();\n> > + sdlInit_ = false;\n> > + }\n> > +\n> > + return FrameSink::stop();\n> > +}\n> > +\n> > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > +{\n> > + std::unique_ptr image = 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) { /* click close icon then quit */\n>\n> Please move the comment to the next line, and s/click/Click/\n>\n> Actually, maybe a switch/case would be better, to prepare for adding\n> support for more events ? Up to you.\n\nI'll leave it as is, if more events prove useful and someone wants to\nchange to switch in future, I really don't mind.\n\n>\n> > + EventLoop::instance()->exit(0);\n> > + }\n> > + }\n> > +}\n> > +\n> > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > +{\n> > + Image *image = mappedBuffers_[buffer].get();\n> > +\n> > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> > +\n> > + Span data = image->data(i);\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> > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_);\n>\n> According to https://wiki.libsdl.org/SDL_UpdateTexture,\n>\n> \"This is a fairly slow function, intended for use with static textures\n> that do not change often.\n>\n> If the texture is intended to be updated often, it is preferred to\n> create the texture as streaming and use the locking functions referenced\n> below. While this function will work with streaming textures, for\n> optimization reasons you may not get the pixels back if you lock the\n> texture afterward.\"\n>\n> Have you considered this ? I don't know what difference it makes when\n> updating the full texture though, maybe the added complexity isn't worth\n> it.\n\nWill look into this if it's not overly complex will try other\ntechnique suggested in that helpful link you shared.\n\n>\n> > + SDL_RenderClear(sdlRenderer_);\n> > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr);\n> > + SDL_RenderPresent(sdlRenderer_);\n>\n> Does this really have to be repeated for each plane ? That sounds\n> strange.\n\nThis code is commonly written this way, but that doesn't mean it's\nmost optimal like you said :)\n\nI'll explore other techniques.\n\n>\n> > + }\n> > +}\n> > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > new file mode 100644\n> > index 00000000..c9b0ab8e\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.h\n> > @@ -0,0 +1,46 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * sdl_sink.h - SDL Sink\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"frame_sink.h\"\n> > +\n> > +class Image;\n> > +\n> > +class SDLSink : public FrameSink\n> > +{\n> > +public:\n> > + SDLSink();\n> > + ~SDLSink();\n> > +\n> > + int configure(const libcamera::CameraConfiguration &cfg) 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> mappedBuffers_;\n> > +\n> > + SDL_Window *sdlWindow_;\n> > + SDL_Renderer *sdlRenderer_;\n> > + SDL_Texture *sdlTexture_;\n> > + SDL_Rect sdlRect_;\n> > + SDL_PixelFormatEnum pixelFormat_;\n> > + bool sdlInit_;\n> > + int pitch_;\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 475DFC0F1B\n\tfor ;\n\tTue, 19 Apr 2022 14:47:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D04A65644;\n\tTue, 19 Apr 2022 16:47:38 +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 4E25B6563C\n\tfor ;\n\tTue, 19 Apr 2022 16:47:37 +0200 (CEST)","from mail-oo1-f69.google.com (mail-oo1-f69.google.com\n\t[209.85.161.69]) 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-618-MW40H1GNP8u061TEUdQ4yg-1; Tue, 19 Apr 2022 10:47:34 -0400","by mail-oo1-f69.google.com with SMTP id\n\tk189-20020a4a4ac6000000b003353c23321fso7644546oob.18\n\tfor ;\n\tTue, 19 Apr 2022 07:47:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650379658;\n\tbh=67ImAOzgg7CtaK5rQAfFM8gxKmfcqfJHE3kfHZEblIc=;\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=HDb7fnoNyGIKgD1BtGWiWIzrIjd/ASi5Pws1v8ffeG8REs8PrAp+69ZU7U4Zviq4+\n\tDccSlrRyTtH81t+q5bPTx1PMmpF3zmnD4uo6jkUfR88r6uXzvkh9SMK6zuFnIZ3P2y\n\tUNOYBk9G/4EfkSbi1hoHyuXQ5V04pT2auQvEND3wawlvwaKzNRLlBEjo30bOWSKun8\n\t/ccs7Zvnl/m/nD6PNemrxoFhJ+FL8SQ2Flqbe8/3bxvmNWoNYqvbxOOGb7yE2ElccD\n\tULThungkAj8iiHA45Nup1hRfla+uYKdpVMrHiOUlzAch/ps09weTVKUYxOzUhb1WzC\n\t2OXVKkkZ3z3sA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1650379656;\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=PwdnRrsxuN1Q2HIML9xep9tcMQUGYfVMqOVJgJzBog8=;\n\tb=LsInftltuUqbPlVKvTPZfGRpOCRwoPI0qgP+hE+8srtTabNzELv/39jb6l6GqK1wfB30md\n\txKaygXdirdvb8w2hU7qC6a0sJKHWeIybq2Dw1DmsJ4wHX9ddTQAtgm0SSy9FyrpP5VH2bj\n\tgm24fagWrh2g1ajf5jBRl/oHkYwsKb4="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"LsInftlt\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"MW40H1GNP8u061TEUdQ4yg-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=PwdnRrsxuN1Q2HIML9xep9tcMQUGYfVMqOVJgJzBog8=;\n\tb=0eBaae/rAa4FLhM+wYAUvSMdKEQNCoFABbSU14a0UbcMQLQrllczYYymGzW+qoKiEO\n\ta25bOA/dYshC7TNs20H5cgX96wV4DPV0IUMQvha1yza+Of9dgvXEeYZtXzPq7XG0uArO\n\t9Mbu8XScx/CWz1seV/bmPR4DqEDGIVb1GOL7SbvxPiAqXUjB+so4C+cr9T1jo8ldn63w\n\tUGMw5rtGPDn6TrTEm9StKQSk4wM5EDMefDopsyE0ZaHZSp1PvFCMEE6vJOL44r4sVhqb\n\toFB+d/dYoT+GfvfxptWOrg1KyJO0hxBrjmdQgYj8FB2loOYu5AQlRj4Fiz+TRW+577gH\n\td4qA==","X-Gm-Message-State":"AOAM5323C7gfCXMwkiHaOJWP6BZS5ZY9Y+ZmDhcTw6KKVkllC+i/okr5\n\tEBC1HM4Hn7AugCT/x3EBvVpD4mmgZrdqxWtLPrmR66iqAQeR4x2KIrBkoEJfqBIX0mhszSv1opn\n\tMGHbqf2YOmAcZE1d6+Rf/U+aqk+qxjNrawZhMpVypd5oyA5ORtA==","X-Received":["by 2002:a05:6870:f624:b0:e1:c071:121c with SMTP id\n\tek36-20020a056870f62400b000e1c071121cmr6737284oab.182.1650379653577; \n\tTue, 19 Apr 2022 07:47:33 -0700 (PDT)","by 2002:a05:6870:f624:b0:e1:c071:121c with SMTP id\n\tek36-20020a056870f62400b000e1c071121cmr6737271oab.182.1650379653124;\n\tTue, 19 Apr 2022 07:47:33 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJwzpxsbVZTcP3Zj+WvZmrl8AokrbrQsn3A5FmaNlz0EKICN47F4O98N1WObGLCjqUgfU1zR7JQIb+ykjiPeEfY=","MIME-Version":"1.0","References":"<20220408160015.33612-1-ecurtin@redhat.com>\n\t<20220408160015.33612-2-ecurtin@redhat.com>\n\t","In-Reply-To":"","Date":"Tue, 19 Apr 2022 15:47:17 +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 v7 2/2] cam: sdl_sink: Add SDL sink","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22742,"web_url":"https://patchwork.libcamera.org/comment/22742/","msgid":"<165038165586.2548121.2421632713624322623@Monstersaurus>","date":"2022-04-19T15:20:55","subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Eric Curtin via libcamera-devel (2022-04-19 15:47:17)\n> On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart\n> wrote:\n> >\n> > Hi Eric,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote:\n> > > This adds more portability to existing cam sinks. You can pass a\n> > > YUYV camera buffer for example and SDL will handle the pixel buffer\n> > > conversion, although SDL does not support decompression for pixelformats\n> > > like MJPEG. This allows cam reference implementation to display images\n> > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference\n> > > implementation, to run as a desktop application in wayland or x11.\n> > > SDL also has support for Android and ChromeOS which has not been tested.\n> > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where\n> > > existing kms sink did not work. Can also be used as kmsdrm sink. Only\n> > > supports one camera stream at present.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/camera_session.cpp | 8 ++\n> > > src/cam/main.cpp | 5 +\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 10 ++\n> > > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 46 +++++++++\n> > > 6 files changed, 268 insertions(+)\n> > > create mode 100644 src/cam/sdl_sink.cpp\n> > > create mode 100644 src/cam/sdl_sink.h\n> > >\n> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > index 0428b538..30162dbd 100644\n> > > --- a/src/cam/camera_session.cpp\n> > > +++ b/src/cam/camera_session.cpp\n> > > @@ -19,6 +19,9 @@\n> > > #ifdef HAVE_KMS\n> > > #include \"kms_sink.h\"\n> > > #endif\n> > > +#ifdef HAVE_SDL\n> > > +#include \"sdl_sink.h\"\n> > > +#endif\n> > > #include \"main.h\"\n> > > #include \"stream_options.h\"\n> > >\n> > > @@ -187,6 +190,11 @@ int CameraSession::start()\n> > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> >\n> > Space before tab. There are several occurrences.\n> >\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..1d62a64a 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"Display viewfinder through DRM/KMS on specified connector\",\n> > > \"display\", ArgumentOptional, \"connector\", false,\n> > > OptCamera);\n> > > +#endif\n> > > +#ifdef HAVE_SDL\n> > > + parser.addOption(OptSDL, OptionNone,\n> > > + \"Display viewfinder through SDL\",\n> > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> >\n> > I'm tempted to refactor the -D option to specify the backend as an\n> > argument, given that the KMS and SDL sinks are mutually exclusive. I\n> > think this can be done later though.\n> >\n> > > #endif\n> > > parser.addOption(OptFile, OptionString,\n> > > \"Write captured frames to disk\\n\"\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index 62f7bbc9..2b285808 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -18,6 +18,7 @@ enum {\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > OptStream = 's',\n> > > + OptSDL = 'S',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > OptMetadata = 258,\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index 5bab8c9e..59787741 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -32,11 +32,21 @@ 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> > > + ])\n> > > +endif\n> > > +\n> > > cam = executable('cam', cam_sources,\n> > > dependencies : [\n> > > libatomic,\n> > > libcamera_public,\n> > > libdrm,\n> > > + libsdl2,\n> > > libevent,\n> > > ],\n> > > cpp_args : cam_cpp_args,\n> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > new file mode 100644\n> > > index 00000000..03511974\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,198 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.cpp - 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> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLSink::SDLSink()\n> > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false)\n> >\n> > Line wrap please. Same in quite a few locations below.\n> \n> Ok.\n> \n> >\n> > > +{\n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + stop();\n> > > +}\n> > > +\n> > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)\n> > > +{\n> > > + int ret = FrameSink::configure(cfg);\n> > > + if (ret < 0)\n> > > + return ret;\n> > > +\n> > > + if (cfg.size() > 1) {\n> > > + std::cerr << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > + << std::endl;\n> > > + } else if (cfg.empty()) {\n> > > + std::cerr << \"Require at least one camera stream to process\" << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> >\n> > You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match\n> > KMSSink::configure().\n> \n> Ok.\n> \n> >\n> > > + sdlRect_.w = sCfg.size.width;\n> > > + sdlRect_.h = sCfg.size.height;\n> >\n> > Please add a blank line here.\n> \n> Ok.\n> \n> >\n> > > + switch (sCfg.pixelFormat) {\n> > > + case libcamera::formats::YUYV:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YUY2;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> > > +\n> > > + /* From here down the fourcc values are identical between SDL, drm, libcamera */\n> >\n> > They're always identical between DRM and libcamera, that's by design :-)\n> > You can write\n> >\n> > /* From here down the fourcc values are identical between SDL and libcamera */\n> >\n> > or simply drop the message, as I don't think it's really relevant. In\n> > that case I'd move the YVYU and UYVY formats just fter YUYV as they are\n> > related.\n> \n> Ok.\n> \n> >\n> > > + case libcamera::formats::NV21:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_NV21;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::NV12:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_NV12;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::YVU420:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YV12;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::YVYU:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YVYU;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> > > + case libcamera::formats::UYVY:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_UYVY;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> >\n> > Fhr the same of completeness, does SDL have VYUY support ? NV16 and\n> > NV61 would also be nice if they are supported. If not that's fine.\n> \n> Ok, I'll add if they are supported.\n> \n> >\n> > > + default:\n> > > + std::cerr << sCfg.pixelFormat.toString() << \" not present in libcamera <-> SDL map\"\n> > > + << std::endl;\n> >\n> > std::cerr << \"Unsupported pixel format \"\n> > << sCfg.pixelFormat.toString() << std::endl;\n> >\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() << std::endl;\n> > > + return ret;\n> > > + }\n> > > +\n> > > + sdlInit_ = true;\n> > > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > + if (!sdlWindow_) {\n> > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > > + if (!sdlRenderer_) {\n> > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> >\n> > Please move the comment to the next line.\n> \n> Ok.\n> \n> >\n> > > + std::cerr << \"Failed to set SDL render logical size: \" << SDL_GetError() << std::endl;\n> > > + }\n> > > +\n> > > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h);\n> > > + if (!sdlTexture_) {\n> > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this));\n> >\n> > Please use std::chrono types for the addTimerEvent duration parameter.\n> \n> Are we sure we want to do this, just for the sake of using the C++\n> version? You end up having to cast because it's not the actual type\n> libevent requires, libevent requires a C timeval to be passed in.\n> \n> ../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t'\n> (aka 'long') from incompatible type 'const std::chrono::microseconds'\n> (aka 'const duration>')\n> tv.tv_usec = period;\n> \n\nI'd say 'yes' - that's the point of Chrono.\n\nIt means you should be able to call\n\n\tEventLoop::instance()->addTimerEvent(1s, std::bind(&SDLSink::processSDLEvents, this));\nor\n\tEventLoop::instance()->addTimerEvent(100ms, std::bind(&SDLSink::processSDLEvents, this));\nor\n\tEventLoop::instance()->addTimerEvent(10000us, std::bind(&SDLSink::processSDLEvents, this));\n\n (not expected to be equivalent durations above)\n\nAnd the event loop would get added correctly.\n\n> >\n> > Unless I'm mistaken, the event will keep being triggered indefinitely, which means\n> > that SDLSink::processSDLEvents() could call SDL_PollEvent() even after\n> > stop() calls SDL_Quit(). That seems dangerous to me, I think we need a\n> > way to cancel periodic timer events.\n> \n> Will look into this, this is all ran through valgrind, address\n> santizers, etc. didn't see anything like that pop up. But will check\n> again.\n> \n> >\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::stop()\n> > > +{\n> > > + if (sdlTexture_) {\n> > > + SDL_DestroyTexture(sdlTexture_);\n> > > + sdlTexture_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlRenderer_) {\n> > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > + sdlRenderer_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlWindow_) {\n> > > + SDL_DestroyWindow(sdlWindow_);\n> > > + sdlWindow_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlInit_) {\n> > > + SDL_Quit();\n> > > + sdlInit_ = false;\n> > > + }\n> > > +\n> > > + return FrameSink::stop();\n> > > +}\n> > > +\n> > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + std::unique_ptr image = 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) { /* click close icon then quit */\n> >\n> > Please move the comment to the next line, and s/click/Click/\n> >\n> > Actually, maybe a switch/case would be better, to prepare for adding\n> > support for more events ? Up to you.\n> \n> I'll leave it as is, if more events prove useful and someone wants to\n> change to switch in future, I really don't mind.\n\nA switch would already let unhandled events be reported via the default\ncase. But maybe that would be too noisy anyway or unwanted.\n\nSo I guess it doesn't matter too much.\n\n--\nKieran\n\n> \n> >\n> > > + EventLoop::instance()->exit(0);\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + Image *image = mappedBuffers_[buffer].get();\n> > > +\n> > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> > > +\n> > > + Span data = image->data(i);\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> > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_);\n> >\n> > According to https://wiki.libsdl.org/SDL_UpdateTexture,\n> >\n> > \"This is a fairly slow function, intended for use with static textures\n> > that do not change often.\n> >\n> > If the texture is intended to be updated often, it is preferred to\n> > create the texture as streaming and use the locking functions referenced\n> > below. While this function will work with streaming textures, for\n> > optimization reasons you may not get the pixels back if you lock the\n> > texture afterward.\"\n> >\n> > Have you considered this ? I don't know what difference it makes when\n> > updating the full texture though, maybe the added complexity isn't worth\n> > it.\n> \n> Will look into this if it's not overly complex will try other\n> technique suggested in that helpful link you shared.\n> \n> >\n> > > + SDL_RenderClear(sdlRenderer_);\n> > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr);\n> > > + SDL_RenderPresent(sdlRenderer_);\n> >\n> > Does this really have to be repeated for each plane ? That sounds\n> > strange.\n> \n> This code is commonly written this way, but that doesn't mean it's\n> most optimal like you said :)\n> \n> I'll explore other techniques.\n> \n> >\n> > > + }\n> > > +}\n> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > new file mode 100644\n> > > index 00000000..c9b0ab8e\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,46 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.h - SDL Sink\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"frame_sink.h\"\n> > > +\n> > > +class Image;\n> > > +\n> > > +class SDLSink : public FrameSink\n> > > +{\n> > > +public:\n> > > + SDLSink();\n> > > + ~SDLSink();\n> > > +\n> > > + int configure(const libcamera::CameraConfiguration &cfg) 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> mappedBuffers_;\n> > > +\n> > > + SDL_Window *sdlWindow_;\n> > > + SDL_Renderer *sdlRenderer_;\n> > > + SDL_Texture *sdlTexture_;\n> > > + SDL_Rect sdlRect_;\n> > > + SDL_PixelFormatEnum pixelFormat_;\n> > > + bool sdlInit_;\n> > > + int pitch_;\n> > > +};\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\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 8C138C3256\n\tfor ;\n\tTue, 19 Apr 2022 15:21:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAEA66563D;\n\tTue, 19 Apr 2022 17:20:59 +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 548416563C\n\tfor ;\n\tTue, 19 Apr 2022 17:20:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE7B125B;\n\tTue, 19 Apr 2022 17:20:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650381659;\n\tbh=dbQ3cX+fadxqPp+A/Ivm3Oxv+cX5j80a6d3m9HScftM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GpN987B2+F7IBy8YiYobEw1Sfl7WbV0iWjDTkuXbYyT4MJGsjWBDeGrnU0wdKh3hv\n\t0y5Q0BTQOV9vw48vS1vVN5vE9SXs0W1LKhxGBvzcuDNM7eFSoEkIV7126A/za8fFPp\n\tjZ+vpUXjngWAjMDdoHK3QxjHS5xQzyRbY+JdR5gCaGz9dWA0WUWELIwLl3KAXNYxul\n\tjWriNcRMENMgy5uU8xIjckun9n9yrku/AalqFw8juTOyFN54TPxkY17Ig1+ZBP0FUf\n\tpo7vrvX87ewSo+UV7X549XzZcfhmEI5gxE2hep4TFp9F8iwNW91/Y3hP3N51tU4izL\n\tPv7Ao0sC2LHGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650381657;\n\tbh=dbQ3cX+fadxqPp+A/Ivm3Oxv+cX5j80a6d3m9HScftM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ovjhHBuRdlNGeKvBktGyNbAa8VeoF2Hz153PDSSYmUVNnI7INjXDqvFGddozU4lq3\n\tDmvXgu2y/CyHii+PtP8REL8Z1G+5GNa7K2jbLxQbqZl6e4u21m+wgu9/aSgalO1K5e\n\tz1FSjqqmIbo6x3NFYNjO9VkNf6/kyUJRCTsZ322Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ovjhHBuR\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220408160015.33612-1-ecurtin@redhat.com>\n\t<20220408160015.33612-2-ecurtin@redhat.com>\n\t\n\t","To":"Eric Curtin ,\n\tEric Curtin via libcamera-devel ,\n\tLaurent Pinchart ","Date":"Tue, 19 Apr 2022 16:20:55 +0100","Message-ID":"<165038165586.2548121.2421632713624322623@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22745,"web_url":"https://patchwork.libcamera.org/comment/22745/","msgid":"","date":"2022-04-19T16:12:01","subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Tue, Apr 19, 2022 at 03:47:17PM +0100, Eric Curtin wrote:\n> On Fri, 15 Apr 2022 at 18:13, Laurent Pinchart wrote:\n> > On Fri, Apr 08, 2022 at 05:00:15PM +0100, Eric Curtin via libcamera-devel wrote:\n> > > This adds more portability to existing cam sinks. You can pass a\n> > > YUYV camera buffer for example and SDL will handle the pixel buffer\n> > > conversion, although SDL does not support decompression for pixelformats\n> > > like MJPEG. This allows cam reference implementation to display images\n> > > on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference\n> > > implementation, to run as a desktop application in wayland or x11.\n> > > SDL also has support for Android and ChromeOS which has not been tested.\n> > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where\n> > > existing kms sink did not work. Can also be used as kmsdrm sink. Only\n> > > supports one camera stream at present.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > src/cam/camera_session.cpp | 8 ++\n> > > src/cam/main.cpp | 5 +\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 10 ++\n> > > src/cam/sdl_sink.cpp | 198 +++++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 46 +++++++++\n> > > 6 files changed, 268 insertions(+)\n> > > create mode 100644 src/cam/sdl_sink.cpp\n> > > create mode 100644 src/cam/sdl_sink.h\n> > >\n> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > index 0428b538..30162dbd 100644\n> > > --- a/src/cam/camera_session.cpp\n> > > +++ b/src/cam/camera_session.cpp\n> > > @@ -19,6 +19,9 @@\n> > > #ifdef HAVE_KMS\n> > > #include \"kms_sink.h\"\n> > > #endif\n> > > +#ifdef HAVE_SDL\n> > > +#include \"sdl_sink.h\"\n> > > +#endif\n> > > #include \"main.h\"\n> > > #include \"stream_options.h\"\n> > >\n> > > @@ -187,6 +190,11 @@ int CameraSession::start()\n> > > sink_ = std::make_unique(options_[OptDisplay].toString());\n> >\n> > Space before tab. There are several occurrences.\n> >\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..1d62a64a 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -137,6 +137,11 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"Display viewfinder through DRM/KMS on specified connector\",\n> > > \"display\", ArgumentOptional, \"connector\", false,\n> > > OptCamera);\n> > > +#endif\n> > > +#ifdef HAVE_SDL\n> > > + parser.addOption(OptSDL, OptionNone,\n> > > + \"Display viewfinder through SDL\",\n> > > + \"sdl\", ArgumentNone, \"\", false, OptCamera);\n> >\n> > I'm tempted to refactor the -D option to specify the backend as an\n> > argument, given that the KMS and SDL sinks are mutually exclusive. I\n> > think this can be done later though.\n> >\n> > > #endif\n> > > parser.addOption(OptFile, OptionString,\n> > > \"Write captured frames to disk\\n\"\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index 62f7bbc9..2b285808 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -18,6 +18,7 @@ enum {\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > OptStream = 's',\n> > > + OptSDL = 'S',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > OptMetadata = 258,\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index 5bab8c9e..59787741 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -32,11 +32,21 @@ 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> > > + ])\n> > > +endif\n> > > +\n> > > cam = executable('cam', cam_sources,\n> > > dependencies : [\n> > > libatomic,\n> > > libcamera_public,\n> > > libdrm,\n> > > + libsdl2,\n> > > libevent,\n> > > ],\n> > > cpp_args : cam_cpp_args,\n> > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp\n> > > new file mode 100644\n> > > index 00000000..03511974\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,198 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.cpp - 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> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLSink::SDLSink()\n> > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlTexture_(nullptr), sdlRect_({}), sdlInit_(false)\n> >\n> > Line wrap please. Same in quite a few locations below.\n> \n> Ok.\n> \n> >\n> > > +{\n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + stop();\n> > > +}\n> > > +\n> > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)\n> > > +{\n> > > + int ret = FrameSink::configure(cfg);\n> > > + if (ret < 0)\n> > > + return ret;\n> > > +\n> > > + if (cfg.size() > 1) {\n> > > + std::cerr << \"SDL sink only supports one camera stream at present, streaming first camera stream\"\n> > > + << std::endl;\n> > > + } else if (cfg.empty()) {\n> > > + std::cerr << \"Require at least one camera stream to process\" << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> >\n> > You could rename 'cfg' to 'config' and 'sCfg' to 'cfg' to match\n> > KMSSink::configure().\n> \n> Ok.\n> \n> >\n> > > + sdlRect_.w = sCfg.size.width;\n> > > + sdlRect_.h = sCfg.size.height;\n> >\n> > Please add a blank line here.\n> \n> Ok.\n> \n> >\n> > > + switch (sCfg.pixelFormat) {\n> > > + case libcamera::formats::YUYV:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YUY2;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> > > +\n> > > + /* From here down the fourcc values are identical between SDL, drm, libcamera */\n> >\n> > They're always identical between DRM and libcamera, that's by design :-)\n> > You can write\n> >\n> > /* From here down the fourcc values are identical between SDL and libcamera */\n> >\n> > or simply drop the message, as I don't think it's really relevant. In\n> > that case I'd move the YVYU and UYVY formats just fter YUYV as they are\n> > related.\n> \n> Ok.\n> \n> >\n> > > + case libcamera::formats::NV21:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_NV21;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::NV12:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_NV12;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::YVU420:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YV12;\n> > > + pitch_ = sdlRect_.w;\n> > > + break;\n> > > + case libcamera::formats::YVYU:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_YVYU;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> > > + case libcamera::formats::UYVY:\n> > > + pixelFormat_ = SDL_PIXELFORMAT_UYVY;\n> > > + pitch_ = 4 * ((sdlRect_.w + 1) / 2);\n> > > + break;\n> >\n> > Fhr the same of completeness, does SDL have VYUY support ? NV16 and\n> > NV61 would also be nice if they are supported. If not that's fine.\n> \n> Ok, I'll add if they are supported.\n> \n> >\n> > > + default:\n> > > + std::cerr << sCfg.pixelFormat.toString() << \" not present in libcamera <-> SDL map\"\n> > > + << std::endl;\n> >\n> > std::cerr << \"Unsupported pixel format \"\n> > << sCfg.pixelFormat.toString() << std::endl;\n> >\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() << std::endl;\n> > > + return ret;\n> > > + }\n> > > +\n> > > + sdlInit_ = true;\n> > > + sdlWindow_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > + if (!sdlWindow_) {\n> > > + std::cerr << \"Failed to create SDL window: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0);\n> > > + if (!sdlRenderer_) {\n> > > + std::cerr << \"Failed to create SDL renderer: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h);\n> > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */\n> >\n> > Please move the comment to the next line.\n> \n> Ok.\n> \n> >\n> > > + std::cerr << \"Failed to set SDL render logical size: \" << SDL_GetError() << std::endl;\n> > > + }\n> > > +\n> > > + sdlTexture_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, sdlRect_.h);\n> > > + if (!sdlTexture_) {\n> > > + std::cerr << \"Failed to create SDL texture: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + EventLoop::instance()->addTimerEvent(10000, std::bind(&SDLSink::processSDLEvents, this));\n> >\n> > Please use std::chrono types for the addTimerEvent duration parameter.\n> \n> Are we sure we want to do this, just for the sake of using the C++\n> version? You end up having to cast because it's not the actual type\n> libevent requires, libevent requires a C timeval to be passed in.\n> \n> ../src/cam/event_loop.cpp:105:15: error: assigning to '__suseconds_t'\n> (aka 'long') from incompatible type 'const std::chrono::microseconds'\n> (aka 'const duration>')\n> tv.tv_usec = period;\n\nThat's the whole point of std::chrono. With the existing code it's too\neasy to write\n\n\tEventLoop::instance()->addTimerEvent(10 /* ms */, std::bind(&SDLSink::processSDLEvents, this));\n\nwhen you would actually mean\n\n\tEventLoop::instance()->addTimerEvent(10000 /* µs */, std::bind(&SDLSink::processSDLEvents, this));\n\nWith std::chrono types, the compiler will accept both\n\n\tEventLoop::instance()->addTimerEvent(10ms, std::bind(&SDLSink::processSDLEvents, this));\n\nand\n\n\tEventLoop::instance()->addTimerEvent(10000µs, std::bind(&SDLSink::processSDLEvents, this));\n\nand do the right thing, and will reject the ambiguous calls\n\n\tEventLoop::instance()->addTimerEvent(10 /* ms */, std::bind(&SDLSink::processSDLEvents, this));\n\tEventLoop::instance()->addTimerEvent(10000 /* µs */, std::bind(&SDLSink::processSDLEvents, this));\n\n> > Unless I'm mistaken, the event will keep being triggered indefinitely, which means\n> > that SDLSink::processSDLEvents() could call SDL_PollEvent() even after\n> > stop() calls SDL_Quit(). That seems dangerous to me, I think we need a\n> > way to cancel periodic timer events.\n> \n> Will look into this, this is all ran through valgrind, address\n> santizers, etc. didn't see anything like that pop up. But will check\n> again.\n\nIt could be that the issue is subject to race conditions, or that the\nevent loops is stopped after the sink is stopped without a chance to\nprocess more events. This could change in the future, so I'd like to fix\nthis issue already, even if it can't be triggered today.\n\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int SDLSink::stop()\n> > > +{\n> > > + if (sdlTexture_) {\n> > > + SDL_DestroyTexture(sdlTexture_);\n> > > + sdlTexture_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlRenderer_) {\n> > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > + sdlRenderer_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlWindow_) {\n> > > + SDL_DestroyWindow(sdlWindow_);\n> > > + sdlWindow_ = nullptr;\n> > > + }\n> > > +\n> > > + if (sdlInit_) {\n> > > + SDL_Quit();\n> > > + sdlInit_ = false;\n> > > + }\n> > > +\n> > > + return FrameSink::stop();\n> > > +}\n> > > +\n> > > +void SDLSink::mapBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + std::unique_ptr image = 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) { /* click close icon then quit */\n> >\n> > Please move the comment to the next line, and s/click/Click/\n> >\n> > Actually, maybe a switch/case would be better, to prepare for adding\n> > support for more events ? Up to you.\n> \n> I'll leave it as is, if more events prove useful and someone wants to\n> change to switch in future, I really don't mind.\n> \n> > > + EventLoop::instance()->exit(0);\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +void SDLSink::renderBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + Image *image = mappedBuffers_[buffer].get();\n> > > +\n> > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> > > +\n> > > + Span data = image->data(i);\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> > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), pitch_);\n> >\n> > According to https://wiki.libsdl.org/SDL_UpdateTexture,\n> >\n> > \"This is a fairly slow function, intended for use with static textures\n> > that do not change often.\n> >\n> > If the texture is intended to be updated often, it is preferred to\n> > create the texture as streaming and use the locking functions referenced\n> > below. While this function will work with streaming textures, for\n> > optimization reasons you may not get the pixels back if you lock the\n> > texture afterward.\"\n> >\n> > Have you considered this ? I don't know what difference it makes when\n> > updating the full texture though, maybe the added complexity isn't worth\n> > it.\n> \n> Will look into this if it's not overly complex will try other\n> technique suggested in that helpful link you shared.\n> \n> >\n> > > + SDL_RenderClear(sdlRenderer_);\n> > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, nullptr, nullptr);\n> > > + SDL_RenderPresent(sdlRenderer_);\n> >\n> > Does this really have to be repeated for each plane ? That sounds\n> > strange.\n> \n> This code is commonly written this way, but that doesn't mean it's\n> most optimal like you said :)\n\nIf we take NV12 as an example, the loop will execute twice for the same\nbuffer once for the luma plane and once for the chroma plane. I don't\nsee anything in the SDL calls above that tells SDL about which plane is\nbeing updated, which makes me believe there's an issue.\n\n> I'll explore other techniques.\n> \n> > > + }\n> > > +}\n> > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h\n> > > new file mode 100644\n> > > index 00000000..c9b0ab8e\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,46 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * sdl_sink.h - SDL Sink\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"frame_sink.h\"\n> > > +\n> > > +class Image;\n> > > +\n> > > +class SDLSink : public FrameSink\n> > > +{\n> > > +public:\n> > > + SDLSink();\n> > > + ~SDLSink();\n> > > +\n> > > + int configure(const libcamera::CameraConfiguration &cfg) 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> mappedBuffers_;\n> > > +\n> > > + SDL_Window *sdlWindow_;\n> > > + SDL_Renderer *sdlRenderer_;\n> > > + SDL_Texture *sdlTexture_;\n> > > + SDL_Rect sdlRect_;\n> > > + SDL_PixelFormatEnum pixelFormat_;\n> > > + bool sdlInit_;\n> > > + int pitch_;\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 CB272C0F1B\n\tfor ;\n\tTue, 19 Apr 2022 16:12:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1356B65642;\n\tTue, 19 Apr 2022 18:12:05 +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 80AA36563C\n\tfor ;\n\tTue, 19 Apr 2022 18:12:03 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-6-68-nat.elisa-mobile.fi\n\t[85.76.6.68])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE95025B;\n\tTue, 19 Apr 2022 18:12:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650384725;\n\tbh=PXGxMcGgc1TlXPaZDTScXK+UF7foyGlMil1rUL4bJPo=;\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=rLwmWUvARshVLsiU74PlrIB5S1XorQMzfdbYsMFqTPpi6rKYDzCwkVgk5eOrbtKYX\n\tLyTOLiA7pGQiKkYl7Dmzrp80iWyAB8dR1SAa7ApHlOZCvQR9RsJtbusOEROQqzW6XE\n\t5zII1XHuuxNJdC+GLNY3KIEupG19BisqxuSmniEthHOlelk5UEtsrA75HmFMJnsLhZ\n\tbgmIGfmsUI4drwJzbWb5xc40D9Hm36vBZCKkdSVu+yP1DqGJIf/T1wbT8L/bTnpvhG\n\tElE0/kMQdyoBTP1wObsER5zQp9AxQ3LlkhhxhyDZw7rTZTiQIzBJ54DnP/I4DCk6an\n\tRWkbieef0eZZQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650384723;\n\tbh=PXGxMcGgc1TlXPaZDTScXK+UF7foyGlMil1rUL4bJPo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CB7h8u2hFS6LNR861/rcNY4S8hevPpzyd2Rp0D+BqkDcRw1wmFVXL2oNDvF2hTV3q\n\tKTRfsTIyvm4ds356vHUifWviRtavPnHZ+XgnmOmdjFvkapHew4rlmwtwXlTcMbQfIG\n\tlNeAzd9i/V9ESrEIpp/Y41tsa450dnO/G0Y57ue8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CB7h8u2h\"; dkim-atps=neutral","Date":"Tue, 19 Apr 2022 19:12:01 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220408160015.33612-1-ecurtin@redhat.com>\n\t<20220408160015.33612-2-ecurtin@redhat.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 2/2] cam: sdl_sink: Add SDL sink","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\" "}}]