[{"id":22513,"web_url":"https://patchwork.libcamera.org/comment/22513/","msgid":"","date":"2022-03-29T15:09:58","subject":"Re: [libcamera-devel] [PATCH v4] 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 Tue, Mar 29, 2022 at 12:17:25PM +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 I have not 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.\n> \n> Signed-off-by: Eric Curtin \n> ---\n> Changes in v2:\n> - Remove hardcoded pixel format from SDL_CreateTexture call\n> \n> Changes in v3:\n> - Drop blank line\n> - The contents of the if..endif are indented\n> - Split configure function into start and configure\n> - Add SDL_DestroyRenderer\n> - Remove assign and test in the same statement\n> \n> Changes in v4:\n> - Add processSDLEvents as a timed event in the evloop\n> ---\n> src/cam/camera_session.cpp | 8 +++\n> src/cam/event_loop.cpp | 10 ++-\n> src/cam/event_loop.h | 1 +\n> src/cam/main.cpp | 5 ++\n> src/cam/main.h | 1 +\n> src/cam/meson.build | 10 +++\n> src/cam/sdl_sink.cpp | 142 +++++++++++++++++++++++++++++++++++++\n> src/cam/sdl_sink.h | 42 +++++++++++\n> 8 files changed, 218 insertions(+), 1 deletion(-)\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> #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/event_loop.cpp b/src/cam/event_loop.cpp\n> index e25784c0..41339d4e 100644\n> --- a/src/cam/event_loop.cpp\n> +++ b/src/cam/event_loop.cpp\n> @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,\n> \t\treturn;\n> \t}\n> \n> -\tint ret = event_add(event->event_, nullptr);\n> +\tstruct timeval *tp = nullptr;\n> +\tstruct timeval tv;\n> +\tif (events == EV_PERSIST) {\n> +\t\ttp = &tv;\n> +\t\ttv.tv_sec = 0;\n> +\t\ttv.tv_usec = 10000; /* every 10 ms */\n> +\t}\n\nCould you please split the change to the event loop to a separate patch,\nwith an explanation of what it does in the commit message ? This isn't\ntrivial to understand.\n\n> +\n> +\tint ret = event_add(event->event_, tp);\n> \tif (ret < 0) {\n> \t\tstd::cerr << \"Failed to add event for fd \" << fd << std::endl;\n> \t\treturn;\n> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> index a4613eb2..8fd9bd20 100644\n> --- a/src/cam/event_loop.h\n> +++ b/src/cam/event_loop.h\n> @@ -20,6 +20,7 @@ class EventLoop\n> {\n> public:\n> \tenum EventType {\n> +\t\tDefault = 0, /* the event can be triggered only by a timeout or by manual activation */\n> \t\tRead = 1,\n> \t\tWrite = 2,\n> \t};\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> #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..a64f95a0 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -11,6 +11,7 @@ enum {\n> \tOptCamera = 'c',\n> \tOptCapture = 'C',\n> \tOptDisplay = 'D',\n> +\tOptSDL = 'S',\n\nLet's keep the options sorted.\n\n> \tOptFile = 'F',\n> \tOptHelp = 'h',\n> \tOptInfo = 'I',\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index e8e2ae57..bd536c5b 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -32,11 +32,21 @@ cam_sources += files([\n> ])\n> endif\n> \n> +libsdl2 = dependency('SDL2', required : false)\n> +\n> +if libsdl2.found()\n> + cam_cpp_args += [ '-DHAVE_SDL' ]\n\nNo space within the [ ].\n\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..6c6e37d0\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -0,0 +1,142 @@\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: sdlRenderer_(0)\n\nIt's a pointer, should be initialized to nullptr, not 0.\n\n> +{\n> +\tmemset(&sdlRect_, 0, sizeof(sdlRect_));\n\nThis could be initialized with\n\n\tsdlRect_({})\n\nin the constructor member initializers list.\n\n> +}\n> +\n> +SDLSink::~SDLSink()\n> +{\n> +\tif (sdlRenderer_)\n> +\t\tSDL_DestroyRenderer(sdlRenderer_);\n> +\tSDL_Quit();\n\nShouldn't the cleanup go to stop(), now that you have the initialization\nin start() ?\n\nNo need for any cleanup on sdlScreen_ and sdlTexture_ ?\n\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> +\tconst libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> +\tpf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> +\tif (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> +\t\tpf = SDL_PIXELFORMAT_YUY2;\n> +\t} else if (int ne = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"); !ne) {\n\nNo variable declaration in an if () statement.\n\n> +\t\tstd::cerr << \"SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n\nThose two lines are too long.\n\nReplace \\n with std::endl\n\n> +\t\treturn -EINVAL;\n> +\t}\n\nI don't think this is right. SDL pixel formats don't map directly to the\n4CC values used by libcamera. You need a translation table.\n\n> +\n> +\tsdlRect_.w = sCfg.size.width;\n> +\tsdlRect_.h = sCfg.size.height;\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 << \"SDL_Init error - exiting: \" << SDL_GetError() << std::endl;\n\n\t\tstd::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n\t\t\t << std::endl;\n\nand similarly below.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tsdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> +\t\t\t\t SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> +\t\t\t\t sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> +\tif (!sdlScreen_) {\n> +\t\tstd::cerr << \"SDL_CreateWindow error - exiting: \" << SDL_GetError() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tsdlRenderer_ = SDL_CreateRenderer(\n> +\t\tsdlScreen_, -1, 0);\n> +\tif (!sdlRenderer_) {\n> +\t\tstd::cerr << \"SDL_CreateRenderer error - exiting: \" << SDL_GetError() << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tSDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,\n> +\t\t\t\t sdlRect_.h);\n> +\n> +\tsdlTexture_ =\n> +\t\tSDL_CreateTexture(sdlRenderer_, pf,\n> +\t\t\t\t SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> +\t\t\t\t sdlRect_.h);\n> +\tEventLoop::instance()->addEvent(-1, EventLoop::Default,\n> +\t\t\t\t\tstd::bind(&SDLSink::processSDLEvents, this));\n> +\n> +\treturn 0;\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\twriteBuffer(buffer);\n\nWill this really work properly if the request contains multiple buffers\n?\n\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> +\t\t\tkill(getpid(), SIGINT);\n\nThat's too harsh. Calling the exit() function of the event loop should\nbe enough.\n\n> +\t\t}\n> +\t}\n> +}\n> +\n> +void SDLSink::writeBuffer(FrameBuffer *buffer)\n\nThis function doesn't \"write\" a buffer, it should be named differently.\n\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(), sdlRect_.w * 2);\n\nIs the last argument format-dependent ?\n\n> +\t\tSDL_RenderClear(sdlRenderer_);\n> +\t\tSDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n\ns/NULL/nullptr/\n\n> +\t\tSDL_RenderPresent(sdlRenderer_);\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..6f20e2d6\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.h\n> @@ -0,0 +1,42 @@\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> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tbool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> +\tvoid writeBuffer(libcamera::FrameBuffer *buffer);\n> +\tvoid processSDLEvents();\n> +\n> +\tstd::map> mappedBuffers_;\n\nI'd add a blank line here.\n\n> +\tSDL_Window *sdlScreen_;\n> +\tSDL_Renderer *sdlRenderer_;\n> +\tSDL_Texture *sdlTexture_;\n> +\tSDL_Rect sdlRect_;\n> +\tSDL_PixelFormatEnum pf;\n\ns/pf/pixelFormat_/\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 CBFF3C0F1B\n\tfor ;\n\tTue, 29 Mar 2022 15:10:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40A8D604C5;\n\tTue, 29 Mar 2022 17:10:04 +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 829A160135\n\tfor ;\n\tTue, 29 Mar 2022 17:10:02 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1AFC2F7;\n\tTue, 29 Mar 2022 17:10:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648566604;\n\tbh=FsGER25dHlViasSTkUgnWxoUQuZJ0AUy4Ie+GDT2j5A=;\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=joPNmx0+qatLUBjpAmXGAAxaCgWW+T+7uptecrE9uoHLSwJNLadZqJ7qeWCGFtr6s\n\tzQsKkG7AhrKNrgsb4ub/t8QWLtBF8ra411xPFaz/f56DI1GpfcOlAGwRB82Lse/BA3\n\tfHZl8UNcXDJdtGUBR1LdZDthPE8Upi/aSasOOByVTYJcPS8XbFwTX058Dnc94jimz9\n\t0s6EPdWJQrF5k4LS0wq6YsOv4VD4165FYRfpWNgi8og7Ht1Vs7ROQePvKkh7JZS9XS\n\tVjyynkpVYiNJ+dCyXx39yQbJHNT0OCzg0/PMwMf+ag+VQZn3vuKYoXo6KZb4eo/XP0\n\twzB4g+IppBs0w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648566602;\n\tbh=FsGER25dHlViasSTkUgnWxoUQuZJ0AUy4Ie+GDT2j5A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OsX//k2ooPxVENgi8wfSRSdW0yrqQFQ3HbdUUyxMbDkIWSlWV6hOI1c3isUOD7S9T\n\tgMowgixH0T9kauk73oqQ3tkaEa3xO6EU3MoNLX3lPwja2taqJw56EL2M6CpqYEQSZh\n\tZBPeotKK09RV03yL2+a0a93FgrXYuSAbfh71IBOo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OsX//k2o\"; dkim-atps=neutral","Date":"Tue, 29 Mar 2022 18:09:58 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220329111725.24565-1-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220329111725.24565-1-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v4] 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":22514,"web_url":"https://patchwork.libcamera.org/comment/22514/","msgid":"","date":"2022-03-29T16:45:57","subject":"Re: [libcamera-devel] [PATCH v4] 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 Tue, 29 Mar 2022 at 16:10, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 29, 2022 at 12:17:25PM +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 I have not 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.\n> >\n> > Signed-off-by: Eric Curtin \n> > ---\n> > Changes in v2:\n> > - Remove hardcoded pixel format from SDL_CreateTexture call\n> >\n> > Changes in v3:\n> > - Drop blank line\n> > - The contents of the if..endif are indented\n> > - Split configure function into start and configure\n> > - Add SDL_DestroyRenderer\n> > - Remove assign and test in the same statement\n> >\n> > Changes in v4:\n> > - Add processSDLEvents as a timed event in the evloop\n> > ---\n> > src/cam/camera_session.cpp | 8 +++\n> > src/cam/event_loop.cpp | 10 ++-\n> > src/cam/event_loop.h | 1 +\n> > src/cam/main.cpp | 5 ++\n> > src/cam/main.h | 1 +\n> > src/cam/meson.build | 10 +++\n> > src/cam/sdl_sink.cpp | 142 +++++++++++++++++++++++++++++++++++++\n> > src/cam/sdl_sink.h | 42 +++++++++++\n> > 8 files changed, 218 insertions(+), 1 deletion(-)\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> > #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/event_loop.cpp b/src/cam/event_loop.cpp\n> > index e25784c0..41339d4e 100644\n> > --- a/src/cam/event_loop.cpp\n> > +++ b/src/cam/event_loop.cpp\n> > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,\n> > return;\n> > }\n> >\n> > - int ret = event_add(event->event_, nullptr);\n> > + struct timeval *tp = nullptr;\n> > + struct timeval tv;\n> > + if (events == EV_PERSIST) {\n> > + tp = &tv;\n> > + tv.tv_sec = 0;\n> > + tv.tv_usec = 10000; /* every 10 ms */\n> > + }\n>\n> Could you please split the change to the event loop to a separate patch,\n> with an explanation of what it does in the commit message ? This isn't\n> trivial to understand.\n\nOk.\n\n>\n> > +\n> > + int ret = event_add(event->event_, tp);\n> > if (ret < 0) {\n> > std::cerr << \"Failed to add event for fd \" << fd << std::endl;\n> > return;\n> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > index a4613eb2..8fd9bd20 100644\n> > --- a/src/cam/event_loop.h\n> > +++ b/src/cam/event_loop.h\n> > @@ -20,6 +20,7 @@ class EventLoop\n> > {\n> > public:\n> > enum EventType {\n> > + Default = 0, /* the event can be triggered only by a timeout or by manual activation */\n> > Read = 1,\n> > Write = 2,\n> > };\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> > #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..a64f95a0 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -11,6 +11,7 @@ enum {\n> > OptCamera = 'c',\n> > OptCapture = 'C',\n> > OptDisplay = 'D',\n> > + OptSDL = 'S',\n>\n> Let's keep the options sorted.\n\nOk.\n\n>\n> > OptFile = 'F',\n> > OptHelp = 'h',\n> > OptInfo = 'I',\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index e8e2ae57..bd536c5b 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -32,11 +32,21 @@ cam_sources += files([\n> > ])\n> > endif\n> >\n> > +libsdl2 = dependency('SDL2', required : false)\n> > +\n> > +if libsdl2.found()\n> > + cam_cpp_args += [ '-DHAVE_SDL' ]\n>\n> No space within the [ ].\n\nOk.\n\n>\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..6c6e37d0\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -0,0 +1,142 @@\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> > + : sdlRenderer_(0)\n>\n> It's a pointer, should be initialized to nullptr, not 0.\n\nOk.\n\n>\n> > +{\n> > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n>\n> This could be initialized with\n>\n> sdlRect_({})\n>\n> in the constructor member initializers list.\n\nOk.\n\n>\n> > +}\n> > +\n> > +SDLSink::~SDLSink()\n> > +{\n> > + if (sdlRenderer_)\n> > + SDL_DestroyRenderer(sdlRenderer_);\n> > + SDL_Quit();\n>\n> Shouldn't the cleanup go to stop(), now that you have the initialization\n> in start() ?\n\nYes thanks for spotting.\n\n>\n> No need for any cleanup on sdlScreen_ and sdlTexture_ ?\n\nNo need.\n\n>\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> > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > + pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> > + pf = SDL_PIXELFORMAT_YUY2;\n> > + } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"); !ne) {\n>\n> No variable declaration in an if () statement.\n\nOk.\n\n>\n> > + std::cerr << \"SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n>\n> Those two lines are too long.\n\nOk.\n\n>\n> Replace \\n with std::endl\n>\n> > + return -EINVAL;\n> > + }\n>\n> I don't think this is right. SDL pixel formats don't map directly to the\n> 4CC values used by libcamera. You need a translation table.\n\nI can start one, I won't be able to test it much as I don't have the\nhardware to do that but I can start.\n\n>\n> > +\n> > + sdlRect_.w = sCfg.size.width;\n> > + sdlRect_.h = sCfg.size.height;\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 << \"SDL_Init error - exiting: \" << SDL_GetError() << std::endl;\n>\n> std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> << std::endl;\n>\n> and similarly below.\n\nOk\n\n>\n> > + return ret;\n> > + }\n> > +\n> > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > + sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > + if (!sdlScreen_) {\n> > + std::cerr << \"SDL_CreateWindow error - exiting: \" << SDL_GetError() << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + sdlRenderer_ = SDL_CreateRenderer(\n> > + sdlScreen_, -1, 0);\n> > + if (!sdlRenderer_) {\n> > + std::cerr << \"SDL_CreateRenderer error - exiting: \" << SDL_GetError() << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,\n> > + sdlRect_.h);\n> > +\n> > + sdlTexture_ =\n> > + SDL_CreateTexture(sdlRenderer_, pf,\n> > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > + sdlRect_.h);\n> > + EventLoop::instance()->addEvent(-1, EventLoop::Default,\n> > + std::bind(&SDLSink::processSDLEvents, this));\n> > +\n> > + return 0;\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> > + writeBuffer(buffer);\n>\n> Will this really work properly if the request contains multiple buffers\n> ?\n\nI will write more code to solve this.\n\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> > + kill(getpid(), SIGINT);\n>\n> That's too harsh. Calling the exit() function of the event loop should\n> be enough.\n\nI'll try that, my first thought was to try and make it behave like\nwhen you type Ctrl-C as much as possible, but yeah it's weird this\nway, a process calling it's own signal handler.\n\n>\n> > + }\n> > + }\n> > +}\n> > +\n> > +void SDLSink::writeBuffer(FrameBuffer *buffer)\n>\n> This function doesn't \"write\" a buffer, it should be named differently.\n\nrenderBuffer maybe?\n\n>\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(), sdlRect_.w * 2);\n>\n> Is the last argument format-dependent ?\n\nI'll check, lacking test hardware of course though, I only have UVC\nYUYV cameras which isn't ideal.\n\n>\n> > + SDL_RenderClear(sdlRenderer_);\n> > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n>\n> s/NULL/nullptr/\n\nOk\n\n>\n> > + SDL_RenderPresent(sdlRenderer_);\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..6f20e2d6\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.h\n> > @@ -0,0 +1,42 @@\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> > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > + bool processRequest(libcamera::Request *request) override;\n> > +\n> > +private:\n> > + void writeBuffer(libcamera::FrameBuffer *buffer);\n> > + void processSDLEvents();\n> > +\n> > + std::map> mappedBuffers_;\n>\n> I'd add a blank line here.\n\nOk\n\n>\n> > + SDL_Window *sdlScreen_;\n> > + SDL_Renderer *sdlRenderer_;\n> > + SDL_Texture *sdlTexture_;\n> > + SDL_Rect sdlRect_;\n> > + SDL_PixelFormatEnum pf;\n>\n> s/pf/pixelFormat_/\n\nOk.\n\n>\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 06E20C3256\n\tfor ;\n\tTue, 29 Mar 2022 16:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B570C65634;\n\tTue, 29 Mar 2022 18:46:20 +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 913B2604BD\n\tfor ;\n\tTue, 29 Mar 2022 18:46:17 +0200 (CEST)","from mail-oa1-f69.google.com (mail-oa1-f69.google.com\n\t[209.85.160.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-389-n3V0CbGnPkSf2N5SNMfO_w-1; Tue, 29 Mar 2022 12:46:14 -0400","by mail-oa1-f69.google.com with SMTP id\n\t586e51a60fabf-de2fae282cso4482562fac.19\n\tfor ;\n\tTue, 29 Mar 2022 09:46:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648572380;\n\tbh=Uph8MehMEV6hcA32vpA9D6gs/lroK8ZgltHe2FbkUBQ=;\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=k/RM0foVSKGUHFtwn195Ta3Iu1I+t/EzPk9lffV/qspkiXc1a68WwwXjcaTDahk8/\n\tkp0zgFUjEaeUAPBX4CBUvxg2tBn1gupPJVsbginrV+GS/ZpOLRlfOfxYVocu404Ank\n\tzCjc/960WLooXIuxStUHNjYOtKPou8cpq93G7Vazkm4JUUxBwLv6ct8y92Nn0Y5933\n\tc+FWZ2qbjry+TOM1TT6Su0oLGNeClWg9eK/gG74HE3pDT7SkI4pDDEwzqUfXp7wy1R\n\t5Uh/Wzpi5taHfdDcj+stF6Ie9rIWKvj7PGxwLkzHsm3vjFPLZaAmQp1Js1nEFMVfxl\n\tw41pFxLA1m0bA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1648572376;\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=rgOiq99D1K0/c1oOLEsup2PvnicuxK0G4JPirI0HBUM=;\n\tb=SYGZNlHnJJ15peq0wfl5dB2J7xs38vmIzi8pjTd0HJnDAXjnEpnD9nKj1lvXyTOZW8+j5x\n\tB1fXQFgO1qfnw8uDnpLWmINLHknQzyzf8F6atLQ90q6epDZIvrpnp8TejoqXdWjSLeACjZ\n\tP81WPXh+oMUF9rqVhcxlwkFuDTCpPY0="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"SYGZNlHn\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"n3V0CbGnPkSf2N5SNMfO_w-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=rgOiq99D1K0/c1oOLEsup2PvnicuxK0G4JPirI0HBUM=;\n\tb=6JasbZmwpDhmmZQTEC7F7q4QHq1bWgxECRd5bGc9QXU9CXW3I7570juyL2b8GZsFIE\n\tZNoB1xbIdPwfo8WnyZX0adbgAWYppU9n8HKQ6e2w9e8YCFGLVqa62PhH2L+LU7lUI9dQ\n\tmGJc7waIcETDXfmS2GtFPvUh6RZincwgC2k9rTOvIqbOH/pwrqc3DFFQ6dl7YeyUZpmJ\n\t+VfcqEuBApUyMah6rqiF2G6NsoMcfqCxSnL/ukOkLSD0Lnh695NV46W8/t/w6BEDb6fO\n\taHTC4d70yHKGNFxNwC5QCCgaNho+HzJO+4ivQ3cKHnroW8qZ6he5VUyQu0c7t4koOXzW\n\tJg2Q==","X-Gm-Message-State":"AOAM5316GNfQx8WpMQq59gH+N/p++rbm2ci4Z+I9bLTslGbGZuHUeK9J\n\t+5Va2Ow5MpTrgHUi3DMBr6rTNwdRAfNRq3L8SMPl9JR5mfnOBY7ZPt5ytYlyP+gqxQXuo0HE8qf\n\tTSV3olKU33pCKbDfXuNVUcfcZ+tNvbh6733fjqcgyZdAg5HO9Rw==","X-Received":["by 2002:a05:6870:5a4:b0:da:b3f:3206 with SMTP id\n\tm36-20020a05687005a400b000da0b3f3206mr33218oap.182.1648572373538; \n\tTue, 29 Mar 2022 09:46:13 -0700 (PDT)","by 2002:a05:6870:5a4:b0:da:b3f:3206 with SMTP id\n\tm36-20020a05687005a400b000da0b3f3206mr33192oap.182.1648572373024;\n\tTue, 29 Mar 2022 09:46:13 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJzID/PYaH6WrIfyXGAE+4ovK0VwrrJFZRAKvh0/lxMhqq2A6zNv6VF5DMY7HfERD4q7SzEJh2iKv6cvDbj6AFQ=","MIME-Version":"1.0","References":"<20220329111725.24565-1-ecurtin@redhat.com>\n\t","In-Reply-To":"","Date":"Tue, 29 Mar 2022 17:45:57 +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 v4] 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":22517,"web_url":"https://patchwork.libcamera.org/comment/22517/","msgid":"","date":"2022-03-29T20:09:18","subject":"Re: [libcamera-devel] [PATCH v4] 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, Mar 29, 2022 at 05:45:57PM +0100, Eric Curtin wrote:\n> On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart wrote:\n> > On Tue, Mar 29, 2022 at 12:17:25PM +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 I have not 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.\n> > >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > Changes in v2:\n> > > - Remove hardcoded pixel format from SDL_CreateTexture call\n> > >\n> > > Changes in v3:\n> > > - Drop blank line\n> > > - The contents of the if..endif are indented\n> > > - Split configure function into start and configure\n> > > - Add SDL_DestroyRenderer\n> > > - Remove assign and test in the same statement\n> > >\n> > > Changes in v4:\n> > > - Add processSDLEvents as a timed event in the evloop\n> > > ---\n> > > src/cam/camera_session.cpp | 8 +++\n> > > src/cam/event_loop.cpp | 10 ++-\n> > > src/cam/event_loop.h | 1 +\n> > > src/cam/main.cpp | 5 ++\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 10 +++\n> > > src/cam/sdl_sink.cpp | 142 +++++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 42 +++++++++++\n> > > 8 files changed, 218 insertions(+), 1 deletion(-)\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> > > #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/event_loop.cpp b/src/cam/event_loop.cpp\n> > > index e25784c0..41339d4e 100644\n> > > --- a/src/cam/event_loop.cpp\n> > > +++ b/src/cam/event_loop.cpp\n> > > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,\n> > > return;\n> > > }\n> > >\n> > > - int ret = event_add(event->event_, nullptr);\n> > > + struct timeval *tp = nullptr;\n> > > + struct timeval tv;\n> > > + if (events == EV_PERSIST) {\n> > > + tp = &tv;\n> > > + tv.tv_sec = 0;\n> > > + tv.tv_usec = 10000; /* every 10 ms */\n> > > + }\n> >\n> > Could you please split the change to the event loop to a separate patch,\n> > with an explanation of what it does in the commit message ? This isn't\n> > trivial to understand.\n> \n> Ok.\n> \n> > > +\n> > > + int ret = event_add(event->event_, tp);\n> > > if (ret < 0) {\n> > > std::cerr << \"Failed to add event for fd \" << fd << std::endl;\n> > > return;\n> > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > > index a4613eb2..8fd9bd20 100644\n> > > --- a/src/cam/event_loop.h\n> > > +++ b/src/cam/event_loop.h\n> > > @@ -20,6 +20,7 @@ class EventLoop\n> > > {\n> > > public:\n> > > enum EventType {\n> > > + Default = 0, /* the event can be triggered only by a timeout or by manual activation */\n> > > Read = 1,\n> > > Write = 2,\n> > > };\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> > > #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..a64f95a0 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -11,6 +11,7 @@ enum {\n> > > OptCamera = 'c',\n> > > OptCapture = 'C',\n> > > OptDisplay = 'D',\n> > > + OptSDL = 'S',\n> >\n> > Let's keep the options sorted.\n> \n> Ok.\n> \n> > > OptFile = 'F',\n> > > OptHelp = 'h',\n> > > OptInfo = 'I',\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index e8e2ae57..bd536c5b 100644\n> > > --- a/src/cam/meson.build\n> > > +++ b/src/cam/meson.build\n> > > @@ -32,11 +32,21 @@ cam_sources += files([\n> > > ])\n> > > endif\n> > >\n> > > +libsdl2 = dependency('SDL2', required : false)\n> > > +\n> > > +if libsdl2.found()\n> > > + cam_cpp_args += [ '-DHAVE_SDL' ]\n> >\n> > No space within the [ ].\n> \n> Ok.\n> \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..6c6e37d0\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,142 @@\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> > > + : sdlRenderer_(0)\n> >\n> > It's a pointer, should be initialized to nullptr, not 0.\n> \n> Ok.\n> \n> > > +{\n> > > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> >\n> > This could be initialized with\n> >\n> > sdlRect_({})\n> >\n> > in the constructor member initializers list.\n> \n> Ok.\n> \n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + if (sdlRenderer_)\n> > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > + SDL_Quit();\n> >\n> > Shouldn't the cleanup go to stop(), now that you have the initialization\n> > in start() ?\n> \n> Yes thanks for spotting.\n> \n> > No need for any cleanup on sdlScreen_ and sdlTexture_ ?\n> \n> No need.\n\nMaybe a comment would be nice to state this, so that it looks less like\nit has been forgotten to someone not familiar with SDL.\n\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> > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > > + pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> > > + pf = SDL_PIXELFORMAT_YUY2;\n> > > + } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"); !ne) {\n> >\n> > No variable declaration in an if () statement.\n> \n> Ok.\n> \n> > > + std::cerr << \"SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> >\n> > Those two lines are too long.\n> \n> Ok.\n> \n> > Replace \\n with std::endl\n> >\n> > > + return -EINVAL;\n> > > + }\n> >\n> > I don't think this is right. SDL pixel formats don't map directly to the\n> > 4CC values used by libcamera. You need a translation table.\n> \n> I can start one, I won't be able to test it much as I don't have the\n> hardware to do that but I can start.\n\nYou don't have to handle all formats, you can limit the translation to\nthe format(s) you can test (or possibly adding a few of the most common\nother formats). More formats can always be added later.\n\n> > > +\n> > > + sdlRect_.w = sCfg.size.width;\n> > > + sdlRect_.h = sCfg.size.height;\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 << \"SDL_Init error - exiting: \" << SDL_GetError() << std::endl;\n> >\n> > std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > << std::endl;\n> >\n> > and similarly below.\n> \n> Ok\n> \n> > > + return ret;\n> > > + }\n> > > +\n> > > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > > + sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > + if (!sdlScreen_) {\n> > > + std::cerr << \"SDL_CreateWindow error - exiting: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + sdlRenderer_ = SDL_CreateRenderer(\n> > > + sdlScreen_, -1, 0);\n> > > + if (!sdlRenderer_) {\n> > > + std::cerr << \"SDL_CreateRenderer error - exiting: \" << SDL_GetError() << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,\n> > > + sdlRect_.h);\n> > > +\n> > > + sdlTexture_ =\n> > > + SDL_CreateTexture(sdlRenderer_, pf,\n> > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > > + sdlRect_.h);\n> > > + EventLoop::instance()->addEvent(-1, EventLoop::Default,\n> > > + std::bind(&SDLSink::processSDLEvents, this));\n> > > +\n> > > + return 0;\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> > > + writeBuffer(buffer);\n> >\n> > Will this really work properly if the request contains multiple buffers\n> > ?\n> \n> I will write more code to solve this.\n\nThe KMS sink doesn't support this either, so a simple option would be to\nhandle the first stream only, or to reject multi-stream configurations\nin the configure() function. Another option could be to create multiple\nSDL windows, one for each stream, but that will become complicated I\nsuppose.\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> > > + kill(getpid(), SIGINT);\n> >\n> > That's too harsh. Calling the exit() function of the event loop should\n> > be enough.\n> \n> I'll try that, my first thought was to try and make it behave like\n> when you type Ctrl-C as much as possible, but yeah it's weird this\n> way, a process calling it's own signal handler.\n> \n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +void SDLSink::writeBuffer(FrameBuffer *buffer)\n> >\n> > This function doesn't \"write\" a buffer, it should be named differently.\n> \n> renderBuffer maybe?\n\nSounds good to me.\n\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(), sdlRect_.w * 2);\n> >\n> > Is the last argument format-dependent ?\n> \n> I'll check, lacking test hardware of course though, I only have UVC\n> YUYV cameras which isn't ideal.\n> \n> > > + SDL_RenderClear(sdlRenderer_);\n> > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> >\n> > s/NULL/nullptr/\n> \n> Ok\n> \n> > > + SDL_RenderPresent(sdlRenderer_);\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..6f20e2d6\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,42 @@\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> > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > +\n> > > + bool processRequest(libcamera::Request *request) override;\n> > > +\n> > > +private:\n> > > + void writeBuffer(libcamera::FrameBuffer *buffer);\n> > > + void processSDLEvents();\n> > > +\n> > > + std::map> mappedBuffers_;\n> >\n> > I'd add a blank line here.\n> \n> Ok\n> \n> > > + SDL_Window *sdlScreen_;\n> > > + SDL_Renderer *sdlRenderer_;\n> > > + SDL_Texture *sdlTexture_;\n> > > + SDL_Rect sdlRect_;\n> > > + SDL_PixelFormatEnum pf;\n> >\n> > s/pf/pixelFormat_/\n> \n> Ok.\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 31BBCC0F1B\n\tfor ;\n\tTue, 29 Mar 2022 20:09:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7760165634;\n\tTue, 29 Mar 2022 22:09:23 +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 7FA6D604BD\n\tfor ;\n\tTue, 29 Mar 2022 22:09:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB04F2F7;\n\tTue, 29 Mar 2022 22:09:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648584563;\n\tbh=pCFYCwkg2Qc2/hs8W+9mmYLOEo1Xahjm5FblEo+34dA=;\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=ToweGsGWH/HQtcjUvv3/kYCYB7azOX8bOUMRqKn/d8Njm5Uv57SWoTx+Wa304XECA\n\tv3XsyNQg6ZIuLUh9pHRmBfzVsn95g7xVUGKooQBvkUzy966ooxOcKnQSlM/8iTj/e0\n\t/LJwNUXkKQAqGQ1WBzPkP5sdSl4i1CxHBWbOS/I3ij+/TgNyI3amHx65EUDDm0aekx\n\tyjSL3SS/xF77QAvBRaAjmAuprWB0DnJAKoYAtpvCx8qq1IKWqPUJ00gcfYRzSqq5gs\n\tIJks1gopHFtSv6M7ra61OyqArfNi/dnoeHUkpG5sZelWAjdvYwJJCbh21hBhow+fJj\n\thdJOnmjrZpAfw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648584562;\n\tbh=pCFYCwkg2Qc2/hs8W+9mmYLOEo1Xahjm5FblEo+34dA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OLkTpgtljmRFM4IWzRY9yKSkuFfw5OXhFhoKuMpgJzDj1NSDpEhIUhPSPeSY14PcJ\n\tGT99ljbqbj2i36qEL6unElouWusJP80g13Tk3W4T7EUzRlm4ZHZzS2ogxqLiHhglmH\n\tl4sr4dNbwwKySFkeBmpTlToBLvZZhnXQGX1cVUSI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OLkTpgtl\"; dkim-atps=neutral","Date":"Tue, 29 Mar 2022 23:09:18 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220329111725.24565-1-ecurtin@redhat.com>\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 v4] 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":22531,"web_url":"https://patchwork.libcamera.org/comment/22531/","msgid":"","date":"2022-03-30T15:11:24","subject":"Re: [libcamera-devel] [PATCH v4] 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 Tue, 29 Mar 2022 at 21:09, Laurent Pinchart\n wrote:\n>\n> Hi Eric,\n>\n> On Tue, Mar 29, 2022 at 05:45:57PM +0100, Eric Curtin wrote:\n> > On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart wrote:\n> > > On Tue, Mar 29, 2022 at 12:17:25PM +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 I have not 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.\n> > > >\n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > Changes in v2:\n> > > > - Remove hardcoded pixel format from SDL_CreateTexture call\n> > > >\n> > > > Changes in v3:\n> > > > - Drop blank line\n> > > > - The contents of the if..endif are indented\n> > > > - Split configure function into start and configure\n> > > > - Add SDL_DestroyRenderer\n> > > > - Remove assign and test in the same statement\n> > > >\n> > > > Changes in v4:\n> > > > - Add processSDLEvents as a timed event in the evloop\n> > > > ---\n> > > > src/cam/camera_session.cpp | 8 +++\n> > > > src/cam/event_loop.cpp | 10 ++-\n> > > > src/cam/event_loop.h | 1 +\n> > > > src/cam/main.cpp | 5 ++\n> > > > src/cam/main.h | 1 +\n> > > > src/cam/meson.build | 10 +++\n> > > > src/cam/sdl_sink.cpp | 142 +++++++++++++++++++++++++++++++++++++\n> > > > src/cam/sdl_sink.h | 42 +++++++++++\n> > > > 8 files changed, 218 insertions(+), 1 deletion(-)\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> > > > #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/event_loop.cpp b/src/cam/event_loop.cpp\n> > > > index e25784c0..41339d4e 100644\n> > > > --- a/src/cam/event_loop.cpp\n> > > > +++ b/src/cam/event_loop.cpp\n> > > > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,\n> > > > return;\n> > > > }\n> > > >\n> > > > - int ret = event_add(event->event_, nullptr);\n> > > > + struct timeval *tp = nullptr;\n> > > > + struct timeval tv;\n> > > > + if (events == EV_PERSIST) {\n> > > > + tp = &tv;\n> > > > + tv.tv_sec = 0;\n> > > > + tv.tv_usec = 10000; /* every 10 ms */\n> > > > + }\n> > >\n> > > Could you please split the change to the event loop to a separate patch,\n> > > with an explanation of what it does in the commit message ? This isn't\n> > > trivial to understand.\n> >\n> > Ok.\n> >\n> > > > +\n> > > > + int ret = event_add(event->event_, tp);\n> > > > if (ret < 0) {\n> > > > std::cerr << \"Failed to add event for fd \" << fd << std::endl;\n> > > > return;\n> > > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > > > index a4613eb2..8fd9bd20 100644\n> > > > --- a/src/cam/event_loop.h\n> > > > +++ b/src/cam/event_loop.h\n> > > > @@ -20,6 +20,7 @@ class EventLoop\n> > > > {\n> > > > public:\n> > > > enum EventType {\n> > > > + Default = 0, /* the event can be triggered only by a timeout or by manual activation */\n> > > > Read = 1,\n> > > > Write = 2,\n> > > > };\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> > > > #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..a64f95a0 100644\n> > > > --- a/src/cam/main.h\n> > > > +++ b/src/cam/main.h\n> > > > @@ -11,6 +11,7 @@ enum {\n> > > > OptCamera = 'c',\n> > > > OptCapture = 'C',\n> > > > OptDisplay = 'D',\n> > > > + OptSDL = 'S',\n> > >\n> > > Let's keep the options sorted.\n> >\n> > Ok.\n> >\n> > > > OptFile = 'F',\n> > > > OptHelp = 'h',\n> > > > OptInfo = 'I',\n> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > index e8e2ae57..bd536c5b 100644\n> > > > --- a/src/cam/meson.build\n> > > > +++ b/src/cam/meson.build\n> > > > @@ -32,11 +32,21 @@ cam_sources += files([\n> > > > ])\n> > > > endif\n> > > >\n> > > > +libsdl2 = dependency('SDL2', required : false)\n> > > > +\n> > > > +if libsdl2.found()\n> > > > + cam_cpp_args += [ '-DHAVE_SDL' ]\n> > >\n> > > No space within the [ ].\n> >\n> > Ok.\n> >\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..6c6e37d0\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.cpp\n> > > > @@ -0,0 +1,142 @@\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> > > > + : sdlRenderer_(0)\n> > >\n> > > It's a pointer, should be initialized to nullptr, not 0.\n> >\n> > Ok.\n> >\n> > > > +{\n> > > > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> > >\n> > > This could be initialized with\n> > >\n> > > sdlRect_({})\n> > >\n> > > in the constructor member initializers list.\n> >\n> > Ok.\n> >\n> > > > +}\n> > > > +\n> > > > +SDLSink::~SDLSink()\n> > > > +{\n> > > > + if (sdlRenderer_)\n> > > > + SDL_DestroyRenderer(sdlRenderer_);\n> > > > + SDL_Quit();\n> > >\n> > > Shouldn't the cleanup go to stop(), now that you have the initialization\n> > > in start() ?\n> >\n> > Yes thanks for spotting.\n> >\n> > > No need for any cleanup on sdlScreen_ and sdlTexture_ ?\n> >\n> > No need.\n>\n> Maybe a comment would be nice to state this, so that it looks less like\n> it has been forgotten to someone not familiar with SDL.\n\nIt's funny actually, was trying different combinations today with\nvalgrind, address sanitizer, the only cleanup function you actually\nneed is SDL_Quit, it seems to clean up the rest automatically. I think\nI'll put them all in explicitly though, just to alleviate future\nreaders getting worried. Some SDL docs call them all, some don't.\n\n>\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> > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > > > + pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> > > > + pf = SDL_PIXELFORMAT_YUY2;\n> > > > + } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"); !ne) {\n> > >\n> > > No variable declaration in an if () statement.\n> >\n> > Ok.\n> >\n> > > > + std::cerr << \"SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> > >\n> > > Those two lines are too long.\n> >\n> > Ok.\n> >\n> > > Replace \\n with std::endl\n> > >\n> > > > + return -EINVAL;\n> > > > + }\n> > >\n> > > I don't think this is right. SDL pixel formats don't map directly to the\n> > > 4CC values used by libcamera. You need a translation table.\n> >\n> > I can start one, I won't be able to test it much as I don't have the\n> > hardware to do that but I can start.\n>\n> You don't have to handle all formats, you can limit the translation to\n> the format(s) you can test (or possibly adding a few of the most common\n> other formats). More formats can always be added later.\n>\n> > > > +\n> > > > + sdlRect_.w = sCfg.size.width;\n> > > > + sdlRect_.h = sCfg.size.height;\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 << \"SDL_Init error - exiting: \" << SDL_GetError() << std::endl;\n> > >\n> > > std::cerr << \"Failed to initialize SDL: \" << SDL_GetError()\n> > > << std::endl;\n> > >\n> > > and similarly below.\n> >\n> > Ok\n> >\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,\n> > > > + sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> > > > + if (!sdlScreen_) {\n> > > > + std::cerr << \"SDL_CreateWindow error - exiting: \" << SDL_GetError() << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + sdlRenderer_ = SDL_CreateRenderer(\n> > > > + sdlScreen_, -1, 0);\n> > > > + if (!sdlRenderer_) {\n> > > > + std::cerr << \"SDL_CreateRenderer error - exiting: \" << SDL_GetError() << std::endl;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,\n> > > > + sdlRect_.h);\n> > > > +\n> > > > + sdlTexture_ =\n> > > > + SDL_CreateTexture(sdlRenderer_, pf,\n> > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,\n> > > > + sdlRect_.h);\n> > > > + EventLoop::instance()->addEvent(-1, EventLoop::Default,\n> > > > + std::bind(&SDLSink::processSDLEvents, this));\n> > > > +\n> > > > + return 0;\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> > > > + writeBuffer(buffer);\n> > >\n> > > Will this really work properly if the request contains multiple buffers\n> > > ?\n> >\n> > I will write more code to solve this.\n>\n> The KMS sink doesn't support this either, so a simple option would be to\n> handle the first stream only, or to reject multi-stream configurations\n> in the configure() function. Another option could be to create multiple\n> SDL windows, one for each stream, but that will become complicated I\n> suppose.\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> > > > + kill(getpid(), SIGINT);\n> > >\n> > > That's too harsh. Calling the exit() function of the event loop should\n> > > be enough.\n> >\n> > I'll try that, my first thought was to try and make it behave like\n> > when you type Ctrl-C as much as possible, but yeah it's weird this\n> > way, a process calling it's own signal handler.\n> >\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void SDLSink::writeBuffer(FrameBuffer *buffer)\n> > >\n> > > This function doesn't \"write\" a buffer, it should be named differently.\n> >\n> > renderBuffer maybe?\n>\n> Sounds good to me.\n>\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(), sdlRect_.w * 2);\n> > >\n> > > Is the last argument format-dependent ?\n> >\n> > I'll check, lacking test hardware of course though, I only have UVC\n> > YUYV cameras which isn't ideal.\n> >\n> > > > + SDL_RenderClear(sdlRenderer_);\n> > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> > >\n> > > s/NULL/nullptr/\n> >\n> > Ok\n> >\n> > > > + SDL_RenderPresent(sdlRenderer_);\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..6f20e2d6\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.h\n> > > > @@ -0,0 +1,42 @@\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> > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > +\n> > > > + bool processRequest(libcamera::Request *request) override;\n> > > > +\n> > > > +private:\n> > > > + void writeBuffer(libcamera::FrameBuffer *buffer);\n> > > > + void processSDLEvents();\n> > > > +\n> > > > + std::map> mappedBuffers_;\n> > >\n> > > I'd add a blank line here.\n> >\n> > Ok\n> >\n> > > > + SDL_Window *sdlScreen_;\n> > > > + SDL_Renderer *sdlRenderer_;\n> > > > + SDL_Texture *sdlTexture_;\n> > > > + SDL_Rect sdlRect_;\n> > > > + SDL_PixelFormatEnum pf;\n> > >\n> > > s/pf/pixelFormat_/\n> >\n> > Ok.\n> >\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 6EB5EC3256\n\tfor ;\n\tWed, 30 Mar 2022 15:11:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2C4F604BC;\n\tWed, 30 Mar 2022 17:11:47 +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 4C1D660135\n\tfor ;\n\tWed, 30 Mar 2022 17:11:45 +0200 (CEST)","from mail-oo1-f71.google.com (mail-oo1-f71.google.com\n\t[209.85.161.71]) 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-357-5rL8G_iPNpeIShEJw3UtTQ-1; Wed, 30 Mar 2022 11:11:42 -0400","by mail-oo1-f71.google.com with SMTP id\n\ti20-20020a4a9294000000b003247837949fso13078878ooh.18\n\tfor ;\n\tWed, 30 Mar 2022 08:11:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648653107;\n\tbh=+ld4uCRa7ESaRI1o48+hUgzA4xFdY9QI91OTo8PLfqk=;\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=N055p2715KWMLXittk1ybt8p5EBjNpWp4eVc3Yg+FrMG8tiQ7HHMPamoAqJveAX0i\n\tEhtbWGSKtODhIEA4ZLbOCpHa6KgUjSn7QHYgh0OJQiVB4NDxN/89sPz1b14e/nFjam\n\tJQGbNFVqmBcNrdeRCKfCbRnA12TkegGLlLLOChy1WQxORS7O3VZkaSxSxq7xQwGiFw\n\tT1HIUO39OvWlUPwvL55WyvwYGMqZJkLIpBkM6sHWDm79oyvM/XaYNrkUl+Oi0slfoQ\n\tRkXesR1g1AVBIHmZDHlGRYYLFBBaKuYCHegt4j+tSWcbnnnsBU08PNx3QhvTD2KfzQ\n\tU6mN8ryaGDELg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1648653104;\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=zd4GrMpGAnj8TUzpebw4vwtC1ehq1LR2+VIpXEdL5+k=;\n\tb=Wq4Locjx/AHn4/iBeK5SaMS7mr2jwGiX7Z+FtZC4Ed5sSjxbNn7R17GS5XYNMDYdSblr47\n\tHU7+Hi5lkmIBOI3Smv6vJrdqaRsmccTfEd1d2rtuF/bZU8isfjWZ4Whnlz19mBeGyeRsdx\n\t2WFKVW/AikVk3N6SJ8vysQvCk81mFKM="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"Wq4Locjx\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"5rL8G_iPNpeIShEJw3UtTQ-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=zd4GrMpGAnj8TUzpebw4vwtC1ehq1LR2+VIpXEdL5+k=;\n\tb=ICGd8PAI6e6xC4T/h+d6ffneTmdtaDdLvDDWhxJLEopmWi1ADmWNr0bO8perdPYcJ8\n\tMy5dPA0zJ+YggWLE7c0anCwsfFFl/pCGDpIh4UVnNNOTDemtMR7QHkcYmagcLK/zjf8J\n\tUtDgxLeCv69ocrUhaZaYfww/ESEIRpW4vfsbyD3AI7Ak3tQyNNmfCXuIR6EY5ceM+rGs\n\t5HhdHMwztwlk/2XgcMiQzd1r3KUr9iuwQmjMQjfN+F3webSh57vS+ABzn93dGXNU1cNe\n\txoMNGig/YT/x+uqLpMBC/woMrtDrCM0y/jMvb6pwgILLcca82WN7DR8oCkzK/u0i9NCr\n\tbHlQ==","X-Gm-Message-State":"AOAM530V8e6+fVteW7CtHaCMR5/SC+9XJXG+yQGaNWDZZ7aZXien0XbG\n\tvFP6yTlFNDFZ6em0aSoZmPLyxwqiUQSggIGRWW3TANwbHdan0drDwO44HsNSbqh2UaCxLKAg7Yk\n\tf+Mi1WYvQdKWHjDmW97NY4lCLXs7Xd2OCAVTJKqKnYPEz1RC0bg==","X-Received":["by 2002:a05:6808:210b:b0:2d9:a6d3:b029 with SMTP id\n\tr11-20020a056808210b00b002d9a6d3b029mr46002oiw.182.1648653101185; \n\tWed, 30 Mar 2022 08:11:41 -0700 (PDT)","by 2002:a05:6808:210b:b0:2d9:a6d3:b029 with SMTP id\n\tr11-20020a056808210b00b002d9a6d3b029mr45978oiw.182.1648653100670;\n\tWed, 30 Mar 2022 08:11:40 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJzOD7t2U3vjSCSXtRtIVKwqD8iM1CBh/O8q3Kw8mSZQTLMqIGleEdX3iYJe1Wrf2+pv97BVEMhtv/WKfT5yT2I=","MIME-Version":"1.0","References":"<20220329111725.24565-1-ecurtin@redhat.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 30 Mar 2022 16:11:24 +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 v4] 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\" "}}]