[libcamera-devel] [PATCH v4] cam: sdl_sink: Add SDL sink
Eric Curtin
ecurtin at redhat.com
Wed Mar 30 17:11:24 CEST 2022
On Tue, 29 Mar 2022 at 21:09, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Tue, Mar 29, 2022 at 05:45:57PM +0100, Eric Curtin wrote:
> > On Tue, 29 Mar 2022 at 16:10, Laurent Pinchart wrote:
> > > On Tue, Mar 29, 2022 at 12:17:25PM +0100, Eric Curtin via libcamera-devel wrote:
> > > > 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. Can also be used as kmsdrm sink.
> > > >
> > > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > > ---
> > > > Changes in v2:
> > > > - Remove hardcoded pixel format from SDL_CreateTexture call
> > > >
> > > > Changes in v3:
> > > > - Drop blank line
> > > > - The contents of the if..endif are indented
> > > > - Split configure function into start and configure
> > > > - Add SDL_DestroyRenderer
> > > > - Remove assign and test in the same statement
> > > >
> > > > Changes in v4:
> > > > - Add processSDLEvents as a timed event in the evloop
> > > > ---
> > > > src/cam/camera_session.cpp | 8 +++
> > > > src/cam/event_loop.cpp | 10 ++-
> > > > src/cam/event_loop.h | 1 +
> > > > src/cam/main.cpp | 5 ++
> > > > src/cam/main.h | 1 +
> > > > src/cam/meson.build | 10 +++
> > > > src/cam/sdl_sink.cpp | 142 +++++++++++++++++++++++++++++++++++++
> > > > src/cam/sdl_sink.h | 42 +++++++++++
> > > > 8 files changed, 218 insertions(+), 1 deletion(-)
> > > > 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/event_loop.cpp b/src/cam/event_loop.cpp
> > > > index e25784c0..41339d4e 100644
> > > > --- a/src/cam/event_loop.cpp
> > > > +++ b/src/cam/event_loop.cpp
> > > > @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
> > > > return;
> > > > }
> > > >
> > > > - int ret = event_add(event->event_, nullptr);
> > > > + struct timeval *tp = nullptr;
> > > > + struct timeval tv;
> > > > + if (events == EV_PERSIST) {
> > > > + tp = &tv;
> > > > + tv.tv_sec = 0;
> > > > + tv.tv_usec = 10000; /* every 10 ms */
> > > > + }
> > >
> > > Could you please split the change to the event loop to a separate patch,
> > > with an explanation of what it does in the commit message ? This isn't
> > > trivial to understand.
> >
> > Ok.
> >
> > > > +
> > > > + int ret = event_add(event->event_, tp);
> > > > if (ret < 0) {
> > > > std::cerr << "Failed to add event for fd " << fd << std::endl;
> > > > return;
> > > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > > > index a4613eb2..8fd9bd20 100644
> > > > --- a/src/cam/event_loop.h
> > > > +++ b/src/cam/event_loop.h
> > > > @@ -20,6 +20,7 @@ class EventLoop
> > > > {
> > > > public:
> > > > enum EventType {
> > > > + Default = 0, /* the event can be triggered only by a timeout or by manual activation */
> > > > Read = 1,
> > > > Write = 2,
> > > > };
> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > > index c7f664b9..1d62a64a 100644
> > > > --- a/src/cam/main.cpp
> > > > +++ b/src/cam/main.cpp
> > > > @@ -137,6 +137,11 @@ 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..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',
> > >
> > > Let's keep the options sorted.
> >
> > Ok.
> >
> > > > OptFile = 'F',
> > > > OptHelp = 'h',
> > > > OptInfo = 'I',
> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > > index e8e2ae57..bd536c5b 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' ]
> > >
> > > No space within the [ ].
> >
> > Ok.
> >
> > > > + 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..6c6e37d0
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.cpp
> > > > @@ -0,0 +1,142 @@
> > > > +/* 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 "event_loop.h"
> > > > +#include "image.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +SDLSink::SDLSink()
> > > > + : sdlRenderer_(0)
> > >
> > > It's a pointer, should be initialized to nullptr, not 0.
> >
> > Ok.
> >
> > > > +{
> > > > + memset(&sdlRect_, 0, sizeof(sdlRect_));
> > >
> > > This could be initialized with
> > >
> > > sdlRect_({})
> > >
> > > in the constructor member initializers list.
> >
> > Ok.
> >
> > > > +}
> > > > +
> > > > +SDLSink::~SDLSink()
> > > > +{
> > > > + if (sdlRenderer_)
> > > > + SDL_DestroyRenderer(sdlRenderer_);
> > > > + SDL_Quit();
> > >
> > > Shouldn't the cleanup go to stop(), now that you have the initialization
> > > in start() ?
> >
> > Yes thanks for spotting.
> >
> > > No need for any cleanup on sdlScreen_ and sdlTexture_ ?
> >
> > No need.
>
> Maybe a comment would be nice to state this, so that it looks less like
> it has been forgotten to someone not familiar with SDL.
It's funny actually, was trying different combinations today with
valgrind, address sanitizer, the only cleanup function you actually
need is SDL_Quit, it seems to clean up the rest automatically. I think
I'll put them all in explicitly though, just to alleviate future
readers getting worried. Some SDL docs call them all, some don't.
>
> > > > +}
> > > > +
> > > > +int SDLSink::configure(const libcamera::CameraConfiguration &cfg)
> > > > +{
> > > > + int ret = FrameSink::configure(cfg);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + const libcamera::StreamConfiguration &sCfg = cfg.at(0);
> > > > + pf = (SDL_PixelFormatEnum)sCfg.pixelFormat.fourcc();
> > > > + if (pf == SDL_DEFINE_PIXELFOURCC('Y', 'U', 'Y', 'V')) {
> > > > + pf = SDL_PIXELFORMAT_YUY2;
> > > > + } else if (int ne = strcmp(SDL_GetPixelFormatName(pf), "SDL_PIXELFORMAT_UNKNOWN"); !ne) {
> > >
> > > No variable declaration in an if () statement.
> >
> > Ok.
> >
> > > > + std::cerr << "SDL_GetPixelFormatName error - exiting: SDL_PIXELFORMAT_UNKNOWN, no " << sCfg.pixelFormat.toString() << " support\n";
> > >
> > > Those two lines are too long.
> >
> > Ok.
> >
> > > Replace \n with std::endl
> > >
> > > > + return -EINVAL;
> > > > + }
> > >
> > > I don't think this is right. SDL pixel formats don't map directly to the
> > > 4CC values used by libcamera. You need a translation table.
> >
> > I can start one, I won't be able to test it much as I don't have the
> > hardware to do that but I can start.
>
> You don't have to handle all formats, you can limit the translation to
> the format(s) you can test (or possibly adding a few of the most common
> other formats). More formats can always be added later.
>
> > > > +
> > > > + sdlRect_.w = sCfg.size.width;
> > > > + sdlRect_.h = sCfg.size.height;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int SDLSink::start()
> > > > +{
> > > > + int ret = SDL_Init(SDL_INIT_VIDEO);
> > > > + if (ret) {
> > > > + std::cerr << "SDL_Init error - exiting: " << SDL_GetError() << std::endl;
> > >
> > > std::cerr << "Failed to initialize SDL: " << SDL_GetError()
> > > << std::endl;
> > >
> > > and similarly below.
> >
> > Ok
> >
> > > > + return ret;
> > > > + }
> > > > +
> > > > + sdlScreen_ = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> > > > + SDL_WINDOWPOS_UNDEFINED, sdlRect_.w,
> > > > + sdlRect_.h, SDL_WINDOW_SHOWN | SDL_WINDOW_RESIZABLE);
> > > > + if (!sdlScreen_) {
> > > > + std::cerr << "SDL_CreateWindow error - exiting: " << SDL_GetError() << std::endl;
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + sdlRenderer_ = SDL_CreateRenderer(
> > > > + sdlScreen_, -1, 0);
> > > > + if (!sdlRenderer_) {
> > > > + std::cerr << "SDL_CreateRenderer error - exiting: " << SDL_GetError() << std::endl;
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + SDL_RenderSetLogicalSize(sdlRenderer_, sdlRect_.w,
> > > > + sdlRect_.h);
> > > > +
> > > > + sdlTexture_ =
> > > > + SDL_CreateTexture(sdlRenderer_, pf,
> > > > + SDL_TEXTUREACCESS_STREAMING, sdlRect_.w,
> > > > + sdlRect_.h);
> > > > + EventLoop::instance()->addEvent(-1, EventLoop::Default,
> > > > + std::bind(&SDLSink::processSDLEvents, this));
> > > > +
> > > > + 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);
> > >
> > > Will this really work properly if the request contains multiple buffers
> > > ?
> >
> > I will write more code to solve this.
>
> The KMS sink doesn't support this either, so a simple option would be to
> handle the first stream only, or to reject multi-stream configurations
> in the configure() function. Another option could be to create multiple
> SDL windows, one for each stream, but that will become complicated I
> suppose.
>
> > > > +
> > > > + 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 */
> > > > + kill(getpid(), SIGINT);
> > >
> > > That's too harsh. Calling the exit() function of the event loop should
> > > be enough.
> >
> > I'll try that, my first thought was to try and make it behave like
> > when you type Ctrl-C as much as possible, but yeah it's weird this
> > way, a process calling it's own signal handler.
> >
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +void SDLSink::writeBuffer(FrameBuffer *buffer)
> > >
> > > This function doesn't "write" a buffer, it should be named differently.
> >
> > renderBuffer maybe?
>
> Sounds good to me.
>
> > > > +{
> > > > + 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);
> > >
> > > Is the last argument format-dependent ?
> >
> > I'll check, lacking test hardware of course though, I only have UVC
> > YUYV cameras which isn't ideal.
> >
> > > > + SDL_RenderClear(sdlRenderer_);
> > > > + SDL_RenderCopy(sdlRenderer_, sdlTexture_, NULL, NULL);
> > >
> > > s/NULL/nullptr/
> >
> > Ok
> >
> > > > + SDL_RenderPresent(sdlRenderer_);
> > > > + }
> > > > +}
> > > > diff --git a/src/cam/sdl_sink.h b/src/cam/sdl_sink.h
> > > > new file mode 100644
> > > > index 00000000..6f20e2d6
> > > > --- /dev/null
> > > > +++ b/src/cam/sdl_sink.h
> > > > @@ -0,0 +1,42 @@
> > > > +/* 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;
> > > > + int start() override;
> > > > + void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > > > +
> > > > + bool processRequest(libcamera::Request *request) override;
> > > > +
> > > > +private:
> > > > + void writeBuffer(libcamera::FrameBuffer *buffer);
> > > > + void processSDLEvents();
> > > > +
> > > > + std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
> > >
> > > I'd add a blank line here.
> >
> > Ok
> >
> > > > + SDL_Window *sdlScreen_;
> > > > + SDL_Renderer *sdlRenderer_;
> > > > + SDL_Texture *sdlTexture_;
> > > > + SDL_Rect sdlRect_;
> > > > + SDL_PixelFormatEnum pf;
> > >
> > > s/pf/pixelFormat_/
> >
> > Ok.
> >
> > > > +};
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list