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

16637 diff mbox series

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 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/meson.build          |  8 ++---
 src/cam/sdl_sink.cpp         |  4 +--
 src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----
 src/cam/sdl_texture_mjpg.h   |  6 ++++
 5 files changed, 70 insertions(+), 13 deletions(-)

Hi Eric,

Thank you for the patch.

On Fri, Jul 15, 2022 at 12:36:05PM +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 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/meson.build          |  8 ++---
>  src/cam/sdl_sink.cpp         |  4 +--
>  src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----
>  src/cam/sdl_texture_mjpg.h   |  6 ++++
>  5 files changed, 70 insertions(+), 13 deletions(-)
> 
> 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/meson.build b/src/cam/meson.build
> index 5957ce14..4dfa7b22 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,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'
>          ])
> @@ -57,8 +57,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..a064e6e5 100644
> --- a/src/cam/sdl_texture_mjpg.cpp
> +++ b/src/cam/sdl_texture_mjpg.cpp
> @@ -7,19 +7,70 @@
>  
>  #include "sdl_texture_mjpg.h"
>  
> -#include <SDL2/SDL_image.h>
> +#include <iostream>
> +#include <setjmp.h>
> +
> +#include <jpeglib.h>
>  
>  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))
> +{
> +}
> +
> +struct error_mgr {
> +	struct jpeg_error_mgr errmgr;
> +	jmp_buf escape;
> +};
> +
> +static void error_exit(j_common_ptr cinfo)

This function should be marked with [[noreturn]].

> +{
> +	struct error_mgr *err = (struct error_mgr *)cinfo->err;

	struct error_mgr *err = static_cast<struct error_mgr *>(cinfo->err);

> +	longjmp(err->escape, 1);
> +}
> +
> +static void output_no_message(j_common_ptr cinfo)

You can write

static void output_no_message([[maybed_unused]] j_common_ptr cinfo)

> +{
> +	(void)cinfo;

and drop this.

> +}

How about grouping the structure and functions together, as we use C++ ?

struct JpegErrorManager {
	JpegErrorManager()
	{
		jpeg_std_error(&errmgr);
		errmgr.error_exit = errorExit;
		errmgr.output_message = outputMessage;
	}

	[[noreturn]] static void errorExit(j_common_ptr cinfo)
	{
		JpegErrorManager *self = static_cast<JpegErrorManager *>(cinfo->err);
		longjmp(self->escape, 1);
	}

	static void outputMessage([[maybed_unused]] j_common_ptr cinfo)
	{
	}

	struct jpeg_error_mgr errmgr;
	jmp_buf escape;
}

I think that's longjmp-safe as there's nothing to do at destruction
time.

> +
> +void SDLTextureMJPG::decompress(const unsigned char *jpeg,
> +				unsigned long jpeg_size)

The function should take a Span<const uint8_t> instead of separate data
pointer and size.

>  {
> +	struct error_mgr jerr;
> +	struct jpeg_decompress_struct cinfo;
> +	cinfo.err = jpeg_std_error(&jerr.errmgr);
> +	jerr.errmgr.error_exit = error_exit;
> +	jerr.errmgr.output_message = output_no_message;

This would become

	struct jpeg_decompress_struct cinfo;
	JpegErrorManager jerr;

	cinfo.err = &jerr.errmgr;

And I'd add a blank line here for clarity.

If you'd rather avoid the JpegErrorManager, I would write

	struct jpeg_decompress_struct cinfo;

	struct error_mgr jerr;
	jpeg_std_error(&jerr.errmgr);
	jerr.errmgr.error_exit = error_exit;
	jerr.errmgr.output_message = output_no_message;

	if (setjmp(jerr.escape)) {
		...
	}

	cinfo.err = &jerr.errmgr;

> +	if (setjmp(jerr.escape)) {
> +		/* libjpeg found an error */
> +		jpeg_destroy_decompress(&cinfo);
> +		std::cerr << "JPEG decompression error" << std::endl;
> +		return;

Let's return an error from this function.

> +	}
> +
> +	jpeg_create_decompress(&cinfo);

The decompressor and the error manager don't need to be recreated for
each image. Optimizing this would require substantial changes though, as
we would need to create the JPEG encoder separately from the texture, so
this can be done later.

> +
> +	jpeg_mem_src(&cinfo, jpeg, jpeg_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);
>  }
>  
>  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());

What should we do here if an error occurred during decompression ? We
could proceed nonetheless (which will result in some garbage being
displayed), or return an error from this function and skip the
SDL_Render* calls in the caller. What do you think would be best ?

> +	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..c68b03b0 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:
> +	void decompress(const unsigned char *jpeg, unsigned long jpeg_size);
> +
> +	std::unique_ptr<unsigned char[]> rgb_;
>  };

