[libcamera-devel] [PATCH] cam: sdl_sink: Use libjpeg over SDL2_image
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 14 13:24:09 CEST 2022
Hi Eric,
I was wondering if you planned to submit a new version of this patch.
There's no big urgency, but if you expect to be too busy for the
foreseable future, I can send a v2 that addresses my review comments.
On Fri, Jul 08, 2022 at 04:37:50PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Thu, Jul 07, 2022 at 11:25:58PM +0100, Eric Curtin wrote:
> > On Thu, 7 Jul 2022 at 21:13, Laurent Pinchart wrote:
> > > On Thu, Jul 07, 2022 at 04:53:13PM +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>
> > > > ---
> > > > README.rst | 2 +-
> > > > src/cam/meson.build | 8 +++----
> > > > src/cam/sdl_sink.cpp | 4 ++--
> > > > src/cam/sdl_texture.h | 2 +-
> > > > src/cam/sdl_texture_mjpg.cpp | 43 +++++++++++++++++++++++++++++++-----
> > > > src/cam/sdl_texture_mjpg.h | 5 +++++
> > > > 6 files changed, 51 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/README.rst b/README.rst
> > > > index b9e72d81..5e8bc1cc 100644
> > > > --- a/README.rst
> > > > +++ b/README.rst
> > > > @@ -93,7 +93,7 @@ for cam: [optional]
> > > >
> > > > - libdrm-dev: Enables the KMS sink
> > > > - libsdl2-dev: Enables the SDL sink
> > > > - - libsdl2-image-dev: Supports MJPEG on the SDL sink
> > > > + - libjpeg-dev: Supports MJPEG on the SDL sink
> > >
> > > Could you please keep this list sorted ?
> > >
> > > >
> > > > for qcam: [optional]
> > > > qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev
> > > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > > index 5957ce14..27cbd0de 100644
> > > > --- a/src/cam/meson.build
> > > > +++ b/src/cam/meson.build
> > > > @@ -25,7 +25,7 @@ cam_cpp_args = []
> > > >
> > > > libdrm = dependency('libdrm', required : false)
> > > > libsdl2 = dependency('SDL2', required : false)
> > > > -libsdl2_image = dependency('SDL2_image', required : false)
> > > > +libjpeg = dependency('libjpeg', required : false)
> > >
> > > Same here.
> > >
> > > >
> > > > if libdrm.found()
> > > > cam_cpp_args += [ '-DHAVE_KMS' ]
> > > > @@ -43,8 +43,8 @@ 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([
> > > > 'sdl_texture_mjpg.cpp'
> > > > ])
> > > > @@ -58,7 +58,7 @@ cam = executable('cam', cam_sources,
> > > > libdrm,
> > > > libevent,
> > > > libsdl2,
> > > > - libsdl2_image,
> > > > + libjpeg,
> > >
> > > And here.
> > >
> > > > 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.h b/src/cam/sdl_texture.h
> > > > index 90974798..42c906f2 100644
> > > > --- a/src/cam/sdl_texture.h
> > > > +++ b/src/cam/sdl_texture.h
> > > > @@ -25,5 +25,5 @@ protected:
> > > > SDL_Texture *ptr_;
> > > > const SDL_Rect rect_;
> > > > const SDL_PixelFormatEnum pixelFormat_;
> > > > - const int pitch_;
> > > > + int pitch_;
> > > > };
> > > > diff --git a/src/cam/sdl_texture_mjpg.cpp b/src/cam/sdl_texture_mjpg.cpp
> > > > index 69e99ad3..232af673 100644
> > > > --- a/src/cam/sdl_texture_mjpg.cpp
> > > > +++ b/src/cam/sdl_texture_mjpg.cpp
> > > > @@ -7,19 +7,52 @@
> > > >
> > > > #include "sdl_texture_mjpg.h"
> > > >
> > > > -#include <SDL2/SDL_image.h>
> > > > +#include <jpeglib.h>
> > > >
> > > > using namespace libcamera;
> > > >
> > > > SDLTextureMJPG::SDLTextureMJPG(const SDL_Rect &rect)
> > > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, 0)
> > >
> > > You can write
> > >
> > > : SDLTexture(rect, SDL_PIXELFORMAT_RGB24, rect.w * 3)
> > >
> > > > {
> > > > + pitch_ = rect_.w * 3;
> > >
> > > and drop this line, and keep pitch_ const.
> > >
> > > > + rgbdata_ = (unsigned char *)malloc(pitch_ * rect_.h);
> > >
> > > Please use C++-style memory allocation
> > >
> > > rgbdata_ = new unsigned char[pitch_ * rect_.h];
> > >
> > > The destructor should then use
> > >
> > > delete[] rgbdata_;
> > >
> > > Even better, I think you can turn rgbdata_ into a
> > >
> > > std::unique_ptr<unsigned char[]> rgbdata_;
> > >
> > > and drop the delete.
> > >
> > > Another option would have been to turn rgbdata_ into an
> > > std::vector<unsigned char> to also remove the need for a custom
> > > destructor. The drawback is that, unlike new that performs
> > > default-initialization, std::vector::resize() performs
> > > default-insertion, which uses value-initialization, effectively
> > > memsetting the vector to 0. There's a way around that by providing a
> > > custom allocator to the vector, but I think we're getting into the "not
> > > worth it" territory (at least as long as libcamera-base doesn't provide
> > > a helper for this). Let's thus use a unique_ptr.
> > >
> > > > +}
> > > > +
> > > > +SDLTextureMJPG::~SDLTextureMJPG()
> > > > +{
> > > > + free(rgbdata_);
> > > > +}
> > > > +
> > > > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,
> > > > + unsigned long jpeg_size)
> > > > +{
> > > > + struct jpeg_error_mgr jerr;
> > > > + struct jpeg_decompress_struct cinfo;
> > > > + cinfo.err = jpeg_std_error(&jerr);
> > >
> > > There's a risk the JPEG data could be corrupted (that's quite common
> > > with UVC cameras). Error handling should be a bit more robust, you
> > > should create your own error handler, store that an error occurred, and
> > > return an error from this function and skip the SDL_UpdateTexture() in
> > > that case.
> > >
> > > Furthermore, jpg_std_error() will by default call exit() when a fatal
> > > error occurs. That's not good. The recommended way to deal with this is
> > > to use setjmp() and longjmp() to exit cleanly instead.
> > >
> > > > +
> > > > + jpeg_create_decompress(&cinfo);
> > > > +
> > > > + jpeg_mem_src(&cinfo, jpeg, jpeg_size);
> > > > +
> > > > + jpeg_read_header(&cinfo, TRUE);
> > > > +
> > > > + jpeg_start_decompress(&cinfo);
> > > > +
> > > > + int row_stride = cinfo.output_width * cinfo.output_components;
> > >
> > > rowStride (libcamera coding style)
> > >
> > > > + unsigned char *buffer = (unsigned char *)malloc(row_stride);
> > >
> > > Here too, std::unique_ptr and new[], or just
> > >
> > > unsigned char buffer[row_stride);
> >
> > For buffer, I need to pass the address of the variable storing the
> > pointer to jpeg_read_scanlines, not sure unique_ptr is possible in
> > this very specific case? (although I can easily change rgbdata_ to
> > unique_ptr, thanks for spotting that).
> >
> > "unsigned char buffer[row_stride];" won't work for the same reason,
> > needs to be a raw C pointer to pass &buffer to jpeg_read_scanlines.
>
> Good point. You could have a separate variable:
>
> unsigned char row[rowStride];
> unsigned char *rows[1] = { &row };
>
> Not sure if it's worth it. This makes me wonder, is there a performance
> benefit from reading multiple scanlines in one go ?
>
> > All the other feedback makes sense and will be addressed.
> >
> > > > + for (int i = 0; cinfo.output_scanline < cinfo.output_height; ++i) {
> > > > + jpeg_read_scanlines(&cinfo, &buffer, 1);
> > > > + memcpy(rgbdata_ + i * pitch_, buffer, row_stride);
> > > > + }
> > > > +
> > > > + free(buffer);
> > > > + jpeg_finish_decompress(&cinfo);
> > > > +
> > > > + jpeg_destroy_decompress(&cinfo);
> > > > }
> > > >
> > > > 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.data(), data.size());
> > > > + SDL_UpdateTexture(ptr_, nullptr, rgbdata_, pitch_);
> > > > }
> > > > diff --git a/src/cam/sdl_texture_mjpg.h b/src/cam/sdl_texture_mjpg.h
> > > > index b103f801..73c7fb68 100644
> > > > --- a/src/cam/sdl_texture_mjpg.h
> > > > +++ b/src/cam/sdl_texture_mjpg.h
> > > > @@ -13,5 +13,10 @@ class SDLTextureMJPG : public SDLTexture
> > > > {
> > > > public:
> > > > SDLTextureMJPG(const SDL_Rect &rect);
> > > > + virtual ~SDLTextureMJPG();
> > >
> > > No need for virtual here, and while at it, you can insert a blank line.
> > >
> > > > void update(const libcamera::Span<uint8_t> &data) override;
> > > > +
> > > > +private:
> > > > + unsigned char *rgbdata_ = 0;
> > >
> > > Member data after member functions. I'd name the member rgbData_ or just
> > > rgb_, up to you.
> > >
> > > > + void decompress(const unsigned char *jpeg, unsigned long jpeg_size);
> > >
> > > void decompress(const unsigned char *jpeg, std::size size);
> > >
> > > > };
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list