[libcamera-devel] [PATCH] cam: sdl_sink: Use libjpeg over SDL2_image

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

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 (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