[{"id":22370,"web_url":"https://patchwork.libcamera.org/comment/22370/","msgid":"<164799206875.506124.16534994403383923955@Monstersaurus>","date":"2022-03-22T23:34:28","subject":"Re: [libcamera-devel] [PATCH v2] 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":"Hi Eric,\n\nQuoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36)\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.\n\nThis sounds very helpful and useful to me...\n\n> \n> Signed-off-by: Eric Curtin \n> ---\n> Changes in v2:\n> - Remove hardcoded pixel format from SDL_CreateTexture call\n> ---\n> src/cam/camera_session.cpp | 8 +++\n> src/cam/main.cpp | 6 ++\n> src/cam/main.h | 1 +\n> src/cam/meson.build | 10 +++\n> src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++\n> src/cam/sdl_sink.h | 40 ++++++++++++\n> 6 files changed, 190 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> #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..406d61bc 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \"display\", ArgumentOptional, \"connector\", false,\n> OptCamera);\n> #endif\n> +\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> \"If the file name ends with a '/', it sets the directory in which\\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> OptFile = 'F',\n> OptHelp = 'h',\n> OptInfo = 'I',\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index e8e2ae57..44202ef0 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> +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..480df1b4\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.cpp\n> @@ -0,0 +1,125 @@\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 \"image.h\"\n> +\n> +using namespace libcamera;\n> +\n> +SDLSink::SDLSink()\n> +{\n> + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> +}\n> +\n> +SDLSink::~SDLSink()\n> +{\n> + SDL_Quit();\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 ((ret = SDL_Init(SDL_INIT_VIDEO))) {\n> + std::cout << \"Could not initialize SDL - \" << SDL_GetError() << \"\\n\";\n> + return ret;\n> + }\n> +\n> + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width,\n> + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> + if (!sdlScreen_) {\n> + std::cerr << \"SDL: could not create window - exiting: \" << SDL_GetError() << \"\\n\";\n> + return -1;\n\nis an errno more appropriate?\nSame for the others..\n\n> + }\n> +\n> + sdlRenderer_ = SDL_CreateRenderer(\n> + sdlScreen_, -1, 0);\n> + if (!sdlRenderer_) {\n> + std::cerr << \"SDL_CreateRenderer Error\\n\";\n> + return -2;\n> + }\n> +\n> + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width,\n> + sCfg.size.height);\n> +\n> + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> + pf = SDL_PIXELFORMAT_YUY2;\n> + }\n\nIs this some sort of conversion that most fourccs are compatible\n'except' YUY2? I'd be wary if there is even a single difference, and\nit might be better to put in an explicit conversion table or map?\n\nParticularly as libcamera fourcc's are based on DRM fourccs... are SDL\nfourcc's defined as directly compatible there? I.e. there are frequently\ndifferences in byte ordering on the RGB/BGR formats that cause issues.\n\n> +\n> + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"))) {\n> + std::cerr << \"error: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> + return -3;\n> + }\n> +\n> + sdlTexture_ =\n> + SDL_CreateTexture(sdlRenderer_, pf,\n> + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width,\n> + sCfg.size.height);\n> + sdlRect_.w = sCfg.size.width;\n> + sdlRect_.h = sCfg.size.height;\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> + return true;\n> +}\n> +\n> +void SDLSink::writeBuffer(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(), sdlRect_.w * 2);\n> + SDL_RenderClear(sdlRenderer_);\n> + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> + SDL_RenderPresent(sdlRenderer_);\n> + SDL_Event e;\n> + while (SDL_PollEvent(&e)) {\n> + if (e.type == SDL_QUIT) { // click close icon then quit\n> + kill(getpid(), SIGINT);\n> + }\n> + };\n\nThis seems like an odd place to be doing the event loop polling. I guess\nit's intentional to be able to 'hook' it in somewhere?\n\nI'd be tempted to pull it out to a separate function at least.\n\nHaving never looked at SDL code before, I must say, - that looks like\nit's really quite easy to pull together a window and display!\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..f4f843fa\n> --- /dev/null\n> +++ b/src/cam/sdl_sink.h\n> @@ -0,0 +1,40 @@\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> +\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> +\n> + std::map> mappedBuffers_;\n> + SDL_Window *sdlScreen_;\n> + SDL_Renderer *sdlRenderer_;\n> + SDL_Texture *sdlTexture_;\n> + SDL_Rect sdlRect_;\n> +};\n> -- \n> 2.35.1\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 591C0C0F1B\n\tfor ;\n\tTue, 22 Mar 2022 23:34:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C97B9604DB;\n\tWed, 23 Mar 2022 00:34:32 +0100 (CET)","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 0B118604C6\n\tfor ;\n\tWed, 23 Mar 2022 00:34:32 +0100 (CET)","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 A92A49DE;\n\tWed, 23 Mar 2022 00:34:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647992072;\n\tbh=N5O3q3SCaSsCcYPMY929aatcTPZDsQl1/K+rx6pXJmA=;\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:\n\tFrom;\n\tb=vLCYpgFNi0/hjoJ3csjqmfo7oIcAId15R6cWZYC/NzY5m4rAELS5OOWUw2YGuUEka\n\tTqpx+nv8aoqbhasYF3ZrOdy248Wlk3gAVakb0dWeBwcRMh5vcKPZcBiIqqBWV1aCFc\n\tEwpepy4EFKhkhMFWG/evSExoBkOEjt9PeInq/43DTuryEkJLbwvh5g+VGNjk91f+Rd\n\tXc2Y3T4CH99d6yX2Xd+aom5L2b7aYvHi3rbXobdaC8GoR75grXezY1SXi/1Pusm34C\n\tmsJ6fbZSr9Bw7MMsZWM+BMXihTDFcmkHytNOCtf1Gk7/+puI7IFIKb2R+EttUrbHsy\n\tMtyL6hBzbvrcA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647992071;\n\tbh=N5O3q3SCaSsCcYPMY929aatcTPZDsQl1/K+rx6pXJmA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=aky4HZSsT9t9kQudQe8oDWwOEatmYipQwHCIwccUuOMFBMIydsoM4C3sVU8LvFOjg\n\ty7QL8gN1CJrmJcGotC0K89JH72fnyEWhu6bzx540z4+SZoPNVOm9anKfJGmykNAiWr\n\tlPi6Hc4HBMEP2hnVCXiITXVztSktBfYbJWX/NHmE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aky4HZSs\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220322190936.69281-1-ecurtin@redhat.com>","References":"<20220322190936.69281-1-ecurtin@redhat.com>","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Date":"Tue, 22 Mar 2022 23:34:28 +0000","Message-ID":"<164799206875.506124.16534994403383923955@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22376,"web_url":"https://patchwork.libcamera.org/comment/22376/","msgid":"","date":"2022-03-23T00:35:50","subject":"Re: [libcamera-devel] [PATCH v2] 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":"On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Hi Eric,\n> \n> Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36)\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.\n> \n> This sounds very helpful and useful to me...\n> \n> > Signed-off-by: Eric Curtin \n> > ---\n> > Changes in v2:\n> > - Remove hardcoded pixel format from SDL_CreateTexture call\n> > ---\n> > src/cam/camera_session.cpp | 8 +++\n> > src/cam/main.cpp | 6 ++\n> > src/cam/main.h | 1 +\n> > src/cam/meson.build | 10 +++\n> > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++\n> > src/cam/sdl_sink.h | 40 ++++++++++++\n> > 6 files changed, 190 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> > #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..406d61bc 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \"display\", ArgumentOptional, \"connector\", false,\n> > OptCamera);\n> > #endif\n> > +\n\nYou can drop the blank line here.\n\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> > \"If the file name ends with a '/', it sets the directory in which\\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> > OptFile = 'F',\n> > OptHelp = 'h',\n> > OptInfo = 'I',\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index e8e2ae57..44202ef0 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> > +cam_sources += files([\n> > + 'sdl_sink.cpp'\n> > +])\n\nThe contents of the if..endif should be indented.\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..480df1b4\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.cpp\n> > @@ -0,0 +1,125 @@\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 \"image.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +SDLSink::SDLSink()\n> > +{\n> > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> > +}\n> > +\n> > +SDLSink::~SDLSink()\n> > +{\n> > + SDL_Quit();\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 ((ret = SDL_Init(SDL_INIT_VIDEO))) {\n> > + std::cout << \"Could not initialize SDL - \" << SDL_GetError() << \"\\n\";\n> > + return ret;\n> > + }\n> > +\n> > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width,\n> > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n\nAs this shows the window, it may be more appropriate to do this in\nstart(). Or maybe it can be created hidden, and shown in start() ?\n\n> > + if (!sdlScreen_) {\n> > + std::cerr << \"SDL: could not create window - exiting: \" << SDL_GetError() << \"\\n\";\n\nstd::endl instead of \"\\n\".\n\n> > + return -1;\n> \n> is an errno more appropriate?\n> Same for the others..\n> \n> > + }\n> > +\n> > + sdlRenderer_ = SDL_CreateRenderer(\n> > + sdlScreen_, -1, 0);\n> > + if (!sdlRenderer_) {\n> > + std::cerr << \"SDL_CreateRenderer Error\\n\";\n> > + return -2;\n\nIs there a need to clean previous steps when an error occurs, or is\nSDL_Quit() enough ? We aim to have no memory leak reported by valgrind\nat program exit.\n\n> > + }\n> > +\n> > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width,\n> > + sCfg.size.height);\n> > +\n> > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n\nPlease don't assign and test in the same statement.\n\n> > + pf = SDL_PIXELFORMAT_YUY2;\n> > + }\n> \n> Is this some sort of conversion that most fourccs are compatible\n> 'except' YUY2? I'd be wary if there is even a single difference, and\n> it might be better to put in an explicit conversion table or map?\n> \n> Particularly as libcamera fourcc's are based on DRM fourccs... are SDL\n> fourcc's defined as directly compatible there? I.e. there are frequently\n> differences in byte ordering on the RGB/BGR formats that cause issues.\n> \n> > +\n> > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"))) {\n\nNo need to assign to ret.\n\n> > + std::cerr << \"error: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> > + return -3;\n> > + }\n> > +\n> > + sdlTexture_ =\n> > + SDL_CreateTexture(sdlRenderer_, pf,\n> > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width,\n> > + sCfg.size.height);\n> > + sdlRect_.w = sCfg.size.width;\n> > + sdlRect_.h = sCfg.size.height;\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> > + return true;\n> > +}\n> > +\n> > +void SDLSink::writeBuffer(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(), sdlRect_.w * 2);\n\nDoes this cause a copy ? If so, is there a zero-copy option ?\n\n> > + SDL_RenderClear(sdlRenderer_);\n> > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> > + SDL_RenderPresent(sdlRenderer_);\n> > + SDL_Event e;\n> > + while (SDL_PollEvent(&e)) {\n> > + if (e.type == SDL_QUIT) { // click close icon then quit\n> > + kill(getpid(), SIGINT);\n> > + }\n> > + };\n> \n> This seems like an odd place to be doing the event loop polling. I guess\n> it's intentional to be able to 'hook' it in somewhere?\n> \n> I'd be tempted to pull it out to a separate function at least.\n\nIt would be nice to integrate that with the main event loop.\n\n> Having never looked at SDL code before, I must say, - that looks like\n> it's really quite easy to pull together a window and display!\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..f4f843fa\n> > --- /dev/null\n> > +++ b/src/cam/sdl_sink.h\n> > @@ -0,0 +1,40 @@\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> > +\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> > +\n> > + std::map> mappedBuffers_;\n> > + SDL_Window *sdlScreen_;\n> > + SDL_Renderer *sdlRenderer_;\n> > + SDL_Texture *sdlTexture_;\n> > + SDL_Rect sdlRect_;\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 F1607C0F1B\n\tfor ;\n\tWed, 23 Mar 2022 00:36:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 549CA604C7;\n\tWed, 23 Mar 2022 01:36:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24AA6604C6\n\tfor ;\n\tWed, 23 Mar 2022 01:36:08 +0100 (CET)","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 7D6EA9DE;\n\tWed, 23 Mar 2022 01:36:07 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647995770;\n\tbh=mi9Rxg43JQwIbqQymF8EpAEGJd2/lr3dAEWEWJ9P8RA=;\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=iN72llwkoiiNIF5AblnZ/I707oBcsshKf313ZzZx3d88M2lbAgrNbCMf1srIzgmp5\n\t0R1VRqDz58uhbYpRejRzMuovSVTbFBJRybn+YU1J1FVJ0IGZgfpJtEr+gYViaUQx20\n\tVgYoO2rns+8UClUxWbL5zBKgUczrHITzvXO2UF2c8N8Gr0BFfkfGI3ezGncc+R0ZMd\n\tExXlGexpGAK2ye3EbRI/P6zr6hLmK+BiTseoOlsSFzfbhA6qrfF3mvWekKvAVoiGiP\n\tv7g2qE181X7vNXSApYteAKGPjNmfvTL1mK8ReXqZ3huZKLhxFXxbRRYOCj4vaLOmUx\n\tOlBmoYQtjdVqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647995767;\n\tbh=mi9Rxg43JQwIbqQymF8EpAEGJd2/lr3dAEWEWJ9P8RA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rlDrLdRZ8Ei9hyh1X2fVd4taLxAMtVqdEyzTEmvLtESjM7sYz+V+JDX4fk/K/2hNq\n\tP+MVD0X6euiL6M7PU0SKYJ1kkzWvn/Xrzr/Oy7HUHi59d31BsjPv10K73wwiSDHt3w\n\trfvexIzKNnHx+HjJx2RcKtMXt6lj+rJpS3bUIBNE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rlDrLdRZ\"; dkim-atps=neutral","Date":"Wed, 23 Mar 2022 02:35:50 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20220322190936.69281-1-ecurtin@redhat.com>\n\t<164799206875.506124.16534994403383923955@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164799206875.506124.16534994403383923955@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":22410,"web_url":"https://patchwork.libcamera.org/comment/22410/","msgid":"","date":"2022-03-23T14:49:53","subject":"Re: [libcamera-devel] [PATCH v2] 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 Wed, 23 Mar 2022 at 00:36, Laurent Pinchart\n wrote:\n>\n> On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Hi Eric,\n> >\n> > Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36)\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.\n> >\n> > This sounds very helpful and useful to me...\n\nYeah, I don't know if I captured in it in commit message well enough,\nbut it also behaves as a kmsdrm sink also, it's one of the reasons why\nDoom generally is so portable, many forks use SDL. It's solving a few\nproblems for me.\n\n> >\n> > > Signed-off-by: Eric Curtin \n> > > ---\n> > > Changes in v2:\n> > > - Remove hardcoded pixel format from SDL_CreateTexture call\n> > > ---\n> > > src/cam/camera_session.cpp | 8 +++\n> > > src/cam/main.cpp | 6 ++\n> > > src/cam/main.h | 1 +\n> > > src/cam/meson.build | 10 +++\n> > > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++\n> > > src/cam/sdl_sink.h | 40 ++++++++++++\n> > > 6 files changed, 190 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> > > #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..406d61bc 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > \"display\", ArgumentOptional, \"connector\", false,\n> > > OptCamera);\n> > > #endif\n> > > +\n>\n> You can drop the blank line here.\n\nSounds good.\n\n>\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> > > \"If the file name ends with a '/', it sets the directory in which\\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> > > OptFile = 'F',\n> > > OptHelp = 'h',\n> > > OptInfo = 'I',\n> > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > index e8e2ae57..44202ef0 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> > > +cam_sources += files([\n> > > + 'sdl_sink.cpp'\n> > > +])\n>\n> The contents of the if..endif should be indented.\n\nThe main reason I didn't is because in the if block above it doesn't,\nbut I can add indenting.\n\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..480df1b4\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.cpp\n> > > @@ -0,0 +1,125 @@\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 \"image.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SDLSink::SDLSink()\n> > > +{\n> > > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> > > +}\n> > > +\n> > > +SDLSink::~SDLSink()\n> > > +{\n> > > + SDL_Quit();\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 ((ret = SDL_Init(SDL_INIT_VIDEO))) {\n> > > + std::cout << \"Could not initialize SDL - \" << SDL_GetError() << \"\\n\";\n> > > + return ret;\n> > > + }\n> > > +\n> > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width,\n> > > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n>\n> As this shows the window, it may be more appropriate to do this in\n> start(). Or maybe it can be created hidden, and shown in start() ?\n\nHmmm, I think if we create a start(), it probably makes sense to leave\npixel format stuff in configure() here and do the rest in start().\n\n>\n> > > + if (!sdlScreen_) {\n> > > + std::cerr << \"SDL: could not create window - exiting: \" << SDL_GetError() << \"\\n\";\n>\n> std::endl instead of \"\\n\".\n>\n> > > + return -1;\n> >\n> > is an errno more appropriate?\n> > Same for the others..\n> >\n> > > + }\n> > > +\n> > > + sdlRenderer_ = SDL_CreateRenderer(\n> > > + sdlScreen_, -1, 0);\n> > > + if (!sdlRenderer_) {\n> > > + std::cerr << \"SDL_CreateRenderer Error\\n\";\n> > > + return -2;\n>\n> Is there a need to clean previous steps when an error occurs, or is\n> SDL_Quit() enough ? We aim to have no memory leak reported by valgrind\n> at program exit.\n\nI'm adding one more cleanup to the destructor. Checked with valgrind\nand address sanitizer for as many scenarios I could recreate.\n\n>\n> > > + }\n> > > +\n> > > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width,\n> > > + sCfg.size.height);\n> > > +\n> > > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n>\n> Please don't assign and test in the same statement.\n\nok.\n\n>\n> > > + pf = SDL_PIXELFORMAT_YUY2;\n> > > + }\n> >\n> > Is this some sort of conversion that most fourccs are compatible\n> > 'except' YUY2? I'd be wary if there is even a single difference, and\n> > it might be better to put in an explicit conversion table or map?\n\nThe thing is... SDL has maps for this stuff, so I didn't want to\nduplicate effort by creating extra maps. These enums are compatible in\nthat they are both 4 chars encoded in a single 32-bit int, but\nsometimes they may use a different 4 char combo as in the YUY2/YUYV\ncase. The SDL_GetPixelFormatName identifies if the 4 chars are not\nlisted as a compatible SDL pixel format. I'd be open to extending this\nto a libcamera map as we encounter more that don't exactly match. Two\nexamples of mapping code that pre-exist:\n\n$ wc -l include/SDL_pixels.h\n644 include/SDL_pixels.h\n\n$ wc -l include/linux/drm_fourcc.h\n1479 include/linux/drm_fourcc.h\n\nA fully complete libdrm -> SDL2 pixel format map, feels like an SDL2\npatch rather than a libcamera patch.\n\n> >\n> > Particularly as libcamera fourcc's are based on DRM fourccs... are SDL\n> > fourcc's defined as directly compatible there? I.e. there are frequently\n> > differences in byte ordering on the RGB/BGR formats that cause issues.\n\nI mitigated against all issues by returning a negative error code at\nthis point and printing:\n\n\"error: SDL_PIXELFORMAT_UNKNOWN, no\"...\n\nI'm open to suggestions of what that error message might be, maybe it\nshould add a suggestion to add to the libcamera DRM -> SDL2 map? Still\nfeels like this map in the long term is more in the scope of either\nlibdrm or SDL2.\n\n> >\n> > > +\n> > > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"))) {\n>\n> No need to assign to ret.\n\nOk\n\n>\n> > > + std::cerr << \"error: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> > > + return -3;\n> > > + }\n> > > +\n> > > + sdlTexture_ =\n> > > + SDL_CreateTexture(sdlRenderer_, pf,\n> > > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width,\n> > > + sCfg.size.height);\n> > > + sdlRect_.w = sCfg.size.width;\n> > > + sdlRect_.h = sCfg.size.height;\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> > > + return true;\n> > > +}\n> > > +\n> > > +void SDLSink::writeBuffer(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(), sdlRect_.w * 2);\n>\n> Does this cause a copy ? If so, is there a zero-copy option ?\n\nNo evidence of zero-copy from what I can see, I'll keep looking.\n\n>\n> > > + SDL_RenderClear(sdlRenderer_);\n> > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> > > + SDL_RenderPresent(sdlRenderer_);\n> > > + SDL_Event e;\n> > > + while (SDL_PollEvent(&e)) {\n> > > + if (e.type == SDL_QUIT) { // click close icon then quit\n> > > + kill(getpid(), SIGINT);\n> > > + }\n> > > + };\n> >\n> > This seems like an odd place to be doing the event loop polling. I guess\n> > it's intentional to be able to 'hook' it in somewhere?\n> >\n> > I'd be tempted to pull it out to a separate function at least.\n\nYup will do. I also think calling this outside the for loop in\nprocessRequest might be a little better. It's only actually needed in\nthe desktop application case when clicking the x button and resizing\nthe window so far. SDL_QUIT means user clicked x or quit.\n\n>\n> It would be nice to integrate that with the main event loop.\n\nMight need assistance to do that in a clean way.\n\n>\n> > Having never looked at SDL code before, I must say, - that looks like\n> > it's really quite easy to pull together a window and display!\n\nYeah it saves you writing 1000s of lines of code alright so one can\nfocus a little more on the camera stuff.\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..f4f843fa\n> > > --- /dev/null\n> > > +++ b/src/cam/sdl_sink.h\n> > > @@ -0,0 +1,40 @@\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> > > +\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> > > +\n> > > + std::map> mappedBuffers_;\n> > > + SDL_Window *sdlScreen_;\n> > > + SDL_Renderer *sdlRenderer_;\n> > > + SDL_Texture *sdlTexture_;\n> > > + SDL_Rect sdlRect_;\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 7952CC0F1B\n\tfor ;\n\tWed, 23 Mar 2022 14:50:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8891604DC;\n\tWed, 23 Mar 2022 15:50:14 +0100 (CET)","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 B2E19604C4\n\tfor ;\n\tWed, 23 Mar 2022 15:50:12 +0100 (CET)","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-443-QH6ZLw1DOnK9ScdMccRgfg-1; Wed, 23 Mar 2022 10:50:10 -0400","by mail-oo1-f71.google.com with SMTP id\n\th42-20020a4a942d000000b00321739192d0so1051095ooi.8\n\tfor ;\n\tWed, 23 Mar 2022 07:50:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648047014;\n\tbh=ZCB/Le4YSJMXWP43FMktIO9GXtiYrrH5bb7PpI2iHXQ=;\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=Qfs9A9M9vAZw68NQV0IWzunnmC2ztqLoA4VdKpjt/CKOCKWjiowHdK/2PH62WhOrX\n\t3fDdPBWT9m11YqT3ct764hPnZURRgrpzibqklx98ySZ4fvvgnBYdCRkeUsohDenFu5\n\tew0yiMuV8YX1iMDSRb8VK0kHDVgPejAgxZH31yW/FCZX/psbWGFPAM6uahNIipNI6D\n\tetdpy7azyxMSlrg7TLnDs/Po4CelRzEsB9HrB9IQ5+xXvDPawuZoi4AAJp67cd+Ud6\n\t4A8reFh6AVwwTFsSjihae8eXhsYoNFVaWwp0b4icBNxin8Tf3UkhhzZQb3dsWYUGEh\n\t48vPCa3VzM37A==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1648047011;\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=m/maVhHBbpOtY+aBJk6Te9SlRwKh6Xbp07CoENKajCc=;\n\tb=bdn2ADvtxNf5G06QQcRfspat5N33P8eSK5tB7OtMCNC+hK8C7cHW6T1tgdwzwrohCjbHPu\n\tHY2QY+9raEZhrHH1CgTv9clF18+Ksr/G1LgcSVGU+kLuJasY6hP6ePRSesHHpoNsHKBhxE\n\thWvcBxffgfIFNbfl1X2m7KhNc+Zy+SU="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"bdn2ADvt\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ecurtin@redhat.com"],"X-MC-Unique":"QH6ZLw1DOnK9ScdMccRgfg-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=m/maVhHBbpOtY+aBJk6Te9SlRwKh6Xbp07CoENKajCc=;\n\tb=gnj523Vctd3v75bRKHGG5U3yZAo34FwdIYA1MkmpPhfJla16W6bht9RhJrl/N6kIWL\n\t/eeoqDYJeTeKllZSS7wEcvuceQRghvv6nhLp/8z7im+S+FamhBozMsmoVLObo54GNnLf\n\tJjeP9bWNUYlIFzFNgcPG//QJKnw33SJSKauu8nuDnrQRrQ4arPK5MrIZyBNs9oiDWiUv\n\tP1JGIKy5nlVuoDE8R8coL7PouQED/r92zKmPnXUQMXi/kL0wGwESkHv6UBzXwz2YOiF0\n\tWicNuIL3uX3w/d7BLFErpFWx/OT+ty0UGXwoY06xY+f6Usi0qzqd+gEaWbOa7rKAtV0Z\n\tv3CA==","X-Gm-Message-State":"AOAM531wGRI1NXjkmsQJxLMxduUH+nQjKXd4CA9Tvd+Jm9JjOyzmftxR\n\tK0+A/KNfxkkRnhi03bUGfQVao6HJ+R77MuUZuelG1/gJk4bDI84uVVZ5lT6XNFUmA5F6tRSiZyi\n\tMHCFadzV7Nk5kNJOFp/0CFhy2HfDJlt7hnPzRdRLgpuXcKI9lvA==","X-Received":["by 2002:a05:6870:5a4:b0:da:b3f:3206 with SMTP id\n\tm36-20020a05687005a400b000da0b3f3206mr52922oap.182.1648047009287; \n\tWed, 23 Mar 2022 07:50:09 -0700 (PDT)","by 2002:a05:6870:5a4:b0:da:b3f:3206 with SMTP id\n\tm36-20020a05687005a400b000da0b3f3206mr52899oap.182.1648047008906;\n\tWed, 23 Mar 2022 07:50:08 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJx+okLRLdM5bh3KeimWss8XoGvKfVmB5TYyDZFDw22tR2HTLn7zBUDR4jvPEag+8frcxDyq5he9h6d0u5q+oYc=","MIME-Version":"1.0","References":"<20220322190936.69281-1-ecurtin@redhat.com>\n\t<164799206875.506124.16534994403383923955@Monstersaurus>\n\t","In-Reply-To":"","Date":"Wed, 23 Mar 2022 14:49:53 +0000","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 v2] 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":22440,"web_url":"https://patchwork.libcamera.org/comment/22440/","msgid":"","date":"2022-03-24T16:30:42","subject":"Re: [libcamera-devel] [PATCH v2] 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 Wed, Mar 23, 2022 at 02:49:53PM +0000, Eric Curtin wrote:\n> On Wed, 23 Mar 2022 at 00:36, Laurent Pinchart wrote:\n> > On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36)\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.\n> > >\n> > > This sounds very helpful and useful to me...\n> \n> Yeah, I don't know if I captured in it in commit message well enough,\n> but it also behaves as a kmsdrm sink also, it's one of the reasons why\n> Doom generally is so portable, many forks use SDL. It's solving a few\n> problems for me.\n> \n> > > > Signed-off-by: Eric Curtin \n> > > > ---\n> > > > Changes in v2:\n> > > > - Remove hardcoded pixel format from SDL_CreateTexture call\n> > > > ---\n> > > > src/cam/camera_session.cpp | 8 +++\n> > > > src/cam/main.cpp | 6 ++\n> > > > src/cam/main.h | 1 +\n> > > > src/cam/meson.build | 10 +++\n> > > > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++\n> > > > src/cam/sdl_sink.h | 40 ++++++++++++\n> > > > 6 files changed, 190 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> > > > #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..406d61bc 100644\n> > > > --- a/src/cam/main.cpp\n> > > > +++ b/src/cam/main.cpp\n> > > > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > > \"display\", ArgumentOptional, \"connector\", false,\n> > > > OptCamera);\n> > > > #endif\n> > > > +\n> >\n> > You can drop the blank line here.\n> \n> Sounds good.\n> \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> > > > \"If the file name ends with a '/', it sets the directory in which\\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> > > > OptFile = 'F',\n> > > > OptHelp = 'h',\n> > > > OptInfo = 'I',\n> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > > > index e8e2ae57..44202ef0 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> > > > +cam_sources += files([\n> > > > + 'sdl_sink.cpp'\n> > > > +])\n> >\n> > The contents of the if..endif should be indented.\n> \n> The main reason I didn't is because in the if block above it doesn't,\n> but I can add indenting.\n\nOops. I'll fix that.\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..480df1b4\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.cpp\n> > > > @@ -0,0 +1,125 @@\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 \"image.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SDLSink::SDLSink()\n> > > > +{\n> > > > + memset(&sdlRect_, 0, sizeof(sdlRect_));\n> > > > +}\n> > > > +\n> > > > +SDLSink::~SDLSink()\n> > > > +{\n> > > > + SDL_Quit();\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 ((ret = SDL_Init(SDL_INIT_VIDEO))) {\n> > > > + std::cout << \"Could not initialize SDL - \" << SDL_GetError() << \"\\n\";\n> > > > + return ret;\n> > > > + }\n> > > > +\n> > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);\n> > > > + sdlScreen_ = SDL_CreateWindow(\"\", SDL_WINDOWPOS_UNDEFINED,\n> > > > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width,\n> > > > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);\n> >\n> > As this shows the window, it may be more appropriate to do this in\n> > start(). Or maybe it can be created hidden, and shown in start() ?\n> \n> Hmmm, I think if we create a start(), it probably makes sense to leave\n> pixel format stuff in configure() here and do the rest in start().\n\nAlso, if we capture from two cameras, and set the --sdl option for both,\nwe'll end up calling SDL_Init() multiple times, I suppose that won't\nwork nicely.\n\nThe KMS sink has the same issue, so you don't need to solve it here.\n\n> > > > + if (!sdlScreen_) {\n> > > > + std::cerr << \"SDL: could not create window - exiting: \" << SDL_GetError() << \"\\n\";\n> >\n> > std::endl instead of \"\\n\".\n> >\n> > > > + return -1;\n> > >\n> > > is an errno more appropriate?\n> > > Same for the others..\n> > >\n> > > > + }\n> > > > +\n> > > > + sdlRenderer_ = SDL_CreateRenderer(\n> > > > + sdlScreen_, -1, 0);\n> > > > + if (!sdlRenderer_) {\n> > > > + std::cerr << \"SDL_CreateRenderer Error\\n\";\n> > > > + return -2;\n> >\n> > Is there a need to clean previous steps when an error occurs, or is\n> > SDL_Quit() enough ? We aim to have no memory leak reported by valgrind\n> > at program exit.\n> \n> I'm adding one more cleanup to the destructor. Checked with valgrind\n> and address sanitizer for as many scenarios I could recreate.\n\nThank you.\n\n> > > > + }\n> > > > +\n> > > > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width,\n> > > > + sCfg.size.height);\n> > > > +\n> > > > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();\n> > > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {\n> >\n> > Please don't assign and test in the same statement.\n> \n> ok.\n> \n> > > > + pf = SDL_PIXELFORMAT_YUY2;\n> > > > + }\n> > >\n> > > Is this some sort of conversion that most fourccs are compatible\n> > > 'except' YUY2? I'd be wary if there is even a single difference, and\n> > > it might be better to put in an explicit conversion table or map?\n> \n> The thing is... SDL has maps for this stuff, so I didn't want to\n> duplicate effort by creating extra maps. These enums are compatible in\n> that they are both 4 chars encoded in a single 32-bit int, but\n> sometimes they may use a different 4 char combo as in the YUY2/YUYV\n> case. The SDL_GetPixelFormatName identifies if the 4 chars are not\n> listed as a compatible SDL pixel format. I'd be open to extending this\n> to a libcamera map as we encounter more that don't exactly match. Two\n> examples of mapping code that pre-exist:\n> \n> $ wc -l include/SDL_pixels.h\n> 644 include/SDL_pixels.h\n> \n> $ wc -l include/linux/drm_fourcc.h\n> 1479 include/linux/drm_fourcc.h\n> \n> A fully complete libdrm -> SDL2 pixel format map, feels like an SDL2\n> patch rather than a libcamera patch.\n> \n> > >\n> > > Particularly as libcamera fourcc's are based on DRM fourccs... are SDL\n> > > fourcc's defined as directly compatible there? I.e. there are frequently\n> > > differences in byte ordering on the RGB/BGR formats that cause issues.\n> \n> I mitigated against all issues by returning a negative error code at\n> this point and printing:\n> \n> \"error: SDL_PIXELFORMAT_UNKNOWN, no\"...\n> \n> I'm open to suggestions of what that error message might be, maybe it\n> should add a suggestion to add to the libcamera DRM -> SDL2 map? Still\n> feels like this map in the long term is more in the scope of either\n> libdrm or SDL2.\n> \n> > > > +\n> > > > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), \"SDL_PIXELFORMAT_UNKNOWN\"))) {\n> >\n> > No need to assign to ret.\n> \n> Ok\n> \n> > > > + std::cerr << \"error: SDL_PIXELFORMAT_UNKNOWN, no \" << sCfg.pixelFormat.toString() << \" support\\n\";\n> > > > + return -3;\n> > > > + }\n> > > > +\n> > > > + sdlTexture_ =\n> > > > + SDL_CreateTexture(sdlRenderer_, pf,\n> > > > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width,\n> > > > + sCfg.size.height);\n> > > > + sdlRect_.w = sCfg.size.width;\n> > > > + sdlRect_.h = sCfg.size.height;\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> > > > + return true;\n> > > > +}\n> > > > +\n> > > > +void SDLSink::writeBuffer(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(), sdlRect_.w * 2);\n> >\n> > Does this cause a copy ? If so, is there a zero-copy option ?\n> \n> No evidence of zero-copy from what I can see, I'll keep looking.\n\nNo problem, I was just wondering.\n\n> > > > + SDL_RenderClear(sdlRenderer_);\n> > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);\n> > > > + SDL_RenderPresent(sdlRenderer_);\n> > > > + SDL_Event e;\n> > > > + while (SDL_PollEvent(&e)) {\n> > > > + if (e.type == SDL_QUIT) { // click close icon then quit\n> > > > + kill(getpid(), SIGINT);\n> > > > + }\n> > > > + };\n> > >\n> > > This seems like an odd place to be doing the event loop polling. I guess\n> > > it's intentional to be able to 'hook' it in somewhere?\n> > >\n> > > I'd be tempted to pull it out to a separate function at least.\n> \n> Yup will do. I also think calling this outside the for loop in\n> processRequest might be a little better. It's only actually needed in\n> the desktop application case when clicking the x button and resizing\n> the window so far. SDL_QUIT means user clicked x or quit.\n> \n> > It would be nice to integrate that with the main event loop.\n> \n> Might need assistance to do that in a clean way.\n\nIt seems it will get messy, SDL doesn't have the ability to expose\nevents through a file descriptor :-S I think we can get libevent to call\nuse back every time it wakes up by adding an event that is neither a\nread or write event:\n\n\tEventLoop::addEvent(-1, 0, callback);\n\nIn the callback function, you can then call SDL_PollEvent() in a while\nloop like above.\n\nThis class doesn't have access to the event loop though, so it would\nrequire a bit of refactoring.\n\n> > > Having never looked at SDL code before, I must say, - that looks like\n> > > it's really quite easy to pull together a window and display!\n> \n> Yeah it saves you writing 1000s of lines of code alright so one can\n> focus a little more on the camera stuff.\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..f4f843fa\n> > > > --- /dev/null\n> > > > +++ b/src/cam/sdl_sink.h\n> > > > @@ -0,0 +1,40 @@\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> > > > +\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> > > > +\n> > > > + std::map> mappedBuffers_;\n> > > > + SDL_Window *sdlScreen_;\n> > > > + SDL_Renderer *sdlRenderer_;\n> > > > + SDL_Texture *sdlTexture_;\n> > > > + SDL_Rect sdlRect_;\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 3C310C0F1B\n\tfor ;\n\tThu, 24 Mar 2022 16:30:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93261604D5;\n\tThu, 24 Mar 2022 17:30:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3464160397\n\tfor ;\n\tThu, 24 Mar 2022 17:30:44 +0100 (CET)","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 A854D1844;\n\tThu, 24 Mar 2022 17:30:43 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648139445;\n\tbh=OrFdR9SP1/o+Ur+5PLpBM/0UDKwm7ee04nN9rmlZ+Nc=;\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=3p+OYkAVC9zXdOPDrpUEde2Lx9km4K/AZDoHPXFnoshM1vftOSKuZeHX9kph8f0D6\n\tgZ0blLJy0z0y9i5AuR0ixfdpFjjh3UnB3uBQN0E0lf+DSsPzxiLQqHuPqnD0kXU3E7\n\trJc2PrXuJp2UCFrTN8cR+rdzkMHSYvAUojJ0X8dPyrD10AE4lZECpkFAWLGcV0Ukgj\n\tJlXNj/RNimSIJOYX0LVsMGQmfWU+2YOmGmoh/wXJHbEKhx4oybkEtDyMMavQ3aG1l2\n\t7NISS5l1zRhYg8sv74MagbNkZV1ZlXVaBrDYDghnZrcy9xNnmX9SmUWDBxU3hqJ7tX\n\tOpwSLA+dhi5Nw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648139443;\n\tbh=OrFdR9SP1/o+Ur+5PLpBM/0UDKwm7ee04nN9rmlZ+Nc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Twn9wUAazNPk07AVva5jxtku2+W3k6Q1MMf2NDJITm3IcLk9yd1e4PUkMDBShScnu\n\taWXDw6BzZHmwpF6lgwCrLydmj7CTr7dV7uBKskcq0HrNuFfBysCPaPAXplhFUOrT/r\n\tLylaeIJfWaAqvHIXyauUV0prColZIun40l+LDbBE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Twn9wUAa\"; dkim-atps=neutral","Date":"Thu, 24 Mar 2022 18:30:42 +0200","To":"Eric Curtin ","Message-ID":"","References":"<20220322190936.69281-1-ecurtin@redhat.com>\n\t<164799206875.506124.16534994403383923955@Monstersaurus>\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 v2] 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\" "}}]