| Message ID | 20220322190936.69281-1-ecurtin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36) > This adds more portability to existing cam sinks. You can pass a > YUYV camera buffer for example and SDL will handle the pixel buffer > conversion, although SDL does not support decompression for pixelformats > like MJPEG. 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 I have not tested. > Also tested on simpledrm raspberry pi 4 framebuffer successfully where > existing kms sink did not work. This sounds very helpful and useful to me... > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > Changes in v2: > - Remove hardcoded pixel format from SDL_CreateTexture call > --- > src/cam/camera_session.cpp | 8 +++ > src/cam/main.cpp | 6 ++ > src/cam/main.h | 1 + > src/cam/meson.build | 10 +++ > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++ > src/cam/sdl_sink.h | 40 ++++++++++++ > 6 files changed, 190 insertions(+) > create mode 100644 src/cam/sdl_sink.cpp > create mode 100644 src/cam/sdl_sink.h > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > index 0428b538..30162dbd 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" > > @@ -187,6 +190,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..406d61bc 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[]) > "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" > "If the file name ends with a '/', it sets the directory in which\n" > diff --git a/src/cam/main.h b/src/cam/main.h > index 62f7bbc9..a64f95a0 100644 > --- a/src/cam/main.h > +++ b/src/cam/main.h > @@ -11,6 +11,7 @@ enum { > OptCamera = 'c', > OptCapture = 'C', > OptDisplay = 'D', > + OptSDL = 'S', > OptFile = 'F', > OptHelp = 'h', > OptInfo = 'I', > diff --git a/src/cam/meson.build b/src/cam/meson.build > index e8e2ae57..44202ef0 100644 > --- a/src/cam/meson.build > +++ b/src/cam/meson.build > @@ -32,11 +32,21 @@ cam_sources += files([ > ]) > endif > > +libsdl2 = dependency('SDL2', required : false) > + > +if libsdl2.found() > +cam_cpp_args += [ '-DHAVE_SDL' ] > +cam_sources += files([ > + 'sdl_sink.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..480df1b4 > --- /dev/null > +++ b/src/cam/sdl_sink.cpp > @@ -0,0 +1,125 @@ > +/* 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 "image.h" > + > +using namespace libcamera; > + > +SDLSink::SDLSink() > +{ > + memset(&sdlRect_, 0, sizeof(sdlRect_)); > +} > + > +SDLSink::~SDLSink() > +{ > + SDL_Quit(); > +} > + > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > +{ > + int ret = FrameSink::configure(cfg); > + if (ret < 0) > + return ret; > + > + if ((ret = SDL_Init(SDL_INIT_VIDEO))) { > + std::cout << "Could not initialize SDL - " << SDL_GetError() << "\n"; > + return ret; > + } > + > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width, > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > + if (!sdlScreen_) { > + std::cerr << "SDL: could not create window - exiting: " << SDL_GetError() << "\n"; > + return -1; is an errno more appropriate? Same for the others.. > + } > + > + sdlRenderer_ = SDL_CreateRenderer( > + sdlScreen_, -1, 0); > + if (!sdlRenderer_) { > + std::cerr << "SDL_CreateRenderer Error\n"; > + return -2; > + } > + > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width, > + sCfg.size.height); > + > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc(); > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) { > + pf = SDL_PIXELFORMAT_YUY2; > + } Is this some sort of conversion that most fourccs are compatible 'except' YUY2? I'd be wary if there is even a single difference, and it might be better to put in an explicit conversion table or map? Particularly as libcamera fourcc's are based on DRM fourccs... are SDL fourcc's defined as directly compatible there? I.e. there are frequently differences in byte ordering on the RGB/BGR formats that cause issues. > + > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"))) { > + std::cerr << "error: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n"; > + return -3; > + } > + > + sdlTexture_ = > + SDL_CreateTexture(sdlRenderer_, pf, > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width, > + sCfg.size.height); > + sdlRect_.w = sCfg.size.width; > + sdlRect_.h = sCfg.size.height; > + > + return 0; > +} > + > +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()) > + writeBuffer(buffer); > + > + return true; > +} > + > +void SDLSink::writeBuffer(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; > + > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2); > + SDL_RenderClear(sdlRenderer_); > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL); > + SDL_RenderPresent(sdlRenderer_); > + SDL_Event e; > + while (SDL_PollEvent(&e)) { > + if (e.type == SDL_QUIT) { // click close icon then quit > + kill(getpid(), SIGINT); > + } > + }; This seems like an odd place to be doing the event loop polling. I guess it's intentional to be able to 'hook' it in somewhere? I'd be tempted to pull it out to a separate function at least. Having never looked at SDL code before, I must say, - that looks like it's really quite easy to pull together a window and display! > + } > +} > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > new file mode 100644 > index 00000000..f4f843fa > --- /dev/null > +++ b/src/cam/sdl_sink.h > @@ -0,0 +1,40 @@ > +/* 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" > + > +class Image; > + > +class SDLSink : public FrameSink > +{ > +public: > + SDLSink(); > + ~SDLSink(); > + > + int configure(const libcamera::CameraConfiguration &cfg) override; > + > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > + > + bool processRequest(libcamera::Request *request) override; > + > +private: > + void writeBuffer(libcamera::FrameBuffer *buffer); > + > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; > + SDL_Window *sdlScreen_; > + SDL_Renderer *sdlRenderer_; > + SDL_Texture *sdlTexture_; > + SDL_Rect sdlRect_; > +}; > -- > 2.35.1 >
On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote: > Hi Eric, > > Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36) > > This adds more portability to existing cam sinks. You can pass a > > YUYV camera buffer for example and SDL will handle the pixel buffer > > conversion, although SDL does not support decompression for pixelformats > > like MJPEG. 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 I have not tested. > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where > > existing kms sink did not work. > > This sounds very helpful and useful to me... > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > Changes in v2: > > - Remove hardcoded pixel format from SDL_CreateTexture call > > --- > > src/cam/camera_session.cpp | 8 +++ > > src/cam/main.cpp | 6 ++ > > src/cam/main.h | 1 + > > src/cam/meson.build | 10 +++ > > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++ > > src/cam/sdl_sink.h | 40 ++++++++++++ > > 6 files changed, 190 insertions(+) > > create mode 100644 src/cam/sdl_sink.cpp > > create mode 100644 src/cam/sdl_sink.h > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > index 0428b538..30162dbd 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" > > > > @@ -187,6 +190,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..406d61bc 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[]) > > "display", ArgumentOptional, "connector", false, > > OptCamera); > > #endif > > + You can drop the blank line here. > > +#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" > > "If the file name ends with a '/', it sets the directory in which\n" > > diff --git a/src/cam/main.h b/src/cam/main.h > > index 62f7bbc9..a64f95a0 100644 > > --- a/src/cam/main.h > > +++ b/src/cam/main.h > > @@ -11,6 +11,7 @@ enum { > > OptCamera = 'c', > > OptCapture = 'C', > > OptDisplay = 'D', > > + OptSDL = 'S', > > OptFile = 'F', > > OptHelp = 'h', > > OptInfo = 'I', > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > index e8e2ae57..44202ef0 100644 > > --- a/src/cam/meson.build > > +++ b/src/cam/meson.build > > @@ -32,11 +32,21 @@ cam_sources += files([ > > ]) > > endif > > > > +libsdl2 = dependency('SDL2', required : false) > > + > > +if libsdl2.found() > > +cam_cpp_args += [ '-DHAVE_SDL' ] > > +cam_sources += files([ > > + 'sdl_sink.cpp' > > +]) The contents of the if..endif should be indented. > > +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..480df1b4 > > --- /dev/null > > +++ b/src/cam/sdl_sink.cpp > > @@ -0,0 +1,125 @@ > > +/* 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 "image.h" > > + > > +using namespace libcamera; > > + > > +SDLSink::SDLSink() > > +{ > > + memset(&sdlRect_, 0, sizeof(sdlRect_)); > > +} > > + > > +SDLSink::~SDLSink() > > +{ > > + SDL_Quit(); > > +} > > + > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > +{ > > + int ret = FrameSink::configure(cfg); > > + if (ret < 0) > > + return ret; > > + > > + if ((ret = SDL_Init(SDL_INIT_VIDEO))) { > > + std::cout << "Could not initialize SDL - " << SDL_GetError() << "\n"; > > + return ret; > > + } > > + > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width, > > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); As this shows the window, it may be more appropriate to do this in start(). Or maybe it can be created hidden, and shown in start() ? > > + if (!sdlScreen_) { > > + std::cerr << "SDL: could not create window - exiting: " << SDL_GetError() << "\n"; std::endl instead of "\n". > > + return -1; > > is an errno more appropriate? > Same for the others.. > > > + } > > + > > + sdlRenderer_ = SDL_CreateRenderer( > > + sdlScreen_, -1, 0); > > + if (!sdlRenderer_) { > > + std::cerr << "SDL_CreateRenderer Error\n"; > > + return -2; Is there a need to clean previous steps when an error occurs, or is SDL_Quit() enough ? We aim to have no memory leak reported by valgrind at program exit. > > + } > > + > > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width, > > + sCfg.size.height); > > + > > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc(); > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) { Please don't assign and test in the same statement. > > + pf = SDL_PIXELFORMAT_YUY2; > > + } > > Is this some sort of conversion that most fourccs are compatible > 'except' YUY2? I'd be wary if there is even a single difference, and > it might be better to put in an explicit conversion table or map? > > Particularly as libcamera fourcc's are based on DRM fourccs... are SDL > fourcc's defined as directly compatible there? I.e. there are frequently > differences in byte ordering on the RGB/BGR formats that cause issues. > > > + > > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"))) { No need to assign to ret. > > + std::cerr << "error: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n"; > > + return -3; > > + } > > + > > + sdlTexture_ = > > + SDL_CreateTexture(sdlRenderer_, pf, > > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width, > > + sCfg.size.height); > > + sdlRect_.w = sCfg.size.width; > > + sdlRect_.h = sCfg.size.height; > > + > > + return 0; > > +} > > + > > +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()) > > + writeBuffer(buffer); > > + > > + return true; > > +} > > + > > +void SDLSink::writeBuffer(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; > > + > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2); Does this cause a copy ? If so, is there a zero-copy option ? > > + SDL_RenderClear(sdlRenderer_); > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL); > > + SDL_RenderPresent(sdlRenderer_); > > + SDL_Event e; > > + while (SDL_PollEvent(&e)) { > > + if (e.type == SDL_QUIT) { // click close icon then quit > > + kill(getpid(), SIGINT); > > + } > > + }; > > This seems like an odd place to be doing the event loop polling. I guess > it's intentional to be able to 'hook' it in somewhere? > > I'd be tempted to pull it out to a separate function at least. It would be nice to integrate that with the main event loop. > Having never looked at SDL code before, I must say, - that looks like > it's really quite easy to pull together a window and display! > > > + } > > +} > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > new file mode 100644 > > index 00000000..f4f843fa > > --- /dev/null > > +++ b/src/cam/sdl_sink.h > > @@ -0,0 +1,40 @@ > > +/* 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" > > + > > +class Image; > > + > > +class SDLSink : public FrameSink > > +{ > > +public: > > + SDLSink(); > > + ~SDLSink(); > > + > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > + > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > + > > + bool processRequest(libcamera::Request *request) override; > > + > > +private: > > + void writeBuffer(libcamera::FrameBuffer *buffer); > > + > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; > > + SDL_Window *sdlScreen_; > > + SDL_Renderer *sdlRenderer_; > > + SDL_Texture *sdlTexture_; > > + SDL_Rect sdlRect_; > > +};
On Wed, 23 Mar 2022 at 00:36, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote: > > Hi Eric, > > > > Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36) > > > This adds more portability to existing cam sinks. You can pass a > > > YUYV camera buffer for example and SDL will handle the pixel buffer > > > conversion, although SDL does not support decompression for pixelformats > > > like MJPEG. 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 I have not tested. > > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where > > > existing kms sink did not work. > > > > This sounds very helpful and useful to me... Yeah, I don't know if I captured in it in commit message well enough, but it also behaves as a kmsdrm sink also, it's one of the reasons why Doom generally is so portable, many forks use SDL. It's solving a few problems for me. > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > Changes in v2: > > > - Remove hardcoded pixel format from SDL_CreateTexture call > > > --- > > > src/cam/camera_session.cpp | 8 +++ > > > src/cam/main.cpp | 6 ++ > > > src/cam/main.h | 1 + > > > src/cam/meson.build | 10 +++ > > > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++ > > > src/cam/sdl_sink.h | 40 ++++++++++++ > > > 6 files changed, 190 insertions(+) > > > create mode 100644 src/cam/sdl_sink.cpp > > > create mode 100644 src/cam/sdl_sink.h > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > index 0428b538..30162dbd 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" > > > > > > @@ -187,6 +190,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..406d61bc 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > "display", ArgumentOptional, "connector", false, > > > OptCamera); > > > #endif > > > + > > You can drop the blank line here. Sounds good. > > > > +#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" > > > "If the file name ends with a '/', it sets the directory in which\n" > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > index 62f7bbc9..a64f95a0 100644 > > > --- a/src/cam/main.h > > > +++ b/src/cam/main.h > > > @@ -11,6 +11,7 @@ enum { > > > OptCamera = 'c', > > > OptCapture = 'C', > > > OptDisplay = 'D', > > > + OptSDL = 'S', > > > OptFile = 'F', > > > OptHelp = 'h', > > > OptInfo = 'I', > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > index e8e2ae57..44202ef0 100644 > > > --- a/src/cam/meson.build > > > +++ b/src/cam/meson.build > > > @@ -32,11 +32,21 @@ cam_sources += files([ > > > ]) > > > endif > > > > > > +libsdl2 = dependency('SDL2', required : false) > > > + > > > +if libsdl2.found() > > > +cam_cpp_args += [ '-DHAVE_SDL' ] > > > +cam_sources += files([ > > > + 'sdl_sink.cpp' > > > +]) > > The contents of the if..endif should be indented. The main reason I didn't is because in the if block above it doesn't, but I can add indenting. > > > > +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..480df1b4 > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.cpp > > > @@ -0,0 +1,125 @@ > > > +/* 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 "image.h" > > > + > > > +using namespace libcamera; > > > + > > > +SDLSink::SDLSink() > > > +{ > > > + memset(&sdlRect_, 0, sizeof(sdlRect_)); > > > +} > > > + > > > +SDLSink::~SDLSink() > > > +{ > > > + SDL_Quit(); > > > +} > > > + > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > > +{ > > > + int ret = FrameSink::configure(cfg); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if ((ret = SDL_Init(SDL_INIT_VIDEO))) { > > > + std::cout << "Could not initialize SDL - " << SDL_GetError() << "\n"; > > > + return ret; > > > + } > > > + > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > > + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width, > > > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > As this shows the window, it may be more appropriate to do this in > start(). Or maybe it can be created hidden, and shown in start() ? Hmmm, I think if we create a start(), it probably makes sense to leave pixel format stuff in configure() here and do the rest in start(). > > > > + if (!sdlScreen_) { > > > + std::cerr << "SDL: could not create window - exiting: " << SDL_GetError() << "\n"; > > std::endl instead of "\n". > > > > + return -1; > > > > is an errno more appropriate? > > Same for the others.. > > > > > + } > > > + > > > + sdlRenderer_ = SDL_CreateRenderer( > > > + sdlScreen_, -1, 0); > > > + if (!sdlRenderer_) { > > > + std::cerr << "SDL_CreateRenderer Error\n"; > > > + return -2; > > Is there a need to clean previous steps when an error occurs, or is > SDL_Quit() enough ? We aim to have no memory leak reported by valgrind > at program exit. I'm adding one more cleanup to the destructor. Checked with valgrind and address sanitizer for as many scenarios I could recreate. > > > > + } > > > + > > > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width, > > > + sCfg.size.height); > > > + > > > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc(); > > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) { > > Please don't assign and test in the same statement. ok. > > > > + pf = SDL_PIXELFORMAT_YUY2; > > > + } > > > > Is this some sort of conversion that most fourccs are compatible > > 'except' YUY2? I'd be wary if there is even a single difference, and > > it might be better to put in an explicit conversion table or map? The thing is... SDL has maps for this stuff, so I didn't want to duplicate effort by creating extra maps. These enums are compatible in that they are both 4 chars encoded in a single 32-bit int, but sometimes they may use a different 4 char combo as in the YUY2/YUYV case. The SDL_GetPixelFormatName identifies if the 4 chars are not listed as a compatible SDL pixel format. I'd be open to extending this to a libcamera map as we encounter more that don't exactly match. Two examples of mapping code that pre-exist: $ wc -l include/SDL_pixels.h 644 include/SDL_pixels.h $ wc -l include/linux/drm_fourcc.h 1479 include/linux/drm_fourcc.h A fully complete libdrm -> SDL2 pixel format map, feels like an SDL2 patch rather than a libcamera patch. > > > > Particularly as libcamera fourcc's are based on DRM fourccs... are SDL > > fourcc's defined as directly compatible there? I.e. there are frequently > > differences in byte ordering on the RGB/BGR formats that cause issues. I mitigated against all issues by returning a negative error code at this point and printing: "error: SDL_PIXELFORMAT_UNKNOWN, no"... I'm open to suggestions of what that error message might be, maybe it should add a suggestion to add to the libcamera DRM -> SDL2 map? Still feels like this map in the long term is more in the scope of either libdrm or SDL2. > > > > > + > > > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"))) { > > No need to assign to ret. Ok > > > > + std::cerr << "error: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n"; > > > + return -3; > > > + } > > > + > > > + sdlTexture_ = > > > + SDL_CreateTexture(sdlRenderer_, pf, > > > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width, > > > + sCfg.size.height); > > > + sdlRect_.w = sCfg.size.width; > > > + sdlRect_.h = sCfg.size.height; > > > + > > > + return 0; > > > +} > > > + > > > +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()) > > > + writeBuffer(buffer); > > > + > > > + return true; > > > +} > > > + > > > +void SDLSink::writeBuffer(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; > > > + > > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2); > > Does this cause a copy ? If so, is there a zero-copy option ? No evidence of zero-copy from what I can see, I'll keep looking. > > > > + SDL_RenderClear(sdlRenderer_); > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL); > > > + SDL_RenderPresent(sdlRenderer_); > > > + SDL_Event e; > > > + while (SDL_PollEvent(&e)) { > > > + if (e.type == SDL_QUIT) { // click close icon then quit > > > + kill(getpid(), SIGINT); > > > + } > > > + }; > > > > This seems like an odd place to be doing the event loop polling. I guess > > it's intentional to be able to 'hook' it in somewhere? > > > > I'd be tempted to pull it out to a separate function at least. Yup will do. I also think calling this outside the for loop in processRequest might be a little better. It's only actually needed in the desktop application case when clicking the x button and resizing the window so far. SDL_QUIT means user clicked x or quit. > > It would be nice to integrate that with the main event loop. Might need assistance to do that in a clean way. > > > Having never looked at SDL code before, I must say, - that looks like > > it's really quite easy to pull together a window and display! Yeah it saves you writing 1000s of lines of code alright so one can focus a little more on the camera stuff. > > > > > + } > > > +} > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > new file mode 100644 > > > index 00000000..f4f843fa > > > --- /dev/null > > > +++ b/src/cam/sdl_sink.h > > > @@ -0,0 +1,40 @@ > > > +/* 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" > > > + > > > +class Image; > > > + > > > +class SDLSink : public FrameSink > > > +{ > > > +public: > > > + SDLSink(); > > > + ~SDLSink(); > > > + > > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > > + > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > > + > > > + bool processRequest(libcamera::Request *request) override; > > > + > > > +private: > > > + void writeBuffer(libcamera::FrameBuffer *buffer); > > > + > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; > > > + SDL_Window *sdlScreen_; > > > + SDL_Renderer *sdlRenderer_; > > > + SDL_Texture *sdlTexture_; > > > + SDL_Rect sdlRect_; > > > +}; > > -- > Regards, > > Laurent Pinchart >
Hi Eric, On Wed, Mar 23, 2022 at 02:49:53PM +0000, Eric Curtin wrote: > On Wed, 23 Mar 2022 at 00:36, Laurent Pinchart wrote: > > On Tue, Mar 22, 2022 at 11:34:28PM +0000, Kieran Bingham via libcamera-devel wrote: > > > Quoting Eric Curtin via libcamera-devel (2022-03-22 19:09:36) > > > > This adds more portability to existing cam sinks. You can pass a > > > > YUYV camera buffer for example and SDL will handle the pixel buffer > > > > conversion, although SDL does not support decompression for pixelformats > > > > like MJPEG. 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 I have not tested. > > > > Also tested on simpledrm raspberry pi 4 framebuffer successfully where > > > > existing kms sink did not work. > > > > > > This sounds very helpful and useful to me... > > Yeah, I don't know if I captured in it in commit message well enough, > but it also behaves as a kmsdrm sink also, it's one of the reasons why > Doom generally is so portable, many forks use SDL. It's solving a few > problems for me. > > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > Changes in v2: > > > > - Remove hardcoded pixel format from SDL_CreateTexture call > > > > --- > > > > src/cam/camera_session.cpp | 8 +++ > > > > src/cam/main.cpp | 6 ++ > > > > src/cam/main.h | 1 + > > > > src/cam/meson.build | 10 +++ > > > > src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++ > > > > src/cam/sdl_sink.h | 40 ++++++++++++ > > > > 6 files changed, 190 insertions(+) > > > > create mode 100644 src/cam/sdl_sink.cpp > > > > create mode 100644 src/cam/sdl_sink.h > > > > > > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > > > > index 0428b538..30162dbd 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" > > > > > > > > @@ -187,6 +190,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..406d61bc 100644 > > > > --- a/src/cam/main.cpp > > > > +++ b/src/cam/main.cpp > > > > @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[]) > > > > "display", ArgumentOptional, "connector", false, > > > > OptCamera); > > > > #endif > > > > + > > > > You can drop the blank line here. > > Sounds good. > > > > > +#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" > > > > "If the file name ends with a '/', it sets the directory in which\n" > > > > diff --git a/src/cam/main.h b/src/cam/main.h > > > > index 62f7bbc9..a64f95a0 100644 > > > > --- a/src/cam/main.h > > > > +++ b/src/cam/main.h > > > > @@ -11,6 +11,7 @@ enum { > > > > OptCamera = 'c', > > > > OptCapture = 'C', > > > > OptDisplay = 'D', > > > > + OptSDL = 'S', > > > > OptFile = 'F', > > > > OptHelp = 'h', > > > > OptInfo = 'I', > > > > diff --git a/src/cam/meson.build b/src/cam/meson.build > > > > index e8e2ae57..44202ef0 100644 > > > > --- a/src/cam/meson.build > > > > +++ b/src/cam/meson.build > > > > @@ -32,11 +32,21 @@ cam_sources += files([ > > > > ]) > > > > endif > > > > > > > > +libsdl2 = dependency('SDL2', required : false) > > > > + > > > > +if libsdl2.found() > > > > +cam_cpp_args += [ '-DHAVE_SDL' ] > > > > +cam_sources += files([ > > > > + 'sdl_sink.cpp' > > > > +]) > > > > The contents of the if..endif should be indented. > > The main reason I didn't is because in the if block above it doesn't, > but I can add indenting. Oops. I'll fix that. > > > > +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..480df1b4 > > > > --- /dev/null > > > > +++ b/src/cam/sdl_sink.cpp > > > > @@ -0,0 +1,125 @@ > > > > +/* 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 "image.h" > > > > + > > > > +using namespace libcamera; > > > > + > > > > +SDLSink::SDLSink() > > > > +{ > > > > + memset(&sdlRect_, 0, sizeof(sdlRect_)); > > > > +} > > > > + > > > > +SDLSink::~SDLSink() > > > > +{ > > > > + SDL_Quit(); > > > > +} > > > > + > > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) > > > > +{ > > > > + int ret = FrameSink::configure(cfg); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if ((ret = SDL_Init(SDL_INIT_VIDEO))) { > > > > + std::cout << "Could not initialize SDL - " << SDL_GetError() << "\n"; > > > > + return ret; > > > > + } > > > > + > > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0); > > > > + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, > > > > + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width, > > > > + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); > > > > As this shows the window, it may be more appropriate to do this in > > start(). Or maybe it can be created hidden, and shown in start() ? > > Hmmm, I think if we create a start(), it probably makes sense to leave > pixel format stuff in configure() here and do the rest in start(). Also, if we capture from two cameras, and set the --sdl option for both, we'll end up calling SDL_Init() multiple times, I suppose that won't work nicely. The KMS sink has the same issue, so you don't need to solve it here. > > > > + if (!sdlScreen_) { > > > > + std::cerr << "SDL: could not create window - exiting: " << SDL_GetError() << "\n"; > > > > std::endl instead of "\n". > > > > > > + return -1; > > > > > > is an errno more appropriate? > > > Same for the others.. > > > > > > > + } > > > > + > > > > + sdlRenderer_ = SDL_CreateRenderer( > > > > + sdlScreen_, -1, 0); > > > > + if (!sdlRenderer_) { > > > > + std::cerr << "SDL_CreateRenderer Error\n"; > > > > + return -2; > > > > Is there a need to clean previous steps when an error occurs, or is > > SDL_Quit() enough ? We aim to have no memory leak reported by valgrind > > at program exit. > > I'm adding one more cleanup to the destructor. Checked with valgrind > and address sanitizer for as many scenarios I could recreate. Thank you. > > > > + } > > > > + > > > > + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width, > > > > + sCfg.size.height); > > > > + > > > > + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc(); > > > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) { > > > > Please don't assign and test in the same statement. > > ok. > > > > > + pf = SDL_PIXELFORMAT_YUY2; > > > > + } > > > > > > Is this some sort of conversion that most fourccs are compatible > > > 'except' YUY2? I'd be wary if there is even a single difference, and > > > it might be better to put in an explicit conversion table or map? > > The thing is... SDL has maps for this stuff, so I didn't want to > duplicate effort by creating extra maps. These enums are compatible in > that they are both 4 chars encoded in a single 32-bit int, but > sometimes they may use a different 4 char combo as in the YUY2/YUYV > case. The SDL_GetPixelFormatName identifies if the 4 chars are not > listed as a compatible SDL pixel format. I'd be open to extending this > to a libcamera map as we encounter more that don't exactly match. Two > examples of mapping code that pre-exist: > > $ wc -l include/SDL_pixels.h > 644 include/SDL_pixels.h > > $ wc -l include/linux/drm_fourcc.h > 1479 include/linux/drm_fourcc.h > > A fully complete libdrm -> SDL2 pixel format map, feels like an SDL2 > patch rather than a libcamera patch. > > > > > > > Particularly as libcamera fourcc's are based on DRM fourccs... are SDL > > > fourcc's defined as directly compatible there? I.e. there are frequently > > > differences in byte ordering on the RGB/BGR formats that cause issues. > > I mitigated against all issues by returning a negative error code at > this point and printing: > > "error: SDL_PIXELFORMAT_UNKNOWN, no"... > > I'm open to suggestions of what that error message might be, maybe it > should add a suggestion to add to the libcamera DRM -> SDL2 map? Still > feels like this map in the long term is more in the scope of either > libdrm or SDL2. > > > > > + > > > > + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"))) { > > > > No need to assign to ret. > > Ok > > > > > + std::cerr << "error: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n"; > > > > + return -3; > > > > + } > > > > + > > > > + sdlTexture_ = > > > > + SDL_CreateTexture(sdlRenderer_, pf, > > > > + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width, > > > > + sCfg.size.height); > > > > + sdlRect_.w = sCfg.size.width; > > > > + sdlRect_.h = sCfg.size.height; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +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()) > > > > + writeBuffer(buffer); > > > > + > > > > + return true; > > > > +} > > > > + > > > > +void SDLSink::writeBuffer(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; > > > > + > > > > + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2); > > > > Does this cause a copy ? If so, is there a zero-copy option ? > > No evidence of zero-copy from what I can see, I'll keep looking. No problem, I was just wondering. > > > > + SDL_RenderClear(sdlRenderer_); > > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL); > > > > + SDL_RenderPresent(sdlRenderer_); > > > > + SDL_Event e; > > > > + while (SDL_PollEvent(&e)) { > > > > + if (e.type == SDL_QUIT) { // click close icon then quit > > > > + kill(getpid(), SIGINT); > > > > + } > > > > + }; > > > > > > This seems like an odd place to be doing the event loop polling. I guess > > > it's intentional to be able to 'hook' it in somewhere? > > > > > > I'd be tempted to pull it out to a separate function at least. > > Yup will do. I also think calling this outside the for loop in > processRequest might be a little better. It's only actually needed in > the desktop application case when clicking the x button and resizing > the window so far. SDL_QUIT means user clicked x or quit. > > > It would be nice to integrate that with the main event loop. > > Might need assistance to do that in a clean way. It seems it will get messy, SDL doesn't have the ability to expose events through a file descriptor :-S I think we can get libevent to call use back every time it wakes up by adding an event that is neither a read or write event: EventLoop::addEvent(-1, 0, callback); In the callback function, you can then call SDL_PollEvent() in a while loop like above. This class doesn't have access to the event loop though, so it would require a bit of refactoring. > > > Having never looked at SDL code before, I must say, - that looks like > > > it's really quite easy to pull together a window and display! > > Yeah it saves you writing 1000s of lines of code alright so one can > focus a little more on the camera stuff. > > > > > + } > > > > +} > > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h > > > > new file mode 100644 > > > > index 00000000..f4f843fa > > > > --- /dev/null > > > > +++ b/src/cam/sdl_sink.h > > > > @@ -0,0 +1,40 @@ > > > > +/* 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" > > > > + > > > > +class Image; > > > > + > > > > +class SDLSink : public FrameSink > > > > +{ > > > > +public: > > > > + SDLSink(); > > > > + ~SDLSink(); > > > > + > > > > + int configure(const libcamera::CameraConfiguration &cfg) override; > > > > + > > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override; > > > > + > > > > + bool processRequest(libcamera::Request *request) override; > > > > + > > > > +private: > > > > + void writeBuffer(libcamera::FrameBuffer *buffer); > > > > + > > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; > > > > + SDL_Window *sdlScreen_; > > > > + SDL_Renderer *sdlRenderer_; > > > > + SDL_Texture *sdlTexture_; > > > > + SDL_Rect sdlRect_; > > > > +};
diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp index 0428b538..30162dbd 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" @@ -187,6 +190,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..406d61bc 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -138,6 +138,12 @@ int CamApp::parseOptions(int argc, char *argv[]) "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" "If the file name ends with a '/', it sets the directory in which\n" diff --git a/src/cam/main.h b/src/cam/main.h index 62f7bbc9..a64f95a0 100644 --- a/src/cam/main.h +++ b/src/cam/main.h @@ -11,6 +11,7 @@ enum { OptCamera = 'c', OptCapture = 'C', OptDisplay = 'D', + OptSDL = 'S', OptFile = 'F', OptHelp = 'h', OptInfo = 'I', diff --git a/src/cam/meson.build b/src/cam/meson.build index e8e2ae57..44202ef0 100644 --- a/src/cam/meson.build +++ b/src/cam/meson.build @@ -32,11 +32,21 @@ cam_sources += files([ ]) endif +libsdl2 = dependency('SDL2', required : false) + +if libsdl2.found() +cam_cpp_args += [ '-DHAVE_SDL' ] +cam_sources += files([ + 'sdl_sink.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..480df1b4 --- /dev/null +++ b/src/cam/sdl_sink.cpp @@ -0,0 +1,125 @@ +/* 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 "image.h" + +using namespace libcamera; + +SDLSink::SDLSink() +{ + memset(&sdlRect_, 0, sizeof(sdlRect_)); +} + +SDLSink::~SDLSink() +{ + SDL_Quit(); +} + +int SDLSink::configure(const libcamera::CameraConfiguration &cfg) +{ + int ret = FrameSink::configure(cfg); + if (ret < 0) + return ret; + + if ((ret = SDL_Init(SDL_INIT_VIDEO))) { + std::cout << "Could not initialize SDL - " << SDL_GetError() << "\n"; + return ret; + } + + const libcamera::StreamConfiguration &sCfg = cfg.at(0); + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED, + SDL_WINDOWPOS_UNDEFINED, sCfg.size.width, + sCfg.size.height, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE); + if (!sdlScreen_) { + std::cerr << "SDL: could not create window - exiting: " << SDL_GetError() << "\n"; + return -1; + } + + sdlRenderer_ = SDL_CreateRenderer( + sdlScreen_, -1, 0); + if (!sdlRenderer_) { + std::cerr << "SDL_CreateRenderer Error\n"; + return -2; + } + + SDL_RenderSetLogicalSize(sdlRenderer_, sCfg.size.width, + sCfg.size.height); + + SDL_PixelFormatEnum pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc(); + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) { + pf = SDL_PIXELFORMAT_YUY2; + } + + if (!(ret = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"))) { + std::cerr << "error: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n"; + return -3; + } + + sdlTexture_ = + SDL_CreateTexture(sdlRenderer_, pf, + SDL_TEXTUREACCESS_STREAMING, sCfg.size.width, + sCfg.size.height); + sdlRect_.w = sCfg.size.width; + sdlRect_.h = sCfg.size.height; + + return 0; +} + +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()) + writeBuffer(buffer); + + return true; +} + +void SDLSink::writeBuffer(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; + + SDL_UpdateTexture(sdlTexture_, &sdlRect_, data.data(), sdlRect_.w * 2); + SDL_RenderClear(sdlRenderer_); + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL); + SDL_RenderPresent(sdlRenderer_); + SDL_Event e; + while (SDL_PollEvent(&e)) { + if (e.type == SDL_QUIT) { // click close icon then quit + kill(getpid(), SIGINT); + } + }; + } +} diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h new file mode 100644 index 00000000..f4f843fa --- /dev/null +++ b/src/cam/sdl_sink.h @@ -0,0 +1,40 @@ +/* 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" + +class Image; + +class SDLSink : public FrameSink +{ +public: + SDLSink(); + ~SDLSink(); + + int configure(const libcamera::CameraConfiguration &cfg) override; + + void mapBuffer(libcamera::FrameBuffer *buffer) override; + + bool processRequest(libcamera::Request *request) override; + +private: + void writeBuffer(libcamera::FrameBuffer *buffer); + + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_; + SDL_Window *sdlScreen_; + SDL_Renderer *sdlRenderer_; + SDL_Texture *sdlTexture_; + SDL_Rect sdlRect_; +};
This adds more portability to existing cam sinks. You can pass a YUYV camera buffer for example and SDL will handle the pixel buffer conversion, although SDL does not support decompression for pixelformats like MJPEG. 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 I have not tested. Also tested on simpledrm raspberry pi 4 framebuffer successfully where existing kms sink did not work. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- Changes in v2: - Remove hardcoded pixel format from SDL_CreateTexture call --- src/cam/camera_session.cpp | 8 +++ src/cam/main.cpp | 6 ++ src/cam/main.h | 1 + src/cam/meson.build | 10 +++ src/cam/sdl_sink.cpp | 125 +++++++++++++++++++++++++++++++++++++ src/cam/sdl_sink.h | 40 ++++++++++++ 6 files changed, 190 insertions(+) create mode 100644 src/cam/sdl_sink.cpp create mode 100644 src/cam/sdl_sink.h