[libcamera-devel] JPEG support for libcamera

[Jacopo Mondi]

Hi Pavel,

On Mon, Jul 04, 2022 at 08:07:20PM +0200, Pavel Machek via libcamera-devel wrote:
> JPEG is very similar to MJPEG format (AFAICT, it may contain extra
> EXIF headers). Add support for it. Tested with PinePhone and command
> line cam utility.
>
> Signed-off-by: Pavel Machek <pavel at ucw.cz>
>
> diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> index ea11dcb4..b30a705d 100644
> --- a/include/linux/drm_fourcc.h
> +++ b/include/linux/drm_fourcc.h
> @@ -352,6 +352,7 @@ extern "C" {
>
>  /* Compressed formats */
>  #define DRM_FORMAT_MJPEG	fourcc_code('M', 'J', 'P', 'G') /* Motion-JPEG */
> +#define DRM_FORMAT_JPEG	fourcc_code('J', 'P', 'E', 'G') /* JFIF JPEG */

If something like this is required it should be done in upstream DRM.

However I'm not sure that's what we want, see below..

>
>  /*
>   * Bayer formats
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index f8e3e95d..673bd642 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -63,6 +63,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
>
>  	switch (cfg.pixelFormat) {
>  #ifdef HAVE_SDL_IMAGE
> +	case libcamera::formats::JPEG:
>  	case libcamera::formats::MJPEG:
>  		texture_ = std::make_unique<SDLTextureMJPG>(rect_);
>  		break;
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 3f242286..fd5689d9 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -80,6 +80,7 @@ bare_structure_from_format(const PixelFormat &format)
>  					 gst_video_format_to_string(gst_format), nullptr);
>
>  	switch (format) {
> +	case formats::JPEG:
>  	case formats::MJPEG:
>  		return gst_structure_new_empty("image/jpeg");
>  	default:
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 283ecb3d..c5e97198 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -944,6 +944,19 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  	} },
>
>  	/* Compressed formats. */
> +	{ formats::JPEG, {
> +		.name = "JPEG",
> +		.format = formats::JPEG,
> +		.v4l2Formats = {
> +			.single = V4L2PixelFormat(V4L2_PIX_FMT_JPEG),
> +			.multi = V4L2PixelFormat(),
> +		},
> +		.bitsPerPixel = 0,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.packed = false,
> +		.pixelsPerGroup = 1,
> +		.planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },

I would rather work with the idea to associate multiple v4l2Formats to
the single formats::MJPEG identifier, by making
PixelFormatInfo::v4l2Formats an array.

However I'm not sure how we could rework this:

V4L2PixelFormat V4L2PixelFormat::fromPixelFormat(const PixelFormat &pixelFormat,
						 bool multiplanar)
{
	const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
	if (!info.isValid())
		return V4L2PixelFormat();

	return multiplanar ? info.v4l2Formats.multi : info.v4l2Formats.single;
}

So that it picks the right V4L2 pixelformat if not by poking the video
device and checking what it supports.

I think we already have a very similar problem in the
mapping of contiguous/non-contiguous YUV planar formats, where we
abuse the V4L2 planar/multiplanar API definition to discern which
format to map to

	{ formats::NV16, {
		.name = "NV16",
		.format = formats::NV16,
		.v4l2Formats = {
			.single = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
			.multi = V4L2PixelFormat(V4L2_PIX_FMT_NV16M),
		},

Which is actually problematic as there are drivers that (rightfully)
expose V4L2_PIX_FMT_NV16 through the multiplanar API.

I would go one step further and remove the single/multiplanar formats
and just place a list there to match against the list of formats
supported by the video device which will have to be passed to
V4L2PixelFormat::fromPixelFormat().

Opinions ?

Pavel: I'm not asking you to go full circle and change such internal
machinery to have your format supported, I can look into doing that,
but in the meantime you should live with your patch out-of-tree. Is
this ok ?


>  	{ formats::MJPEG, {
>  		.name = "MJPEG",
>  		.format = formats::MJPEG,
> diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> index 7dda0132..6b931c34 100644
> --- a/src/libcamera/formats.yaml
> +++ b/src/libcamera/formats.yaml
> @@ -76,6 +76,8 @@ formats:
>    - YVU444:
>        fourcc: DRM_FORMAT_YVU444
>
> +  - JPEG:
> +      fourcc: DRM_FORMAT_JPEG
>    - MJPEG:
>        fourcc: DRM_FORMAT_MJPEG
>
> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> index 58fc4e9d..82dfbd1e 100644
> --- a/src/libcamera/v4l2_pixelformat.cpp
> +++ b/src/libcamera/v4l2_pixelformat.cpp
> @@ -183,6 +183,8 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
>  	/* Compressed formats. */
>  	{ V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
>  		{ formats::MJPEG, "Motion-JPEG" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_JPEG),
> +		{ formats::JPEG, "JPEG" } },
>  };
>
>  } /* namespace */
>
> --
> People of Russia, stop Putin before his war on Ukraine escalates.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220707/f2c7864a/attachment.sig>