| Message ID | 20220719091745.8040-1-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 - 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. > { > + 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_; > };
On Wed, 20 Jul 2022 at 12:56, Laurent Pinchart <laurent.pinchart@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@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 >
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@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. -------- #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_; > > > };
On Thu, 21 Jul 2022 at 21:49, Laurent Pinchart <laurent.pinchart@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@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 >
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) +{ + cinfo.err = jpeg_std_error(&errmgr_); + errmgr_.error_exit = errorExit; + errmgr_.output_message = outputMessage; +} 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) { + 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_; };
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@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