[libcamera-devel] [PATCH v4] cam: sdl_sink: Use libjpeg over SDL2_image
Eric Curtin
ecurtin at redhat.com
Fri Jul 22 11:32:46 CEST 2022
On Thu, 21 Jul 2022 at 21:49, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Wed, Jul 20, 2022 at 02:54:29PM +0100, Eric Curtin wrote:
> > On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart wrote:
> > > 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;
>
> This one is a bit misleading, it will indeed print 16, but passing by
> reference will pass the address of the variable, so 8 bytes only.
I was confused by that I'll admit, I realized afterwards by
investigating more. I'm good with v5.
>
> --------
> #include <libcamera/base/span.h>
>
> namespace libcamera {
>
> void by_pointer(Span<uint8_t> *data);
> void by_reference(Span<uint8_t> &data);
> void by_value(Span<uint8_t> data);
>
> void test_function()
> {
> Span data{ reinterpret_cast<uint8_t *>(0x01234567), 0x89abcdef };
>
> by_pointer(&data);
> by_reference(data);
> by_value(data);
> }
>
> }
> --------
>
> Compiled with -O0 and disassembled with
>
> $ objdump -d -r --demangle src/libcamera/libcamera.so.0.0.0.p/reference.cpp.o
>
> produces
>
> --------
> 0000000000000000 <libcamera::test_function()>:
> 0: 55 push %rbp
> 1: 48 89 e5 mov %rsp,%rbp
> 4: 48 83 ec 20 sub $0x20,%rsp
> 8: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
> f: 00 00
> 11: 48 89 45 f8 mov %rax,-0x8(%rbp)
> 15: 31 c0 xor %eax,%eax
> 17: 48 8d 45 e0 lea -0x20(%rbp),%rax
> 1b: ba ef cd ab 89 mov $0x89abcdef,%edx
> 20: be 67 45 23 01 mov $0x1234567,%esi
> 25: 48 89 c7 mov %rax,%rdi
> 28: e8 00 00 00 00 call 2d <libcamera::test_function()+0x2d>
> 29: R_X86_64_PLT32 libcamera::Span<unsigned char, 18446744073709551615ul>::Span(unsigned char*, unsigned long)-0x4
> 2d: 48 8d 45 e0 lea -0x20(%rbp),%rax
> 31: 48 89 c7 mov %rax,%rdi
> 34: e8 00 00 00 00 call 39 <libcamera::test_function()+0x39>
> 35: R_X86_64_PLT32 libcamera::by_pointer(libcamera::Span<unsigned char, 18446744073709551615ul>*)-0x4
> 39: 48 8d 45 e0 lea -0x20(%rbp),%rax
> 3d: 48 89 c7 mov %rax,%rdi
> 40: e8 00 00 00 00 call 45 <libcamera::test_function()+0x45>
> 41: R_X86_64_PLT32 libcamera::by_reference(libcamera::Span<unsigned char, 18446744073709551615ul>&)-0x4
> 45: 48 8b 55 e0 mov -0x20(%rbp),%rdx
> 49: 48 8b 45 e8 mov -0x18(%rbp),%rax
> 4d: 48 89 d7 mov %rdx,%rdi
> 50: 48 89 c6 mov %rax,%rsi
> 53: e8 00 00 00 00 call 58 <libcamera::test_function()+0x58>
> 54: R_X86_64_PLT32 libcamera::by_value(libcamera::Span<unsigned char, 18446744073709551615ul>)-0x4
> 58: 90 nop
> 59: 48 8b 45 f8 mov -0x8(%rbp),%rax
> 5d: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax
> 64: 00 00
> 66: 74 05 je 6d <libcamera::test_function()+0x6d>
> 68: e8 00 00 00 00 call 6d <libcamera::test_function()+0x6d>
> 69: R_X86_64_PLT32 __stack_chk_fail-0x4
> 6d: c9 leave
> 6e: c3 ret
> --------
>
> As you can see, both by_pointer() and by_reference() get passed the
> address of the data variable in rdi, while by_value() get the contents
> of the Span in rdi and rsi.
>
> Obviously this only works if the number of parameters to the function is
> low enough to fit everything in registers, and it only helps if the
> caller has the Span data and size in registers already, as if it has to
> load them from memory, the callee could do so as well.
>
> I any case, I agree it's not a big optimization (if at all). The second
> point I mentioned (Span<const uint8_t>) is more important.
>
> > }
> >
> > 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