| Message ID | 20220505151851.2421-3-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50) > This adds more portability to existing cam sinks. You can pass a > YUYV camera buffer and SDL will handle the pixel buffer conversion > to the display. This allows cam reference implementation to display > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam > reference implementation, to run as a desktop application in wayland or > x11. SDL also has support for Android and ChromeOS which has not been > tested. Also tested on simpledrm raspberry pi 4 framebuffer > successfully where existing kms sink did not work. Can also be used as > kmsdrm sink. Only supports one camera stream at present. > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/camera_session.cpp | 8 ++ > src/cam/main.cpp | 4 + > src/cam/main.h | 1 + > src/cam/meson.build | 12 +++ > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++ > src/cam/sdl_sink.h | 48 +++++++++ > src/cam/sdl_texture.cpp | 32 ++++++ > src/cam/sdl_texture.h | 23 +++++ > src/cam/sdl_texture_yuyv.cpp | 13 +++ > src/cam/sdl_texture_yuyv.h | 10 ++ Given the number of sdl_ files here I wonder if they should be under src/cam/sdl/.... But not crucial to this patch. I can't test this right now - but I'm looking forward to seeing a preview on cam ... so I'll hopefully test this tomorrow. > 10 files changed, 343 insertions(+) > create mode 100644 src/cam/sdl_sink.cpp > create mode 100644 src/cam/sdl_sink.h > create mode 100644 src/cam/sdl_texture.cpp > create mode 100644 src/cam/sdl_texture.h > create mode 100644 src/cam/sdl_texture_yuyv.cpp > create mode 100644 src/cam/sdl_texture_yuyv.h > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > index efffafbf..71e6bd60 100644 > --- a/src/cam/camera_session.cpp > +++ b/src/cam/camera_session.cpp > @@ -19,6 +19,9 @@ > #ifdef HAVE_KMS > #include "kms_sink.h" > #endif > +#ifdef HAVE_SDL > +#include "sdl_sink.h" > +#endif > #include "main.h" > #include "stream_options.h" > > @@ -186,6 +189,11 @@ int CameraSession::start() > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > #endif > > +#ifdef HAVE_SDL > + if (options_.isSet(OptSDL)) > + sink_ = std::make_unique<SDLSink>(); > +#endif > + It looks like the precedence of which sink really gets used if multiples are set is simply the order in this file that they get set. But I think that's ok for the moment. Not sure we could do much else right now without extending to support multiple sinks which is out of scope here. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > if (options_.isSet(OptFile)) { > if (!options_[OptFile].toString().empty()) > sink_ = std::make_unique<FileSink>(streamNames_, > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index c7f664b9..fd3108b0 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > "Display viewfinder through DRM/KMS on specified connector", > "display", ArgumentOptional, "connector", false, > OptCamera); > +#endif > +#ifdef HAVE_SDL > + parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL", > + "sdl", ArgumentNone, "", false, OptCamera); > #endif > parser.addOption(OptFile, OptionString, > "Write captured frames to disk\n" > diff --git a/src/cam/main.h b/src/cam/main.h > index 62f7bbc9..2b285808 100644 > --- a/src/cam/main.h > +++ b/src/cam/main.h > @@ -18,6 +18,7 @@ enum { > OptListProperties = 'p', > OptMonitor = 'm', > OptStream = 's', > + OptSDL = 'S', > OptListControls = 256, > OptStrictFormats = 257, > OptMetadata = 258, > diff --git a/src/cam/meson.build b/src/cam/meson.build > index 5bab8c9e..3032730b 100644 > --- a/src/cam/meson.build > +++ b/src/cam/meson.build > @@ -32,11 +32,23 @@ if libdrm.found() > ]) > endif > > +libsdl2 = dependency('SDL2', required : false) > + > +if libsdl2.found() > + cam_cpp_args += ['-DHAVE_SDL'] > + cam_sources += files([ > + 'sdl_sink.cpp', > + 'sdl_texture.cpp', > + 'sdl_texture_yuyv.cpp' > + ]) > +endif > + > cam = executable('cam', cam_sources, > dependencies : [ > libatomic, > libcamera_public, > libdrm, > + libsdl2, > libevent, > ], > cpp_args : cam_cpp_args, > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > new file mode 100644 > index 00000000..6fbeaf56 > --- /dev/null > +++ b/src/cam/sdl_sink.cpp > @@ -0,0 +1,192 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * sdl_sink.cpp - SDL Sink > + */ > + > +#include "sdl_sink.h" > + > +#include <assert.h> > +#include <fcntl.h> > +#include <iomanip> > +#include <iostream> > +#include <signal.h> > +#include <sstream> > +#include <string.h> > +#include <unistd.h> > + > +#include <libcamera/camera.h> > +#include <libcamera/formats.h> > + > +#include "sdl_texture_yuyv.h" > + > +#include "event_loop.h" > +#include "image.h" > + > +using namespace libcamera; > + > +SDLSink::SDLSink() > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}), > + sdlInit_(false) > +{ > +} > + > +SDLSink::~SDLSink() > +{ > + stop(); > +} > + > +int SDLSink::configure(const libcamera::CameraConfiguration &config) > +{ > + int ret = FrameSink::configure(config); > + if (ret < 0) > + return ret; > + > + if (config.size() > 1) { > + std::cerr > + << "SDL sink only supports one camera stream at present, streaming first camera stream" > + << std::endl; > + } else if (config.empty()) { > + std::cerr << "Require at least one camera stream to process" > + << std::endl; > + return -EINVAL; > + } > + > + const libcamera::StreamConfiguration &cfg = config.at(0); > + sdlRect_.w = cfg.size.width; > + sdlRect_.h = cfg.size.height; > + > + switch (cfg.pixelFormat) { > + case libcamera::formats::YUYV: > + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_); > + break; > + default: > + std::cerr << "Unsupported pixel format " > + << cfg.pixelFormat.toString() << std::endl; > + return -EINVAL; > + }; > + > + return 0; > +} > + > +int SDLSink::start() > +{ > + int ret = SDL_Init(SDL_INIT_VIDEO); > + if (ret) { > + std::cerr << "Failed to initialize SDL: " << SDL_GetError() > + << std::endl; > + return ret; > + } > + > + sdlInit_ = true; > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, > + sdlRect_.h, > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > + if (!sdlWindow_) { > + std::cerr << "Failed to create SDL window: " << SDL_GetError() > + << std::endl; > + return -EINVAL; > + } > + > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > + if (!sdlRenderer_) { > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() > + << std::endl; > + return -EINVAL; > + } > + > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > + std::cerr << "Failed to set SDL render logical size: " > + << SDL_GetError() << std::endl; > + } > + > + ret = sdlTexture_->create(sdlRenderer_); > + if (ret) { > + return ret; > + } > + > + EventLoop::instance()->addTimerEvent( > + 10ms, std::bind(&SDLSink::processSDLEvents, this)); > + > + return 0; > +} > + > +int SDLSink::stop() > +{ > + if (sdlTexture_) { > + sdlTexture_->destroy(); > + sdlTexture_ = nullptr; > + } > + > + if (sdlRenderer_) { > + SDL_DestroyRenderer(sdlRenderer_); > + sdlRenderer_ = nullptr; > + } > + > + if (sdlWindow_) { > + SDL_DestroyWindow(sdlWindow_); > + sdlWindow_ = nullptr; > + } > + > + if (sdlInit_) { > + SDL_Quit(); > + sdlInit_ = false; > + } > + > + return FrameSink::stop(); > +} > + > +void SDLSink::mapBuffer(FrameBuffer *buffer) > +{ > + std::unique_ptr<Image> image = > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly); > + assert(image != nullptr); > + > + mappedBuffers_[buffer] = std::move(image); > +} > + > +bool SDLSink::processRequest(Request *request) > +{ > + for (auto [stream, buffer] : request->buffers()) { > + renderBuffer(buffer); > + break; /* to be expanded to launch SDL window per buffer */ > + } > + > + return true; > +} > + > +/* > + * Process SDL events, required for things like window resize and quit button > + */ > +void SDLSink::processSDLEvents() > +{ > + for (SDL_Event e; SDL_PollEvent(&e);) { > + if (e.type == SDL_QUIT) { > + /* Click close icon then quit */ > + EventLoop::instance()->exit(0); > + } > + } > +} > + > +void SDLSink::renderBuffer(FrameBuffer *buffer) > +{ > + Image *image = mappedBuffers_[buffer].get(); > + > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > + const FrameMetadata::Plane &meta = > + buffer->metadata().planes()[i]; > + > + Span<uint8_t> data = image->data(i); > + if (meta.bytesused > data.size()) > + std::cerr << "payload size " << meta.bytesused > + << " larger than plane size " << data.size() > + << std::endl; > + > + sdlTexture_->update(data); > + } > + > + SDL_RenderClear(sdlRenderer_); > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr); > + SDL_RenderPresent(sdlRenderer_); > +} > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > new file mode 100644 > index 00000000..37dd9f7e > --- /dev/null > +++ b/src/cam/sdl_sink.h > @@ -0,0 +1,48 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * sdl_sink.h - SDL Sink > + */ > + > +#pragma once > + > +#include <map> > +#include <memory> > +#include <string> > + > +#include <libcamera/stream.h> > + > +#include <SDL2/SDL.h> > + > +#include "frame_sink.h" > +#include "sdl_texture.h" > + > +class Image; > + > +class SDLSink : public FrameSink > +{ > +public: > + SDLSink(); > + ~SDLSink(); > + > + int configure(const libcamera::CameraConfiguration &config) override; > + int start() override; > + int stop() override; > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > + > + bool processRequest(libcamera::Request *request) override; > + > +private: > + void renderBuffer(libcamera::FrameBuffer *buffer); > + void processSDLEvents(); > + > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> > + mappedBuffers_; > + > + std::unique_ptr<SDLTexture> sdlTexture_; > + > + SDL_Window *sdlWindow_; > + SDL_Renderer *sdlRenderer_; > + SDL_Rect sdlRect_; > + SDL_PixelFormatEnum pixelFormat_; > + bool sdlInit_; > +}; > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp > new file mode 100644 > index 00000000..b39080c9 > --- /dev/null > +++ b/src/cam/sdl_texture.cpp > @@ -0,0 +1,32 @@ > +#include "sdl_texture.h" > + > +#include <iostream> > + > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > + const int pitch) > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch) > +{ > +} > + > +SDLTexture::~SDLTexture() > +{ > +} > + > +int SDLTexture::create(SDL_Renderer *sdlRenderer_) > +{ > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, > + sdlRect_.h); > + if (!ptr_) { > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() > + << std::endl; > + return -ENOMEM; > + } > + > + return 0; > +} > + > +void SDLTexture::destroy() > +{ > + SDL_DestroyTexture(ptr_); > +} > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > new file mode 100644 > index 00000000..f9525f7f > --- /dev/null > +++ b/src/cam/sdl_texture.h > @@ -0,0 +1,23 @@ > +#pragma once > + > +#include <SDL2/SDL.h> > + > +#include "image.h" > + > +class SDLTexture > +{ > +public: > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > + const int pitch); > + virtual ~SDLTexture(); > + int create(SDL_Renderer *sdlRenderer_); > + void destroy(); > + virtual void update(const libcamera::Span<uint8_t> &data) = 0; > + > + SDL_Texture *ptr_; > + > +protected: > + const SDL_Rect &sdlRect_; > + SDL_PixelFormatEnum pixelFormat_; > + const int pitch_; > +}; > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp > new file mode 100644 > index 00000000..cf519a5d > --- /dev/null > +++ b/src/cam/sdl_texture_yuyv.cpp > @@ -0,0 +1,13 @@ > +#include "sdl_texture_yuyv.h" > + > +using namespace libcamera; > + > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect) > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2)) > +{ > +} > + > +void SDLTextureYUYV::update(const Span<uint8_t> &data) > +{ > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_); > +} > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h > new file mode 100644 > index 00000000..ad70f14f > --- /dev/null > +++ b/src/cam/sdl_texture_yuyv.h > @@ -0,0 +1,10 @@ > +#pragma once > + > +#include "sdl_texture.h" > + > +class SDLTextureYUYV : public SDLTexture > +{ > +public: > + SDLTextureYUYV(const SDL_Rect &sdlRect); > + void update(const libcamera::Span<uint8_t> &data) override; > +}; > -- > 2.35.1 >
Hi Eric, Thank you for the patch. On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50) > > This adds more portability to existing cam sinks. You can pass a > > YUYV camera buffer and SDL will handle the pixel buffer conversion > > to the display. This allows cam reference implementation to display > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam > > reference implementation, to run as a desktop application in wayland or s/wayland/Wayland/ > > x11. SDL also has support for Android and ChromeOS which has not been s/x11/X11/ > > tested. Also tested on simpledrm raspberry pi 4 framebuffer s/raspberry pi/Raspberry Pi/ > > successfully where existing kms sink did not work. Can also be used as > > kmsdrm sink. Only supports one camera stream at present. > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > src/cam/camera_session.cpp | 8 ++ > > src/cam/main.cpp | 4 + > > src/cam/main.h | 1 + > > src/cam/meson.build | 12 +++ > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++ > > src/cam/sdl_sink.h | 48 +++++++++ > > src/cam/sdl_texture.cpp | 32 ++++++ > > src/cam/sdl_texture.h | 23 +++++ > > src/cam/sdl_texture_yuyv.cpp | 13 +++ > > src/cam/sdl_texture_yuyv.h | 10 ++ > > Given the number of sdl_ files here I wonder if they should be under > src/cam/sdl/.... > > But not crucial to this patch. > > I can't test this right now - but I'm looking forward to seeing a > preview on cam ... so I'll hopefully test this tomorrow. > > > 10 files changed, 343 insertions(+) > > create mode 100644 src/cam/sdl_sink.cpp > > create mode 100644 src/cam/sdl_sink.h > > create mode 100644 src/cam/sdl_texture.cpp > > create mode 100644 src/cam/sdl_texture.h > > create mode 100644 src/cam/sdl_texture_yuyv.cpp > > create mode 100644 src/cam/sdl_texture_yuyv.h > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > index efffafbf..71e6bd60 100644 > > --- a/src/cam/camera_session.cpp > > +++ b/src/cam/camera_session.cpp > > @@ -19,6 +19,9 @@ > > #ifdef HAVE_KMS > > #include "kms_sink.h" > > #endif > > +#ifdef HAVE_SDL > > +#include "sdl_sink.h" > > +#endif Please move this below in alphabetical order. > > #include "main.h" > > #include "stream_options.h" > > > > @@ -186,6 +189,11 @@ int CameraSession::start() > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > #endif > > > > +#ifdef HAVE_SDL > > + if (options_.isSet(OptSDL)) > > + sink_ = std::make_unique<SDLSink>(); > > +#endif > > + > > It looks like the precedence of which sink really gets used if multiples > are set is simply the order in this file that they get set. But I think > that's ok for the moment. Not sure we could do much else right now > without extending to support multiple sinks which is out of scope here. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > if (options_.isSet(OptFile)) { > > if (!options_[OptFile].toString().empty()) > > sink_ = std::make_unique<FileSink>(streamNames_, > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index c7f664b9..fd3108b0 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > "Display viewfinder through DRM/KMS on specified connector", > > "display", ArgumentOptional, "connector", false, > > OptCamera); > > +#endif > > +#ifdef HAVE_SDL > > + parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL", > > + "sdl", ArgumentNone, "", false, OptCamera); > > #endif I wonder if we should merge all sink arguments into one, but that can be done on top (the cam arguments syntax isn't stable yet). > > parser.addOption(OptFile, OptionString, > > "Write captured frames to disk\n" > > diff --git a/src/cam/main.h b/src/cam/main.h > > index 62f7bbc9..2b285808 100644 > > --- a/src/cam/main.h > > +++ b/src/cam/main.h > > @@ -18,6 +18,7 @@ enum { > > OptListProperties = 'p', > > OptMonitor = 'm', > > OptStream = 's', > > + OptSDL = 'S', > > OptListControls = 256, > > OptStrictFormats = 257, > > OptMetadata = 258, > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > index 5bab8c9e..3032730b 100644 > > --- a/src/cam/meson.build > > +++ b/src/cam/meson.build > > @@ -32,11 +32,23 @@ if libdrm.found() > > ]) > > endif > > > > +libsdl2 = dependency('SDL2', required : false) > > + > > +if libsdl2.found() > > + cam_cpp_args += ['-DHAVE_SDL'] > > + cam_sources += files([ > > + 'sdl_sink.cpp', > > + 'sdl_texture.cpp', > > + 'sdl_texture_yuyv.cpp' > > + ]) > > +endif > > + > > cam = executable('cam', cam_sources, > > dependencies : [ > > libatomic, > > libcamera_public, > > libdrm, > > + libsdl2, Please keep the list alphabetically ordered. > > libevent, > > ], > > cpp_args : cam_cpp_args, > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > new file mode 100644 > > index 00000000..6fbeaf56 > > --- /dev/null > > +++ b/src/cam/sdl_sink.cpp > > @@ -0,0 +1,192 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * sdl_sink.cpp - SDL Sink > > + */ > > + > > +#include "sdl_sink.h" > > + > > +#include <assert.h> > > +#include <fcntl.h> > > +#include <iomanip> > > +#include <iostream> > > +#include <signal.h> > > +#include <sstream> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include <libcamera/camera.h> > > +#include <libcamera/formats.h> > > + > > +#include "sdl_texture_yuyv.h" > > + > > +#include "event_loop.h" > > +#include "image.h" I don't think there's a need to split sdl_texture_yuyv.h and the next two headers in two groups. > > + > > +using namespace libcamera; > > + > > +SDLSink::SDLSink() > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}), > > + sdlInit_(false) > > +{ > > +} > > + > > +SDLSink::~SDLSink() > > +{ > > + stop(); > > +} > > + > > +int SDLSink::configure(const libcamera::CameraConfiguration &config) > > +{ > > + int ret = FrameSink::configure(config); > > + if (ret < 0) > > + return ret; > > + > > + if (config.size() > 1) { > > + std::cerr > > + << "SDL sink only supports one camera stream at present, streaming first camera stream" > > + << std::endl; > > + } else if (config.empty()) { > > + std::cerr << "Require at least one camera stream to process" > > + << std::endl; > > + return -EINVAL; > > + } > > + > > + const libcamera::StreamConfiguration &cfg = config.at(0); > > + sdlRect_.w = cfg.size.width; > > + sdlRect_.h = cfg.size.height; > > + > > + switch (cfg.pixelFormat) { > > + case libcamera::formats::YUYV: > > + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_); > > + break; > > + default: > > + std::cerr << "Unsupported pixel format " > > + << cfg.pixelFormat.toString() << std::endl; > > + return -EINVAL; > > + }; > > + > > + return 0; > > +} > > + > > +int SDLSink::start() > > +{ > > + int ret = SDL_Init(SDL_INIT_VIDEO); > > + if (ret) { > > + std::cerr << "Failed to initialize SDL: " << SDL_GetError() > > + << std::endl; > > + return ret; > > + } > > + > > + sdlInit_ = true; > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, > > + sdlRect_.h, > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > + if (!sdlWindow_) { > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() > > + << std::endl; > > + return -EINVAL; > > + } > > + > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > + if (!sdlRenderer_) { > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() > > + << std::endl; > > + return -EINVAL; > > + } > > + > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > + std::cerr << "Failed to set SDL render logical size: " > > + << SDL_GetError() << std::endl; > > + } /* * Not critical to set, don't return in this case, set for scaling * purposes. */ if (ret) std::cerr << "Failed to set SDL render logical size: " << SDL_GetError() << std::endl; > > + > > + ret = sdlTexture_->create(sdlRenderer_); > > + if (ret) { > > + return ret; > > + } > > + > > + EventLoop::instance()->addTimerEvent( > > + 10ms, std::bind(&SDLSink::processSDLEvents, this)); I think I mentioned before that this won't work nicely if we stop and restart. cam doesn't do so at the moment, so it doesn't have to be fixed yet, but a todo comment would be good: /* \todo Make the event cancellable to support stop/start cycles. */ EventLoop::instance()->addTimerEvent( 10ms, std::bind(&SDLSink::processSDLEvents, this)); > > + > > + return 0; > > +} > > + > > +int SDLSink::stop() > > +{ > > + if (sdlTexture_) { > > + sdlTexture_->destroy(); Can you turn SDLTexture::destroy() into a destructor ? Then you could simply write sdlTexture_ = nullptr; or sdlTexture_.reset(); > > + sdlTexture_ = nullptr; > > + } > > + > > + if (sdlRenderer_) { > > + SDL_DestroyRenderer(sdlRenderer_); > > + sdlRenderer_ = nullptr; > > + } > > + > > + if (sdlWindow_) { > > + SDL_DestroyWindow(sdlWindow_); > > + sdlWindow_ = nullptr; > > + } > > + > > + if (sdlInit_) { > > + SDL_Quit(); > > + sdlInit_ = false; > > + } > > + > > + return FrameSink::stop(); > > +} > > + > > +void SDLSink::mapBuffer(FrameBuffer *buffer) > > +{ > > + std::unique_ptr<Image> image = > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly); > > + assert(image != nullptr); > > + > > + mappedBuffers_[buffer] = std::move(image); > > +} > > + > > +bool SDLSink::processRequest(Request *request) > > +{ > > + for (auto [stream, buffer] : request->buffers()) { > > + renderBuffer(buffer); > > + break; /* to be expanded to launch SDL window per buffer */ > > + } > > + > > + return true; > > +} > > + > > +/* > > + * Process SDL events, required for things like window resize and quit button > > + */ > > +void SDLSink::processSDLEvents() > > +{ > > + for (SDL_Event e; SDL_PollEvent(&e);) { > > + if (e.type == SDL_QUIT) { > > + /* Click close icon then quit */ > > + EventLoop::instance()->exit(0); > > + } > > + } > > +} > > + > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > +{ > > + Image *image = mappedBuffers_[buffer].get(); > > + > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > + const FrameMetadata::Plane &meta = > > + buffer->metadata().planes()[i]; > > + > > + Span<uint8_t> data = image->data(i); > > + if (meta.bytesused > data.size()) > > + std::cerr << "payload size " << meta.bytesused > > + << " larger than plane size " << data.size() > > + << std::endl; > > + > > + sdlTexture_->update(data); I really don't think this will do what you expect for multi-planar formats. As multi-planar formats are not supported yet, I'd replace the loop by a single pass over plane 0, and add a /* \todo Implement support for multi-planar formats. */ > > + } > > + > > + SDL_RenderClear(sdlRenderer_); > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr); > > + SDL_RenderPresent(sdlRenderer_); > > +} > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > new file mode 100644 > > index 00000000..37dd9f7e > > --- /dev/null > > +++ b/src/cam/sdl_sink.h > > @@ -0,0 +1,48 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * sdl_sink.h - SDL Sink > > + */ > > + > > +#pragma once > > + > > +#include <map> > > +#include <memory> > > +#include <string> I think you can drop the string header. > > + > > +#include <libcamera/stream.h> > > + > > +#include <SDL2/SDL.h> > > + > > +#include "frame_sink.h" > > +#include "sdl_texture.h" You could also forward-declare the SDLTexture class and drop this header. > > + > > +class Image; > > + > > +class SDLSink : public FrameSink > > +{ > > +public: > > + SDLSink(); > > + ~SDLSink(); > > + > > + int configure(const libcamera::CameraConfiguration &config) override; > > + int start() override; > > + int stop() override; > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > + > > + bool processRequest(libcamera::Request *request) override; > > + > > +private: > > + void renderBuffer(libcamera::FrameBuffer *buffer); > > + void processSDLEvents(); > > + > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> > > + mappedBuffers_; > > + > > + std::unique_ptr<SDLTexture> sdlTexture_; > > + > > + SDL_Window *sdlWindow_; > > + SDL_Renderer *sdlRenderer_; > > + SDL_Rect sdlRect_; I'm tempted to drop the sdl prefix from variable names, up to you. > > + SDL_PixelFormatEnum pixelFormat_; > > + bool sdlInit_; > > +}; > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp > > new file mode 100644 > > index 00000000..b39080c9 > > --- /dev/null > > +++ b/src/cam/sdl_texture.cpp > > @@ -0,0 +1,32 @@ Missing SPDX tag and copyright header. Same below. > > +#include "sdl_texture.h" > > + > > +#include <iostream> > > + > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > + const int pitch) > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch) > > +{ > > +} > > + > > +SDLTexture::~SDLTexture() > > +{ > > +} > > + > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_) > > +{ > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, > > + sdlRect_.h); > > + if (!ptr_) { > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() > > + << std::endl; > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > +void SDLTexture::destroy() > > +{ > > + SDL_DestroyTexture(ptr_); > > +} > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > new file mode 100644 > > index 00000000..f9525f7f > > --- /dev/null > > +++ b/src/cam/sdl_texture.h > > @@ -0,0 +1,23 @@ > > +#pragma once > > + > > +#include <SDL2/SDL.h> > > + > > +#include "image.h" > > + > > +class SDLTexture > > +{ > > +public: > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > + const int pitch); > > + virtual ~SDLTexture(); > > + int create(SDL_Renderer *sdlRenderer_); s/sdlRenderer_/sdlRenderer/ And same here (and with sdlRect in the previous function) about the sdl prefix. > > + void destroy(); > > + virtual void update(const libcamera::Span<uint8_t> &data) = 0; > > + > > + SDL_Texture *ptr_; Is it worth making this protected and adding an accessor function ? > > + > > +protected: > > + const SDL_Rect &sdlRect_; Same here, rect_. > > + SDL_PixelFormatEnum pixelFormat_; > > + const int pitch_; > > +}; > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp > > new file mode 100644 > > index 00000000..cf519a5d > > --- /dev/null > > +++ b/src/cam/sdl_texture_yuyv.cpp > > @@ -0,0 +1,13 @@ > > +#include "sdl_texture_yuyv.h" > > + > > +using namespace libcamera; > > + > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect) > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2)) > > +{ > > +} > > + > > +void SDLTextureYUYV::update(const Span<uint8_t> &data) > > +{ > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_); > > +} > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h > > new file mode 100644 > > index 00000000..ad70f14f > > --- /dev/null > > +++ b/src/cam/sdl_texture_yuyv.h > > @@ -0,0 +1,10 @@ > > +#pragma once > > + > > +#include "sdl_texture.h" > > + > > +class SDLTextureYUYV : public SDLTexture > > +{ > > +public: > > + SDLTextureYUYV(const SDL_Rect &sdlRect); > > + void update(const libcamera::Span<uint8_t> &data) override; > > +};
Quoting Laurent Pinchart (2022-05-18 21:06:10) > Hi Eric, > > Thank you for the patch. > > On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote: > > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50) > > > This adds more portability to existing cam sinks. You can pass a > > > YUYV camera buffer and SDL will handle the pixel buffer conversion > > > to the display. This allows cam reference implementation to display > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam > > > reference implementation, to run as a desktop application in wayland or > > s/wayland/Wayland/ > > > > x11. SDL also has support for Android and ChromeOS which has not been > > s/x11/X11/ > > > > tested. Also tested on simpledrm raspberry pi 4 framebuffer > > s/raspberry pi/Raspberry Pi/ > > > > successfully where existing kms sink did not work. Can also be used as > > > kmsdrm sink. Only supports one camera stream at present. > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > src/cam/camera_session.cpp | 8 ++ > > > src/cam/main.cpp | 4 + > > > src/cam/main.h | 1 + > > > src/cam/meson.build | 12 +++ > > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++ > > > src/cam/sdl_sink.h | 48 +++++++++ > > > src/cam/sdl_texture.cpp | 32 ++++++ > > > src/cam/sdl_texture.h | 23 +++++ > > > src/cam/sdl_texture_yuyv.cpp | 13 +++ > > > src/cam/sdl_texture_yuyv.h | 10 ++ > > > > Given the number of sdl_ files here I wonder if they should be under > > src/cam/sdl/.... > > > > But not crucial to this patch. > > > > I can't test this right now - but I'm looking forward to seeing a > > preview on cam ... so I'll hopefully test this tomorrow. > > > > > 10 files changed, 343 insertions(+) > > > create mode 100644 src/cam/sdl_sink.cpp > > > create mode 100644 src/cam/sdl_sink.h > > > create mode 100644 src/cam/sdl_texture.cpp > > > create mode 100644 src/cam/sdl_texture.h > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp > > > create mode 100644 src/cam/sdl_texture_yuyv.h > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > index efffafbf..71e6bd60 100644 > > > --- a/src/cam/camera_session.cpp > > > +++ b/src/cam/camera_session.cpp > > > @@ -19,6 +19,9 @@ > > > #ifdef HAVE_KMS > > > #include "kms_sink.h" > > > #endif > > > +#ifdef HAVE_SDL > > > +#include "sdl_sink.h" > > > +#endif > > Please move this below in alphabetical order. > > > > #include "main.h" > > > #include "stream_options.h" > > > > > > @@ -186,6 +189,11 @@ int CameraSession::start() > > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > > #endif > > > > > > +#ifdef HAVE_SDL > > > + if (options_.isSet(OptSDL)) > > > + sink_ = std::make_unique<SDLSink>(); > > > +#endif > > > + > > > > It looks like the precedence of which sink really gets used if multiples > > are set is simply the order in this file that they get set. But I think > > that's ok for the moment. Not sure we could do much else right now > > without extending to support multiple sinks which is out of scope here. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > if (options_.isSet(OptFile)) { > > > if (!options_[OptFile].toString().empty()) > > > sink_ = std::make_unique<FileSink>(streamNames_, > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > index c7f664b9..fd3108b0 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > "Display viewfinder through DRM/KMS on specified connector", > > > "display", ArgumentOptional, "connector", false, > > > OptCamera); > > > +#endif > > > +#ifdef HAVE_SDL > > > + parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL", > > > + "sdl", ArgumentNone, "", false, OptCamera); > > > #endif > > I wonder if we should merge all sink arguments into one, but that can be > done on top (the cam arguments syntax isn't stable yet). > > > > parser.addOption(OptFile, OptionString, > > > "Write captured frames to disk\n" > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > index 62f7bbc9..2b285808 100644 > > > --- a/src/cam/main.h > > > +++ b/src/cam/main.h > > > @@ -18,6 +18,7 @@ enum { > > > OptListProperties = 'p', > > > OptMonitor = 'm', > > > OptStream = 's', > > > + OptSDL = 'S', > > > OptListControls = 256, > > > OptStrictFormats = 257, > > > OptMetadata = 258, > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > index 5bab8c9e..3032730b 100644 > > > --- a/src/cam/meson.build > > > +++ b/src/cam/meson.build > > > @@ -32,11 +32,23 @@ if libdrm.found() > > > ]) > > > endif > > > > > > +libsdl2 = dependency('SDL2', required : false) I installed libsdl2-dev but got failures around SDL2/SDL_image.h On ubuntu I installed libsdl2-image-dev, and then rebuilt. That found the header, but then caused a link error... Does this also need to have some 'SDL2-Image' dependency check? > > > + > > > +if libsdl2.found() > > > + cam_cpp_args += ['-DHAVE_SDL'] > > > + cam_sources += files([ > > > + 'sdl_sink.cpp', > > > + 'sdl_texture.cpp', > > > + 'sdl_texture_yuyv.cpp' > > > + ]) > > > +endif > > > + > > > cam = executable('cam', cam_sources, > > > dependencies : [ > > > libatomic, > > > libcamera_public, > > > libdrm, > > > + libsdl2, > > Please keep the list alphabetically ordered. > > > > libevent, > > > ], > > > cpp_args : cam_cpp_args, > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > new file mode 100644 > > > index 00000000..6fbeaf56 > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.cpp > > > @@ -0,0 +1,192 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.cpp - SDL Sink > > > + */ > > > + > > > +#include "sdl_sink.h" > > > + > > > +#include <assert.h> > > > +#include <fcntl.h> > > > +#include <iomanip> > > > +#include <iostream> > > > +#include <signal.h> > > > +#include <sstream> > > > +#include <string.h> > > > +#include <unistd.h> > > > + > > > +#include <libcamera/camera.h> > > > +#include <libcamera/formats.h> > > > + > > > +#include "sdl_texture_yuyv.h" > > > + > > > +#include "event_loop.h" > > > +#include "image.h" > > I don't think there's a need to split sdl_texture_yuyv.h and the next > two headers in two groups. > > > > + > > > +using namespace libcamera; > > > + > > > +SDLSink::SDLSink() > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}), > > > + sdlInit_(false) > > > +{ > > > +} > > > + > > > +SDLSink::~SDLSink() > > > +{ > > > + stop(); > > > +} > > > + > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > +{ > > > + int ret = FrameSink::configure(config); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (config.size() > 1) { > > > + std::cerr > > > + << "SDL sink only supports one camera stream at present, streaming first camera stream" > > > + << std::endl; > > > + } else if (config.empty()) { > > > + std::cerr << "Require at least one camera stream to process" > > > + << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + const libcamera::StreamConfiguration &cfg = config.at(0); > > > + sdlRect_.w = cfg.size.width; > > > + sdlRect_.h = cfg.size.height; > > > + > > > + switch (cfg.pixelFormat) { > > > + case libcamera::formats::YUYV: > > > + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_); > > > + break; > > > + default: > > > + std::cerr << "Unsupported pixel format " > > > + << cfg.pixelFormat.toString() << std::endl; > > > + return -EINVAL; > > > + }; > > > + > > > + return 0; > > > +} > > > + > > > +int SDLSink::start() > > > +{ > > > + int ret = SDL_Init(SDL_INIT_VIDEO); > > > + if (ret) { > > > + std::cerr << "Failed to initialize SDL: " << SDL_GetError() > > > + << std::endl; > > > + return ret; > > > + } > > > + > > > + sdlInit_ = true; > > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, > > > + sdlRect_.h, > > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > > + if (!sdlWindow_) { > > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() > > > + << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > > + if (!sdlRenderer_) { > > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() > > > + << std::endl; > > > + return -EINVAL; > > > + } > > > + > > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > > + std::cerr << "Failed to set SDL render logical size: " > > > + << SDL_GetError() << std::endl; > > > + } > > /* > * Not critical to set, don't return in this case, set for scaling > * purposes. > */ > if (ret) > std::cerr << "Failed to set SDL render logical size: " > << SDL_GetError() << std::endl; > > > > + > > > + ret = sdlTexture_->create(sdlRenderer_); > > > + if (ret) { > > > + return ret; > > > + } > > > + > > > + EventLoop::instance()->addTimerEvent( > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this)); > > I think I mentioned before that this won't work nicely if we stop and > restart. cam doesn't do so at the moment, so it doesn't have to be fixed > yet, but a todo comment would be good: > > /* \todo Make the event cancellable to support stop/start cycles. */ > EventLoop::instance()->addTimerEvent( > 10ms, std::bind(&SDLSink::processSDLEvents, this)); > > > > + > > > + return 0; > > > +} > > > + > > > +int SDLSink::stop() > > > +{ > > > + if (sdlTexture_) { > > > + sdlTexture_->destroy(); > > Can you turn SDLTexture::destroy() into a destructor ? Then you could > simply write > > sdlTexture_ = nullptr; > > or > > sdlTexture_.reset(); > > > > + sdlTexture_ = nullptr; > > > + } > > > + > > > + if (sdlRenderer_) { > > > + SDL_DestroyRenderer(sdlRenderer_); > > > + sdlRenderer_ = nullptr; > > > + } > > > + > > > + if (sdlWindow_) { > > > + SDL_DestroyWindow(sdlWindow_); > > > + sdlWindow_ = nullptr; > > > + } > > > + > > > + if (sdlInit_) { > > > + SDL_Quit(); > > > + sdlInit_ = false; > > > + } > > > + > > > + return FrameSink::stop(); > > > +} > > > + > > > +void SDLSink::mapBuffer(FrameBuffer *buffer) > > > +{ > > > + std::unique_ptr<Image> image = > > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly); > > > + assert(image != nullptr); > > > + > > > + mappedBuffers_[buffer] = std::move(image); > > > +} > > > + > > > +bool SDLSink::processRequest(Request *request) > > > +{ > > > + for (auto [stream, buffer] : request->buffers()) { > > > + renderBuffer(buffer); > > > + break; /* to be expanded to launch SDL window per buffer */ > > > + } > > > + > > > + return true; > > > +} > > > + > > > +/* > > > + * Process SDL events, required for things like window resize and quit button > > > + */ > > > +void SDLSink::processSDLEvents() > > > +{ > > > + for (SDL_Event e; SDL_PollEvent(&e);) { > > > + if (e.type == SDL_QUIT) { > > > + /* Click close icon then quit */ > > > + EventLoop::instance()->exit(0); > > > + } > > > + } > > > +} > > > + > > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > > +{ > > > + Image *image = mappedBuffers_[buffer].get(); > > > + > > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > > + const FrameMetadata::Plane &meta = > > > + buffer->metadata().planes()[i]; > > > + > > > + Span<uint8_t> data = image->data(i); > > > + if (meta.bytesused > data.size()) > > > + std::cerr << "payload size " << meta.bytesused > > > + << " larger than plane size " << data.size() > > > + << std::endl; > > > + > > > + sdlTexture_->update(data); > > I really don't think this will do what you expect for multi-planar > formats. As multi-planar formats are not supported yet, I'd replace the > loop by a single pass over plane 0, and add a > > /* \todo Implement support for multi-planar formats. */ > > > > + } > > > + > > > + SDL_RenderClear(sdlRenderer_); > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr); > > > + SDL_RenderPresent(sdlRenderer_); > > > +} > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > new file mode 100644 > > > index 00000000..37dd9f7e > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.h > > > @@ -0,0 +1,48 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * sdl_sink.h - SDL Sink > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <map> > > > +#include <memory> > > > +#include <string> > > I think you can drop the string header. > > > > + > > > +#include <libcamera/stream.h> > > > + > > > +#include <SDL2/SDL.h> > > > + > > > +#include "frame_sink.h" > > > +#include "sdl_texture.h" > > You could also forward-declare the SDLTexture class and drop this > header. > > > > + > > > +class Image; > > > + > > > +class SDLSink : public FrameSink > > > +{ > > > +public: > > > + SDLSink(); > > > + ~SDLSink(); > > > + > > > + int configure(const libcamera::CameraConfiguration &config) override; > > > + int start() override; > > > + int stop() override; > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > > + > > > + bool processRequest(libcamera::Request *request) override; > > > + > > > +private: > > > + void renderBuffer(libcamera::FrameBuffer *buffer); > > > + void processSDLEvents(); > > > + > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> > > > + mappedBuffers_; > > > + > > > + std::unique_ptr<SDLTexture> sdlTexture_; > > > + > > > + SDL_Window *sdlWindow_; > > > + SDL_Renderer *sdlRenderer_; > > > + SDL_Rect sdlRect_; > > I'm tempted to drop the sdl prefix from variable names, up to you. > > > > + SDL_PixelFormatEnum pixelFormat_; > > > + bool sdlInit_; > > > +}; > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp > > > new file mode 100644 > > > index 00000000..b39080c9 > > > --- /dev/null > > > +++ b/src/cam/sdl_texture.cpp > > > @@ -0,0 +1,32 @@ > > Missing SPDX tag and copyright header. Same below. > > > > +#include "sdl_texture.h" > > > + > > > +#include <iostream> > > > + > > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > > + const int pitch) > > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch) > > > +{ > > > +} > > > + > > > +SDLTexture::~SDLTexture() > > > +{ > > > +} > > > + > > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_) > > > +{ > > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, > > > + sdlRect_.h); > > > + if (!ptr_) { > > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() > > > + << std::endl; > > > + return -ENOMEM; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +void SDLTexture::destroy() > > > +{ > > > + SDL_DestroyTexture(ptr_); > > > +} > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > new file mode 100644 > > > index 00000000..f9525f7f > > > --- /dev/null > > > +++ b/src/cam/sdl_texture.h > > > @@ -0,0 +1,23 @@ > > > +#pragma once > > > + > > > +#include <SDL2/SDL.h> > > > + > > > +#include "image.h" > > > + > > > +class SDLTexture > > > +{ > > > +public: > > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > > + const int pitch); > > > + virtual ~SDLTexture(); > > > + int create(SDL_Renderer *sdlRenderer_); > > s/sdlRenderer_/sdlRenderer/ > > And same here (and with sdlRect in the previous function) about the sdl > prefix. > > > > + void destroy(); > > > + virtual void update(const libcamera::Span<uint8_t> &data) = 0; > > > + > > > + SDL_Texture *ptr_; > > Is it worth making this protected and adding an accessor function ? > > > > + > > > +protected: > > > + const SDL_Rect &sdlRect_; > > Same here, rect_. > > > > + SDL_PixelFormatEnum pixelFormat_; > > > + const int pitch_; > > > +}; > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp > > > new file mode 100644 > > > index 00000000..cf519a5d > > > --- /dev/null > > > +++ b/src/cam/sdl_texture_yuyv.cpp > > > @@ -0,0 +1,13 @@ > > > +#include "sdl_texture_yuyv.h" > > > + > > > +using namespace libcamera; > > > + > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect) > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2)) > > > +{ > > > +} > > > + > > > +void SDLTextureYUYV::update(const Span<uint8_t> &data) > > > +{ > > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_); > > > +} > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h > > > new file mode 100644 > > > index 00000000..ad70f14f > > > --- /dev/null > > > +++ b/src/cam/sdl_texture_yuyv.h > > > @@ -0,0 +1,10 @@ > > > +#pragma once > > > + > > > +#include "sdl_texture.h" > > > + > > > +class SDLTextureYUYV : public SDLTexture > > > +{ > > > +public: > > > + SDLTextureYUYV(const SDL_Rect &sdlRect); > > > + void update(const libcamera::Span<uint8_t> &data) override; > > > +}; > > -- > Regards, > > Laurent Pinchart
On Thu, 19 May 2022 at 17:22, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Laurent Pinchart (2022-05-18 21:06:10) > > Hi Eric, > > > > Thank you for the patch. > > > > On Wed, May 18, 2022 at 12:38:20AM +0100, Kieran Bingham via libcamera-devel wrote: > > > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:50) > > > > This adds more portability to existing cam sinks. You can pass a > > > > YUYV camera buffer and SDL will handle the pixel buffer conversion > > > > to the display. This allows cam reference implementation to display > > > > images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam > > > > reference implementation, to run as a desktop application in wayland or > > > > s/wayland/Wayland/ > > > > > > x11. SDL also has support for Android and ChromeOS which has not been > > > > s/x11/X11/ > > > > > > tested. Also tested on simpledrm raspberry pi 4 framebuffer > > > > s/raspberry pi/Raspberry Pi/ > > > > > > successfully where existing kms sink did not work. Can also be used as > > > > kmsdrm sink. Only supports one camera stream at present. > > > > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > src/cam/camera_session.cpp | 8 ++ > > > > src/cam/main.cpp | 4 + > > > > src/cam/main.h | 1 + > > > > src/cam/meson.build | 12 +++ > > > > src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++ > > > > src/cam/sdl_sink.h | 48 +++++++++ > > > > src/cam/sdl_texture.cpp | 32 ++++++ > > > > src/cam/sdl_texture.h | 23 +++++ > > > > src/cam/sdl_texture_yuyv.cpp | 13 +++ > > > > src/cam/sdl_texture_yuyv.h | 10 ++ > > > > > > Given the number of sdl_ files here I wonder if they should be under > > > src/cam/sdl/.... > > > > > > But not crucial to this patch. > > > > > > I can't test this right now - but I'm looking forward to seeing a > > > preview on cam ... so I'll hopefully test this tomorrow. > > > > > > > 10 files changed, 343 insertions(+) > > > > create mode 100644 src/cam/sdl_sink.cpp > > > > create mode 100644 src/cam/sdl_sink.h > > > > create mode 100644 src/cam/sdl_texture.cpp > > > > create mode 100644 src/cam/sdl_texture.h > > > > create mode 100644 src/cam/sdl_texture_yuyv.cpp > > > > create mode 100644 src/cam/sdl_texture_yuyv.h > > > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > > index efffafbf..71e6bd60 100644 > > > > --- a/src/cam/camera_session.cpp > > > > +++ b/src/cam/camera_session.cpp > > > > @@ -19,6 +19,9 @@ > > > > #ifdef HAVE_KMS > > > > #include "kms_sink.h" > > > > #endif > > > > +#ifdef HAVE_SDL > > > > +#include "sdl_sink.h" > > > > +#endif > > > > Please move this below in alphabetical order. > > > > > > #include "main.h" > > > > #include "stream_options.h" > > > > > > > > @@ -186,6 +189,11 @@ int CameraSession::start() > > > > sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); > > > > #endif > > > > > > > > +#ifdef HAVE_SDL > > > > + if (options_.isSet(OptSDL)) > > > > + sink_ = std::make_unique<SDLSink>(); > > > > +#endif > > > > + > > > > > > It looks like the precedence of which sink really gets used if multiples > > > are set is simply the order in this file that they get set. But I think > > > that's ok for the moment. Not sure we could do much else right now > > > without extending to support multiple sinks which is out of scope here. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > if (options_.isSet(OptFile)) { > > > > if (!options_[OptFile].toString().empty()) > > > > sink_ = std::make_unique<FileSink>(streamNames_, > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > > index c7f664b9..fd3108b0 100644 > > > > --- a/src/cam/main.cpp > > > > +++ b/src/cam/main.cpp > > > > @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > > "Display viewfinder through DRM/KMS on specified connector", > > > > "display", ArgumentOptional, "connector", false, > > > > OptCamera); > > > > +#endif > > > > +#ifdef HAVE_SDL > > > > + parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL", > > > > + "sdl", ArgumentNone, "", false, OptCamera); > > > > #endif > > > > I wonder if we should merge all sink arguments into one, but that can be > > done on top (the cam arguments syntax isn't stable yet). > > > > > > parser.addOption(OptFile, OptionString, > > > > "Write captured frames to disk\n" > > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > > index 62f7bbc9..2b285808 100644 > > > > --- a/src/cam/main.h > > > > +++ b/src/cam/main.h > > > > @@ -18,6 +18,7 @@ enum { > > > > OptListProperties = 'p', > > > > OptMonitor = 'm', > > > > OptStream = 's', > > > > + OptSDL = 'S', > > > > OptListControls = 256, > > > > OptStrictFormats = 257, > > > > OptMetadata = 258, > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > index 5bab8c9e..3032730b 100644 > > > > --- a/src/cam/meson.build > > > > +++ b/src/cam/meson.build > > > > @@ -32,11 +32,23 @@ if libdrm.found() > > > > ]) > > > > endif > > > > > > > > +libsdl2 = dependency('SDL2', required : false) > > I installed libsdl2-dev but got failures around SDL2/SDL_image.h > > On ubuntu I installed libsdl2-image-dev, and then rebuilt. That found > the header, but then caused a link error... What version of Ubuntu? Fedora tends to be my primary build machine. It was intended that patch 3 was only the SDL core library dependant code and patch 4 was the SDL image dependant code for MJPG. > > Does this also need to have some 'SDL2-Image' dependency check? > > > > > > + > > > > +if libsdl2.found() > > > > + cam_cpp_args += ['-DHAVE_SDL'] > > > > + cam_sources += files([ > > > > + 'sdl_sink.cpp', > > > > + 'sdl_texture.cpp', > > > > + 'sdl_texture_yuyv.cpp' > > > > + ]) > > > > +endif > > > > + > > > > cam = executable('cam', cam_sources, > > > > dependencies : [ > > > > libatomic, > > > > libcamera_public, > > > > libdrm, > > > > + libsdl2, > > > > Please keep the list alphabetically ordered. > > > > > > libevent, > > > > ], > > > > cpp_args : cam_cpp_args, > > > > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp > > > > new file mode 100644 > > > > index 00000000..6fbeaf56 > > > > --- /dev/null > > > > +++ b/src/cam/sdl_sink.cpp > > > > @@ -0,0 +1,192 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > +/* > > > > + * sdl_sink.cpp - SDL Sink > > > > + */ > > > > + > > > > +#include "sdl_sink.h" > > > > + > > > > +#include <assert.h> > > > > +#include <fcntl.h> > > > > +#include <iomanip> > > > > +#include <iostream> > > > > +#include <signal.h> > > > > +#include <sstream> > > > > +#include <string.h> > > > > +#include <unistd.h> > > > > + > > > > +#include <libcamera/camera.h> > > > > +#include <libcamera/formats.h> > > > > + > > > > +#include "sdl_texture_yuyv.h" > > > > + > > > > +#include "event_loop.h" > > > > +#include "image.h" > > > > I don't think there's a need to split sdl_texture_yuyv.h and the next > > two headers in two groups. > > > > > > + > > > > +using namespace libcamera; > > > > + > > > > +SDLSink::SDLSink() > > > > + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}), > > > > + sdlInit_(false) > > > > +{ > > > > +} > > > > + > > > > +SDLSink::~SDLSink() > > > > +{ > > > > + stop(); > > > > +} > > > > + > > > > +int SDLSink::configure(const libcamera::CameraConfiguration &config) > > > > +{ > > > > + int ret = FrameSink::configure(config); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if (config.size() > 1) { > > > > + std::cerr > > > > + << "SDL sink only supports one camera stream at present, streaming first camera stream" > > > > + << std::endl; > > > > + } else if (config.empty()) { > > > > + std::cerr << "Require at least one camera stream to process" > > > > + << std::endl; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + const libcamera::StreamConfiguration &cfg = config.at(0); > > > > + sdlRect_.w = cfg.size.width; > > > > + sdlRect_.h = cfg.size.height; > > > > + > > > > + switch (cfg.pixelFormat) { > > > > + case libcamera::formats::YUYV: > > > > + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_); > > > > + break; > > > > + default: > > > > + std::cerr << "Unsupported pixel format " > > > > + << cfg.pixelFormat.toString() << std::endl; > > > > + return -EINVAL; > > > > + }; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int SDLSink::start() > > > > +{ > > > > + int ret = SDL_Init(SDL_INIT_VIDEO); > > > > + if (ret) { > > > > + std::cerr << "Failed to initialize SDL: " << SDL_GetError() > > > > + << std::endl; > > > > + return ret; > > > > + } > > > > + > > > > + sdlInit_ = true; > > > > + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, > > > > + sdlRect_.h, > > > > + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > > > + if (!sdlWindow_) { > > > > + std::cerr << "Failed to create SDL window: " << SDL_GetError() > > > > + << std::endl; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); > > > > + if (!sdlRenderer_) { > > > > + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() > > > > + << std::endl; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); > > > > + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ > > > > + std::cerr << "Failed to set SDL render logical size: " > > > > + << SDL_GetError() << std::endl; > > > > + } > > > > /* > > * Not critical to set, don't return in this case, set for scaling > > * purposes. > > */ > > if (ret) > > std::cerr << "Failed to set SDL render logical size: " > > << SDL_GetError() << std::endl; > > > > > > + > > > > + ret = sdlTexture_->create(sdlRenderer_); > > > > + if (ret) { > > > > + return ret; > > > > + } > > > > + > > > > + EventLoop::instance()->addTimerEvent( > > > > + 10ms, std::bind(&SDLSink::processSDLEvents, this)); > > > > I think I mentioned before that this won't work nicely if we stop and > > restart. cam doesn't do so at the moment, so it doesn't have to be fixed > > yet, but a todo comment would be good: > > > > /* \todo Make the event cancellable to support stop/start cycles. */ > > EventLoop::instance()->addTimerEvent( > > 10ms, std::bind(&SDLSink::processSDLEvents, this)); > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int SDLSink::stop() > > > > +{ > > > > + if (sdlTexture_) { > > > > + sdlTexture_->destroy(); > > > > Can you turn SDLTexture::destroy() into a destructor ? Then you could > > simply write > > > > sdlTexture_ = nullptr; > > > > or > > > > sdlTexture_.reset(); > > > > > > + sdlTexture_ = nullptr; > > > > + } > > > > + > > > > + if (sdlRenderer_) { > > > > + SDL_DestroyRenderer(sdlRenderer_); > > > > + sdlRenderer_ = nullptr; > > > > + } > > > > + > > > > + if (sdlWindow_) { > > > > + SDL_DestroyWindow(sdlWindow_); > > > > + sdlWindow_ = nullptr; > > > > + } > > > > + > > > > + if (sdlInit_) { > > > > + SDL_Quit(); > > > > + sdlInit_ = false; > > > > + } > > > > + > > > > + return FrameSink::stop(); > > > > +} > > > > + > > > > +void SDLSink::mapBuffer(FrameBuffer *buffer) > > > > +{ > > > > + std::unique_ptr<Image> image = > > > > + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly); > > > > + assert(image != nullptr); > > > > + > > > > + mappedBuffers_[buffer] = std::move(image); > > > > +} > > > > + > > > > +bool SDLSink::processRequest(Request *request) > > > > +{ > > > > + for (auto [stream, buffer] : request->buffers()) { > > > > + renderBuffer(buffer); > > > > + break; /* to be expanded to launch SDL window per buffer */ > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +/* > > > > + * Process SDL events, required for things like window resize and quit button > > > > + */ > > > > +void SDLSink::processSDLEvents() > > > > +{ > > > > + for (SDL_Event e; SDL_PollEvent(&e);) { > > > > + if (e.type == SDL_QUIT) { > > > > + /* Click close icon then quit */ > > > > + EventLoop::instance()->exit(0); > > > > + } > > > > + } > > > > +} > > > > + > > > > +void SDLSink::renderBuffer(FrameBuffer *buffer) > > > > +{ > > > > + Image *image = mappedBuffers_[buffer].get(); > > > > + > > > > + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > > > + const FrameMetadata::Plane &meta = > > > > + buffer->metadata().planes()[i]; > > > > + > > > > + Span<uint8_t> data = image->data(i); > > > > + if (meta.bytesused > data.size()) > > > > + std::cerr << "payload size " << meta.bytesused > > > > + << " larger than plane size " << data.size() > > > > + << std::endl; > > > > + > > > > + sdlTexture_->update(data); > > > > I really don't think this will do what you expect for multi-planar > > formats. As multi-planar formats are not supported yet, I'd replace the > > loop by a single pass over plane 0, and add a > > > > /* \todo Implement support for multi-planar formats. */ > > > > > > + } > > > > + > > > > + SDL_RenderClear(sdlRenderer_); > > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr); > > > > + SDL_RenderPresent(sdlRenderer_); > > > > +} > > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > > new file mode 100644 > > > > index 00000000..37dd9f7e > > > > --- /dev/null > > > > +++ b/src/cam/sdl_sink.h > > > > @@ -0,0 +1,48 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > +/* > > > > + * sdl_sink.h - SDL Sink > > > > + */ > > > > + > > > > +#pragma once > > > > + > > > > +#include <map> > > > > +#include <memory> > > > > +#include <string> > > > > I think you can drop the string header. > > > > > > + > > > > +#include <libcamera/stream.h> > > > > + > > > > +#include <SDL2/SDL.h> > > > > + > > > > +#include "frame_sink.h" > > > > +#include "sdl_texture.h" > > > > You could also forward-declare the SDLTexture class and drop this > > header. > > > > > > + > > > > +class Image; > > > > + > > > > +class SDLSink : public FrameSink > > > > +{ > > > > +public: > > > > + SDLSink(); > > > > + ~SDLSink(); > > > > + > > > > + int configure(const libcamera::CameraConfiguration &config) override; > > > > + int start() override; > > > > + int stop() override; > > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > > > + > > > > + bool processRequest(libcamera::Request *request) override; > > > > + > > > > +private: > > > > + void renderBuffer(libcamera::FrameBuffer *buffer); > > > > + void processSDLEvents(); > > > > + > > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> > > > > + mappedBuffers_; > > > > + > > > > + std::unique_ptr<SDLTexture> sdlTexture_; > > > > + > > > > + SDL_Window *sdlWindow_; > > > > + SDL_Renderer *sdlRenderer_; > > > > + SDL_Rect sdlRect_; > > > > I'm tempted to drop the sdl prefix from variable names, up to you. > > > > > > + SDL_PixelFormatEnum pixelFormat_; > > > > + bool sdlInit_; > > > > +}; > > > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp > > > > new file mode 100644 > > > > index 00000000..b39080c9 > > > > --- /dev/null > > > > +++ b/src/cam/sdl_texture.cpp > > > > @@ -0,0 +1,32 @@ > > > > Missing SPDX tag and copyright header. Same below. > > > > > > +#include "sdl_texture.h" > > > > + > > > > +#include <iostream> > > > > + > > > > +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > > > + const int pitch) > > > > + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch) > > > > +{ > > > > +} > > > > + > > > > +SDLTexture::~SDLTexture() > > > > +{ > > > > +} > > > > + > > > > +int SDLTexture::create(SDL_Renderer *sdlRenderer_) > > > > +{ > > > > + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, > > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, > > > > + sdlRect_.h); > > > > + if (!ptr_) { > > > > + std::cerr << "Failed to create SDL texture: " << SDL_GetError() > > > > + << std::endl; > > > > + return -ENOMEM; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +void SDLTexture::destroy() > > > > +{ > > > > + SDL_DestroyTexture(ptr_); > > > > +} > > > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h > > > > new file mode 100644 > > > > index 00000000..f9525f7f > > > > --- /dev/null > > > > +++ b/src/cam/sdl_texture.h > > > > @@ -0,0 +1,23 @@ > > > > +#pragma once > > > > + > > > > +#include <SDL2/SDL.h> > > > > + > > > > +#include "image.h" > > > > + > > > > +class SDLTexture > > > > +{ > > > > +public: > > > > + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, > > > > + const int pitch); > > > > + virtual ~SDLTexture(); > > > > + int create(SDL_Renderer *sdlRenderer_); > > > > s/sdlRenderer_/sdlRenderer/ > > > > And same here (and with sdlRect in the previous function) about the sdl > > prefix. > > > > > > + void destroy(); > > > > + virtual void update(const libcamera::Span<uint8_t> &data) = 0; > > > > + > > > > + SDL_Texture *ptr_; > > > > Is it worth making this protected and adding an accessor function ? > > > > > > + > > > > +protected: > > > > + const SDL_Rect &sdlRect_; > > > > Same here, rect_. > > > > > > + SDL_PixelFormatEnum pixelFormat_; > > > > + const int pitch_; > > > > +}; > > > > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp > > > > new file mode 100644 > > > > index 00000000..cf519a5d > > > > --- /dev/null > > > > +++ b/src/cam/sdl_texture_yuyv.cpp > > > > @@ -0,0 +1,13 @@ > > > > +#include "sdl_texture_yuyv.h" > > > > + > > > > +using namespace libcamera; > > > > + > > > > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect) > > > > + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2)) > > > > +{ > > > > +} > > > > + > > > > +void SDLTextureYUYV::update(const Span<uint8_t> &data) > > > > +{ > > > > + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_); > > > > +} > > > > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h > > > > new file mode 100644 > > > > index 00000000..ad70f14f > > > > --- /dev/null > > > > +++ b/src/cam/sdl_texture_yuyv.h > > > > @@ -0,0 +1,10 @@ > > > > +#pragma once > > > > + > > > > +#include "sdl_texture.h" > > > > + > > > > +class SDLTextureYUYV : public SDLTexture > > > > +{ > > > > +public: > > > > + SDLTextureYUYV(const SDL_Rect &sdlRect); > > > > + void update(const libcamera::Span<uint8_t> &data) override; > > > > +}; > > > > -- > > Regards, > > > > Laurent Pinchart >
diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp index efffafbf..71e6bd60 100644 --- a/src/cam/camera_session.cpp +++ b/src/cam/camera_session.cpp @@ -19,6 +19,9 @@ #ifdef HAVE_KMS #include "kms_sink.h" #endif +#ifdef HAVE_SDL +#include "sdl_sink.h" +#endif #include "main.h" #include "stream_options.h" @@ -186,6 +189,11 @@ int CameraSession::start() sink_ = std::make_unique<KMSSink>(options_[OptDisplay].toString()); #endif +#ifdef HAVE_SDL + if (options_.isSet(OptSDL)) + sink_ = std::make_unique<SDLSink>(); +#endif + if (options_.isSet(OptFile)) { if (!options_[OptFile].toString().empty()) sink_ = std::make_unique<FileSink>(streamNames_, diff --git a/src/cam/main.cpp b/src/cam/main.cpp index c7f664b9..fd3108b0 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -137,6 +137,10 @@ int CamApp::parseOptions(int argc, char *argv[]) "Display viewfinder through DRM/KMS on specified connector", "display", ArgumentOptional, "connector", false, OptCamera); +#endif +#ifdef HAVE_SDL + parser.addOption(OptSDL, OptionNone, "Display viewfinder through SDL", + "sdl", ArgumentNone, "", false, OptCamera); #endif parser.addOption(OptFile, OptionString, "Write captured frames to disk\n" diff --git a/src/cam/main.h b/src/cam/main.h index 62f7bbc9..2b285808 100644 --- a/src/cam/main.h +++ b/src/cam/main.h @@ -18,6 +18,7 @@ enum { OptListProperties = 'p', OptMonitor = 'm', OptStream = 's', + OptSDL = 'S', OptListControls = 256, OptStrictFormats = 257, OptMetadata = 258, diff --git a/src/cam/meson.build b/src/cam/meson.build index 5bab8c9e..3032730b 100644 --- a/src/cam/meson.build +++ b/src/cam/meson.build @@ -32,11 +32,23 @@ if libdrm.found() ]) endif +libsdl2 = dependency('SDL2', required : false) + +if libsdl2.found() + cam_cpp_args += ['-DHAVE_SDL'] + cam_sources += files([ + 'sdl_sink.cpp', + 'sdl_texture.cpp', + 'sdl_texture_yuyv.cpp' + ]) +endif + cam = executable('cam', cam_sources, dependencies : [ libatomic, libcamera_public, libdrm, + libsdl2, libevent, ], cpp_args : cam_cpp_args, diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp new file mode 100644 index 00000000..6fbeaf56 --- /dev/null +++ b/src/cam/sdl_sink.cpp @@ -0,0 +1,192 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * sdl_sink.cpp - SDL Sink + */ + +#include "sdl_sink.h" + +#include <assert.h> +#include <fcntl.h> +#include <iomanip> +#include <iostream> +#include <signal.h> +#include <sstream> +#include <string.h> +#include <unistd.h> + +#include <libcamera/camera.h> +#include <libcamera/formats.h> + +#include "sdl_texture_yuyv.h" + +#include "event_loop.h" +#include "image.h" + +using namespace libcamera; + +SDLSink::SDLSink() + : sdlWindow_(nullptr), sdlRenderer_(nullptr), sdlRect_({}), + sdlInit_(false) +{ +} + +SDLSink::~SDLSink() +{ + stop(); +} + +int SDLSink::configure(const libcamera::CameraConfiguration &config) +{ + int ret = FrameSink::configure(config); + if (ret < 0) + return ret; + + if (config.size() > 1) { + std::cerr + << "SDL sink only supports one camera stream at present, streaming first camera stream" + << std::endl; + } else if (config.empty()) { + std::cerr << "Require at least one camera stream to process" + << std::endl; + return -EINVAL; + } + + const libcamera::StreamConfiguration &cfg = config.at(0); + sdlRect_.w = cfg.size.width; + sdlRect_.h = cfg.size.height; + + switch (cfg.pixelFormat) { + case libcamera::formats::YUYV: + sdlTexture_ = std::make_unique<SDLTextureYUYV>(sdlRect_); + break; + default: + std::cerr << "Unsupported pixel format " + << cfg.pixelFormat.toString() << std::endl; + return -EINVAL; + }; + + return 0; +} + +int SDLSink::start() +{ + int ret = SDL_Init(SDL_INIT_VIDEO); + if (ret) { + std::cerr << "Failed to initialize SDL: " << SDL_GetError() + << std::endl; + return ret; + } + + sdlInit_ = true; + sdlWindow_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w, + sdlRect_.h, + SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); + if (!sdlWindow_) { + std::cerr << "Failed to create SDL window: " << SDL_GetError() + << std::endl; + return -EINVAL; + } + + sdlRenderer_ = SDL_CreateRenderer(sdlWindow_, -1, 0); + if (!sdlRenderer_) { + std::cerr << "Failed to create SDL renderer: " << SDL_GetError() + << std::endl; + return -EINVAL; + } + + ret = SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w, sdlRect_.h); + if (ret) { /* Not critical to set, don't return in this case, set for scaling purposes */ + std::cerr << "Failed to set SDL render logical size: " + << SDL_GetError() << std::endl; + } + + ret = sdlTexture_->create(sdlRenderer_); + if (ret) { + return ret; + } + + EventLoop::instance()->addTimerEvent( + 10ms, std::bind(&SDLSink::processSDLEvents, this)); + + return 0; +} + +int SDLSink::stop() +{ + if (sdlTexture_) { + sdlTexture_->destroy(); + sdlTexture_ = nullptr; + } + + if (sdlRenderer_) { + SDL_DestroyRenderer(sdlRenderer_); + sdlRenderer_ = nullptr; + } + + if (sdlWindow_) { + SDL_DestroyWindow(sdlWindow_); + sdlWindow_ = nullptr; + } + + if (sdlInit_) { + SDL_Quit(); + sdlInit_ = false; + } + + return FrameSink::stop(); +} + +void SDLSink::mapBuffer(FrameBuffer *buffer) +{ + std::unique_ptr<Image> image = + Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly); + assert(image != nullptr); + + mappedBuffers_[buffer] = std::move(image); +} + +bool SDLSink::processRequest(Request *request) +{ + for (auto [stream, buffer] : request->buffers()) { + renderBuffer(buffer); + break; /* to be expanded to launch SDL window per buffer */ + } + + return true; +} + +/* + * Process SDL events, required for things like window resize and quit button + */ +void SDLSink::processSDLEvents() +{ + for (SDL_Event e; SDL_PollEvent(&e);) { + if (e.type == SDL_QUIT) { + /* Click close icon then quit */ + EventLoop::instance()->exit(0); + } + } +} + +void SDLSink::renderBuffer(FrameBuffer *buffer) +{ + Image *image = mappedBuffers_[buffer].get(); + + for (unsigned int i = 0; i < buffer->planes().size(); ++i) { + const FrameMetadata::Plane &meta = + buffer->metadata().planes()[i]; + + Span<uint8_t> data = image->data(i); + if (meta.bytesused > data.size()) + std::cerr << "payload size " << meta.bytesused + << " larger than plane size " << data.size() + << std::endl; + + sdlTexture_->update(data); + } + + SDL_RenderClear(sdlRenderer_); + SDL_RenderCopy(sdlRenderer_, sdlTexture_->ptr_, nullptr, nullptr); + SDL_RenderPresent(sdlRenderer_); +} diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h new file mode 100644 index 00000000..37dd9f7e --- /dev/null +++ b/src/cam/sdl_sink.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * sdl_sink.h - SDL Sink + */ + +#pragma once + +#include <map> +#include <memory> +#include <string> + +#include <libcamera/stream.h> + +#include <SDL2/SDL.h> + +#include "frame_sink.h" +#include "sdl_texture.h" + +class Image; + +class SDLSink : public FrameSink +{ +public: + SDLSink(); + ~SDLSink(); + + int configure(const libcamera::CameraConfiguration &config) override; + int start() override; + int stop() override; + void mapBuffer(libcamera::FrameBuffer *buffer) override; + + bool processRequest(libcamera::Request *request) override; + +private: + void renderBuffer(libcamera::FrameBuffer *buffer); + void processSDLEvents(); + + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> + mappedBuffers_; + + std::unique_ptr<SDLTexture> sdlTexture_; + + SDL_Window *sdlWindow_; + SDL_Renderer *sdlRenderer_; + SDL_Rect sdlRect_; + SDL_PixelFormatEnum pixelFormat_; + bool sdlInit_; +}; diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp new file mode 100644 index 00000000..b39080c9 --- /dev/null +++ b/src/cam/sdl_texture.cpp @@ -0,0 +1,32 @@ +#include "sdl_texture.h" + +#include <iostream> + +SDLTexture::SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, + const int pitch) + : sdlRect_(sdlRect), pixelFormat_(pixelFormat), pitch_(pitch) +{ +} + +SDLTexture::~SDLTexture() +{ +} + +int SDLTexture::create(SDL_Renderer *sdlRenderer_) +{ + ptr_ = SDL_CreateTexture(sdlRenderer_, pixelFormat_, + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w, + sdlRect_.h); + if (!ptr_) { + std::cerr << "Failed to create SDL texture: " << SDL_GetError() + << std::endl; + return -ENOMEM; + } + + return 0; +} + +void SDLTexture::destroy() +{ + SDL_DestroyTexture(ptr_); +} diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h new file mode 100644 index 00000000..f9525f7f --- /dev/null +++ b/src/cam/sdl_texture.h @@ -0,0 +1,23 @@ +#pragma once + +#include <SDL2/SDL.h> + +#include "image.h" + +class SDLTexture +{ +public: + SDLTexture(const SDL_Rect &sdlRect, SDL_PixelFormatEnum pixelFormat, + const int pitch); + virtual ~SDLTexture(); + int create(SDL_Renderer *sdlRenderer_); + void destroy(); + virtual void update(const libcamera::Span<uint8_t> &data) = 0; + + SDL_Texture *ptr_; + +protected: + const SDL_Rect &sdlRect_; + SDL_PixelFormatEnum pixelFormat_; + const int pitch_; +}; diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp new file mode 100644 index 00000000..cf519a5d --- /dev/null +++ b/src/cam/sdl_texture_yuyv.cpp @@ -0,0 +1,13 @@ +#include "sdl_texture_yuyv.h" + +using namespace libcamera; + +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &sdlRect) + : SDLTexture(sdlRect, SDL_PIXELFORMAT_YUY2, 4 * ((sdlRect.w + 1) / 2)) +{ +} + +void SDLTextureYUYV::update(const Span<uint8_t> &data) +{ + SDL_UpdateTexture(ptr_, &sdlRect_, data.data(), pitch_); +} diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h new file mode 100644 index 00000000..ad70f14f --- /dev/null +++ b/src/cam/sdl_texture_yuyv.h @@ -0,0 +1,10 @@ +#pragma once + +#include "sdl_texture.h" + +class SDLTextureYUYV : public SDLTexture +{ +public: + SDLTextureYUYV(const SDL_Rect &sdlRect); + void update(const libcamera::Span<uint8_t> &data) override; +};
This adds more portability to existing cam sinks. You can pass a YUYV camera buffer and SDL will handle the pixel buffer conversion to the display. This allows cam reference implementation to display images on VMs, Mac M1, Raspberry Pi, etc. This also enables cam reference implementation, to run as a desktop application in wayland or x11. SDL also has support for Android and ChromeOS which has not been tested. Also tested on simpledrm raspberry pi 4 framebuffer successfully where existing kms sink did not work. Can also be used as kmsdrm sink. Only supports one camera stream at present. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- src/cam/camera_session.cpp | 8 ++ src/cam/main.cpp | 4 + src/cam/main.h | 1 + src/cam/meson.build | 12 +++ src/cam/sdl_sink.cpp | 192 +++++++++++++++++++++++++++++++++++ src/cam/sdl_sink.h | 48 +++++++++ src/cam/sdl_texture.cpp | 32 ++++++ src/cam/sdl_texture.h | 23 +++++ src/cam/sdl_texture_yuyv.cpp | 13 +++ src/cam/sdl_texture_yuyv.h | 10 ++ 10 files changed, 343 insertions(+) create mode 100644 src/cam/sdl_sink.cpp create mode 100644 src/cam/sdl_sink.h create mode 100644 src/cam/sdl_texture.cpp create mode 100644 src/cam/sdl_texture.h create mode 100644 src/cam/sdl_texture_yuyv.cpp create mode 100644 src/cam/sdl_texture_yuyv.h