[libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over SDL2_image
Eric Curtin
ecurtin at redhat.com
Wed Jul 20 15:54:29 CEST 2022
On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Tue, Jul 19, 2022 at 10:17:46AM +0100, Eric Curtin wrote:
> > We were using the libjpeg functionality of SDL2_image only, instead just
> > use libjpeg directly to reduce our dependancy count, it is a more
> > commonly available library.
> >
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> > Changes in v4:
> > - decompress function takes a Span<const uint8_t>&
> >
> > Changes in v3:
> > - create JpegErrorManager struct
> > - change c cast to reinterpret_cast
> >
> > Changes in v2:
> > - alphabetically sorted various orderings
> > - pitch_ is const again
> > - added setjmp logic for error handling in libjpeg
> > - rgbdata_ to unique_ptr and renamed to rgb_
> > - removed a copy from buffer to rgb_
> > - removed a destructor
> > ---
> > README.rst | 2 +-
> > src/cam/jpeg_error_manager.cpp | 26 ++++++++++++++++++++
> > src/cam/jpeg_error_manager.h | 21 ++++++++++++++++
> > src/cam/meson.build | 9 +++----
> > src/cam/sdl_sink.cpp | 4 ++--
> > src/cam/sdl_texture_mjpg.cpp | 44 +++++++++++++++++++++++++++++-----
> > src/cam/sdl_texture_mjpg.h | 6 +++++
> > 7 files changed, 99 insertions(+), 13 deletions(-)
> > create mode 100644 src/cam/jpeg_error_manager.cpp
> > create mode 100644 src/cam/jpeg_error_manager.h
> >
> > diff --git a/README.rst b/README.rst
> > index b9e72d81..47b914f0 100644
> > --- a/README.rst
> > +++ b/README.rst
> > @@ -92,8 +92,8 @@ for cam: [optional]
> > tool:
> >
> > - libdrm-dev: Enables the KMS sink
> > + - libjpeg-dev: Enables MJPEG on the SDL sink
> > - libsdl2-dev: Enables the SDL sink
> > - - libsdl2-image-dev: Supports MJPEG on the SDL sink
> >
> > for qcam: [optional]
> > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev
> > diff --git a/src/cam/jpeg_error_manager.cpp b/src/cam/jpeg_error_manager.cpp
> > new file mode 100644
> > index 00000000..7e45e577
> > --- /dev/null
> > +++ b/src/cam/jpeg_error_manager.cpp
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * jpeg_error_manager.cpp - JPEG Error Manager
> > + */
> > +
> > +#include "jpeg_error_manager.h"
> > +
> > +static void errorExit(j_common_ptr cinfo)
> > +{
> > + JpegErrorManager *self =
> > + reinterpret_cast<JpegErrorManager *>(cinfo->err);
> > + longjmp(self->escape_, 1);
> > +}
> > +
> > +static void outputMessage([[maybe_unused]] j_common_ptr cinfo)
> > +{
> > +}
> > +
> > +JpegErrorManager::JpegErrorManager(struct jpeg_decompress_struct &cinfo)
>
> I wouldn't mix the error manager and decompressor. You can keep the
> jpeg_std_error() call below, and move the assignment of cinfo.err to the
> code that creates the decompressor.
>
> > +{
> > + cinfo.err = jpeg_std_error(&errmgr_);
> > + errmgr_.error_exit = errorExit;
> > + errmgr_.output_message = outputMessage;
> > +}
>
> This can live in sdl_texture_mjpg.cpp as it's internal to the
> implementation of the SDL texture for MJPEG, there's no need to have one
> file per class.
>
> > diff --git a/src/cam/jpeg_error_manager.h b/src/cam/jpeg_error_manager.h
> > new file mode 100644
> > index 00000000..0af28da1
> > --- /dev/null
> > +++ b/src/cam/jpeg_error_manager.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy
> > + *
> > + * jpeg_error_manager.h - JPEG Error Manager
> > + */
> > +
> > +#pragma once
> > +
> > +#include <setjmp.h>
> > +#include <stdio.h>
> > +
> > +#include <jpeglib.h>
> > +
> > +struct JpegErrorManager {
> > + JpegErrorManager(struct jpeg_decompress_struct &cinfo);
> > +
> > + /* Order very important for reinterpret_cast */
> > + struct jpeg_error_mgr errmgr_;
> > + jmp_buf escape_;
> > +};
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 5957ce14..86cef683 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -24,8 +24,8 @@ cam_sources = files([
> > cam_cpp_args = []
> >
> > libdrm = dependency('libdrm', required : false)
> > +libjpeg = dependency('libjpeg', required : false)
> > libsdl2 = dependency('SDL2', required : false)
> > -libsdl2_image = dependency('SDL2_image', required : false)
> >
> > if libdrm.found()
> > cam_cpp_args += [ '-DHAVE_KMS' ]
> > @@ -43,9 +43,10 @@ if libsdl2.found()
> > 'sdl_texture_yuyv.cpp'
> > ])
> >
> > - if libsdl2_image.found()
> > - cam_cpp_args += ['-DHAVE_SDL_IMAGE']
> > + if libjpeg.found()
> > + cam_cpp_args += ['-DHAVE_LIBJPEG']
> > cam_sources += files([
> > + 'jpeg_error_manager.cpp',
> > 'sdl_texture_mjpg.cpp'
> > ])
> > endif
> > @@ -57,8 +58,8 @@ cam = executable('cam', cam_sources,
> > libcamera_public,
> > libdrm,
> > libevent,
> > + libjpeg,
> > libsdl2,
> > - libsdl2_image,
> > libyaml,
> > ],
> > cpp_args : cam_cpp_args,
> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > index f8e3e95d..19fdfd6d 100644
> > --- a/src/cam/sdl_sink.cpp
> > +++ b/src/cam/sdl_sink.cpp
> > @@ -21,7 +21,7 @@
> >
> > #include "event_loop.h"
> > #include "image.h"
> > -#ifdef HAVE_SDL_IMAGE
> > +#ifdef HAVE_LIBJPEG
> > #include "sdl_texture_mjpg.h"
> > #endif
> > #include "sdl_texture_yuyv.h"
> > @@ -62,7 +62,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
> > rect_.h = cfg.size.height;
> >
> > switch (cfg.pixelFormat) {
> > -#ifdef HAVE_SDL_IMAGE
> > +#ifdef HAVE_LIBJPEG
> > case libcamera::formats::MJPEG:
> > texture_ = std::make_unique<SDLTextureMJPG>(rect_);
> > break;
> > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp
> > index 69e99ad3..22f570c6 100644
> > --- a/src/cam/sdl_texture_mjpg.cpp
> > +++ b/src/cam/sdl_texture_mjpg.cpp
> > @@ -7,19 +7,51 @@
> >
> > #include "sdl_texture_mjpg.h"
> >
> > -#include <SDL2/SDL_image.h>
> > +#include "jpeg_error_manager.h"
> > +
> > +#include <iostream>
> >
> > using namespace libcamera;
> >
> > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)
> > - : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)
> > + : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3),
> > + rgb_(std::make_unique<unsigned char[]>(pitch_ * rect.h))
> > +{
> > +}
> > +
> > +int SDLTextureMJPG::decompress(const Span<uint8_t> &data)
>
> int SDLTextureMJPG::decompress(Span<const uint8_t> data)
>
> for two reasons:
>
> - a Span is a pointer + size, which will fit in registers when calling
> this function, so that should be more efficient than passing a pointer
> to the Span
I'm not sure about that:
#include <libcamera/libcamera/base/span.h>
#include <iostream>
int main() {
std::cout << sizeof(const libcamera::Span<const uint8_t>) << std::endl;
std::cout << sizeof(const libcamera::Span<const uint8_t>*) << std::endl;
std::cout << sizeof(const libcamera::Span<const uint8_t>&) << std::endl;
}
Output:
16
8
16
Even though pass by value and pass by reference are the same amount of
bytes (both 16 bytes and the pointer is 8 on x86), I would expect pass
by reference in the worst case to be of equal performance because it
ensures no constructor of any kind is called (where another copy or
two could occur in pass by value, I don't even need to read the
constructor's source code if it's a reference, that's why class/struct
as reference parameter is a great rule of thumb). But I'm not sure
something like this is worth a v6 either, so it's fine the way it is I
guess.
>
> - Span<const uint8_t> to indicate that the function doesn't modify the
> data. A const Span<uint8_t> prevents changing the Span itself, but
> allows modifying the data that the Span points to.
>
> This would however require changing the update() function accordingly,
> so I'll submit a patch on top.
>
> I've made those modifications locally to test and make sure I wasn't
> saying something stupid, so I'll send a v5 shortly as it's ready.
Gonna run a test.
>
> > {
> > + struct jpeg_decompress_struct cinfo;
> > + JpegErrorManager jpegErrorManager(cinfo);
> > + if (setjmp(jpegErrorManager.escape_)) {
> > + /* libjpeg found an error */
> > + jpeg_destroy_decompress(&cinfo);
> > + std::cerr << "JPEG decompression error" << std::endl;
> > + return -EINVAL;
> > + }
> > +
> > + jpeg_create_decompress(&cinfo);
> > +
> > + jpeg_mem_src(&cinfo, data.data(), data.size());
> > +
> > + jpeg_read_header(&cinfo, TRUE);
> > +
> > + jpeg_start_decompress(&cinfo);
> > +
> > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {
> > + JSAMPROW rowptr = rgb_.get() + i * pitch_;
> > + jpeg_read_scanlines(&cinfo, &rowptr, 1);
> > + }
> > +
> > + jpeg_finish_decompress(&cinfo);
> > +
> > + jpeg_destroy_decompress(&cinfo);
> > +
> > + return 0;
> > }
> >
> > void SDLTextureMJPG::update(const Span<uint8_t> &data)
> > {
> > - SDL_RWops *bufferStream = SDL_RWFromMem(data.data(), data.size());
> > - SDL_Surface *frame = IMG_Load_RW(bufferStream, 0);
> > - SDL_UpdateTexture(ptr_, nullptr, frame->pixels, frame->pitch);
> > - SDL_FreeSurface(frame);
> > + decompress(data);
> > + SDL_UpdateTexture(ptr_, nullptr, rgb_.get(), pitch_);
> > }
> > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h
> > index b103f801..328c45a9 100644
> > --- a/src/cam/sdl_texture_mjpg.h
> > +++ b/src/cam/sdl_texture_mjpg.h
> > @@ -13,5 +13,11 @@ class SDLTextureMJPG : public SDLTexture
> > {
> > public:
> > SDLTextureMJPG(const SDL_Rect &rect);
> > +
> > void update(const libcamera::Span<uint8_t> &data) override;
> > +
> > +private:
> > + int decompress(const libcamera::Span<uint8_t> &data);
> > +
> > + std::unique_ptr<unsigned char[]> rgb_;
> > };
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list