[libcamera-devel] JPEG support for libcamera
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 7 10:10:10 CEST 2022
Hi Jacopo,
On Thu, Jul 07, 2022 at 09:33:42AM +0200, Jacopo Mondi via libcamera-devel wrote:
> 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 don't think that's really a problem, the multiplanar argument to
fromPixelFormat() refers to multiplanar formats, not the multiplanar
API. What is, however, a problem is that no caller sets that argument to
true :-)
> 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().
I don't like the idea of passing the list to the function if all callers
have to do so manually. Not only will it complicate the callers, but
some won't even have a list to pass (see
V4L2CameraProxy::setFmtFromConfig() for instance).
Looking at commit 395d43d6d75b ("libcamera: v4l2_videodevice: Drop
toV4L2PixelFormat()"), I think we should resurect that function, and use
it to replace most V4L2PixelFormat::fromPixelFormat() calls. It can
internally call a V4L2PixelFormat::fromPixelFormat() overload with a
list of formats, while a different V4L2PixelFormat::fromPixelFormat()
would exist for usage in the V4L2 compat layer. I'm interested in
knowing if all usages of V4L2PixelFormat::fromPixelFormat() from
pipeline handlers can be converted to
V4L2VideoDevice::toV4L2PixelFormat(), or if some would be problematic.
> 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list