On Sun, 17 Jul 2022 at 21:42, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Fri, Jul 15, 2022 at 12:36:05PM +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 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/meson.build          |  8 ++---
> >  src/cam/sdl_sink.cpp         |  4 +--
> >  src/cam/sdl_texture_mjpg.cpp | 63 ++++++++++++++++++++++++++++++++----
> >  src/cam/sdl_texture_mjpg.h   |  6 ++++
> >  5 files changed, 70 insertions(+), 13 deletions(-)
> >
> > 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/meson.build b/src/cam/meson.build
> > index 5957ce14..4dfa7b22 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,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'
> >          ])
> > @@ -57,8 +57,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..a064e6e5 100644
> > --- a/src/cam/sdl_texture_mjpg.cpp
> > +++ b/src/cam/sdl_texture_mjpg.cpp
> > @@ -7,19 +7,70 @@
> >
> >  #include "sdl_texture_mjpg.h"
> >
> > -#include <SDL2/SDL_image.h>
> > +#include <iostream>
> > +#include <setjmp.h>
> > +
> > +#include <jpeglib.h>
> >
> >  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))
> > +{
> > +}
> > +
> > +struct error_mgr {
> > +     struct jpeg_error_mgr errmgr;
> > +     jmp_buf escape;
> > +};
> > +
> > +static void error_exit(j_common_ptr cinfo)
>
> This function should be marked with [[noreturn]].
>
> > +{
> > +     struct error_mgr *err = (struct error_mgr *)cinfo->err;
>
>         struct error_mgr *err = static_cast<struct error_mgr *>(cinfo->err);

This has to be reinterpret_cast, that seem dangerous but it's really
not, it's basically used to add jmp_buf to jpeg_error_mgr struct. It's
kinda being used to fake inheritance in C, without the api supporting
it.

>
> > +     longjmp(err->escape, 1);
> > +}
> > +
> > +static void output_no_message(j_common_ptr cinfo)
>
> You can write
>
> static void output_no_message([[maybed_unused]] j_common_ptr cinfo)
>
> > +{
> > +     (void)cinfo;
>
> and drop this.
>
> > +}
>
> How about grouping the structure and functions together, as we use C++ ?

I guess we can.

>
> struct JpegErrorManager {
>         JpegErrorManager()
>         {
>                 jpeg_std_error(&errmgr);
>                 errmgr.error_exit = errorExit;
>                 errmgr.output_message = outputMessage;
>         }
>
>         [[noreturn]] static void errorExit(j_common_ptr cinfo)
>         {
>                 JpegErrorManager *self = static_cast<JpegErrorManager *>(cinfo->err);
>                 longjmp(self->escape, 1);
>         }
>
>         static void outputMessage([[maybed_unused]] j_common_ptr cinfo)
>         {
>         }
>
>         struct jpeg_error_mgr errmgr;
>         jmp_buf escape;
> }
>
> I think that's longjmp-safe as there's nothing to do at destruction
> time.
>
> > +
> > +void SDLTextureMJPG::decompress(const unsigned char *jpeg,
> > +                             unsigned long jpeg_size)
>
> The function should take a Span<const uint8_t> instead of separate data
> pointer and size.
>
> >  {
> > +     struct error_mgr jerr;
> > +     struct jpeg_decompress_struct cinfo;
> > +     cinfo.err = jpeg_std_error(&jerr.errmgr);
> > +     jerr.errmgr.error_exit = error_exit;
> > +     jerr.errmgr.output_message = output_no_message;
>
> This would become
>
>         struct jpeg_decompress_struct cinfo;
>         JpegErrorManager jerr;
>
>         cinfo.err = &jerr.errmgr;
>
> And I'd add a blank line here for clarity.
>
> If you'd rather avoid the JpegErrorManager, I would write
>
>         struct jpeg_decompress_struct cinfo;
>
>         struct error_mgr jerr;
>         jpeg_std_error(&jerr.errmgr);
>         jerr.errmgr.error_exit = error_exit;
>         jerr.errmgr.output_message = output_no_message;
>
>         if (setjmp(jerr.escape)) {
>                 ...
>         }
>
>         cinfo.err = &jerr.errmgr;
>
> > +     if (setjmp(jerr.escape)) {
> > +             /* libjpeg found an error */
> > +             jpeg_destroy_decompress(&cinfo);
> > +             std::cerr << "JPEG decompression error" << std::endl;
> > +             return;
>
> Let's return an error from this function.
>
> > +     }
> > +
> > +     jpeg_create_decompress(&cinfo);
>
> The decompressor and the error manager don't need to be recreated for
> each image. Optimizing this would require substantial changes though, as
> we would need to create the JPEG encoder separately from the texture, so
> this can be done later.
>
> > +
> > +     jpeg_mem_src(&cinfo, jpeg, jpeg_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);
> >  }
> >
> >  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());
>
> What should we do here if an error occurred during decompression ? We
> could proceed nonetheless (which will result in some garbage being
> displayed), or return an error from this function and skip the
> SDL_Render* calls in the caller. What do you think would be best ?

I'm unsure, if say the last two scanlines were garbage, maybe you
should still display the frame (but maybe this sort of case is
unlikely when the data is corrupt), for simplicity was gonna display
anyway, No harm in returning non-zero I guess in this case though, if
someone decides that's useful in future.


>
> > +     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..c68b03b0 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:
> > +     void decompress(const unsigned char *jpeg, unsigned long jpeg_size);
> > +
> > +     std::unique_ptr<unsigned char[]> rgb_;
> >  };
>
> --
> Regards,
>
> Laurent Pinchart
>