[libcamera-devel] [PATCH v2] cam: sdl_sink: Add SDL sink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 24 17:30:42 CET 2022
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 at 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_;
> > > > +};
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list