[libcamera-devel] JPEG support for libcamera
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 7 23:32:30 CEST 2022
Hi Jacopo,
On Thu, Jul 07, 2022 at 05:42:38PM +0200, Jacopo Mondi wrote:
> On Thu, Jul 07, 2022 at 11:10:10AM +0300, Laurent Pinchart wrote:
> > 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 :-)
>
> Oh, it's even documented
>
> * planes formats. The two entries in the array store the contiguous and
> * non-contiguous V4L2 formats respectively. If the PixelFormat isn't a
>
> In facts planar/multiplanar API do not really play a role in format
> selection, but I thought we used that to indeitify the two
>
> > > 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
>
> I would rather pass the video device, but the concept is the same, if
> not that we would need an overload
Passing the video device is a good idea, I haven't thought about it.
> > 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
>
> I wonder if an overload with no formats list won't provide an easy
> way out for lazy developers not to get the video device format list..
Possibly, yes, but if we require pipeline handlers to always use
V4L2VideoDevice::toV4L2PixelFormat(), this could be caught during review
(and even flagged by checkstyle.py).
> > knowing if all usages of V4L2PixelFormat::fromPixelFormat() from
> > pipeline handlers can be converted to
> > V4L2VideoDevice::toV4L2PixelFormat(), or if some would be problematic.
>
> Let's find it out then...
>
> ----------------------------------------------------------------------------
> src/libcamera/pipeline/ipu3/cio2.cpp:
>
> - should be ok as the class has access to the output video device and
> can easily retrieve the list of supported formats. Please not this
> shouldn't even be necessary, as we know what formats the CIO2 videod
> device supports
>
> outputFormat->fourcc = V4L2PixelFormat::fromPixelFormat(itInfo->second);
> ...
> ret = output_->setFormat(outputFormat);
>
>
> ----------------------------------------------------------------------------
> src/libcamera/pipeline/ipu3/imgu.cpp:
>
> - Should equally be safe as the function that calls
> V4L2PixelFormat::fromPixelFormat() receives the video device as
> function parameter.
>
> ----------------------------------------------------------------------------
>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:
>
> - The toV4L2DeviceFormat() static function can be gives the video
> device (it's always Unicam::Image) the format should be converted for
>
> - In RPiCameraConfiguration::validate() the functions is used twice
>
> if (i == maxIndex)
> dev = data_->isp_[Isp::Output0].dev();
> else
> dev = data_->isp_[Isp::Output1].dev();
>
> V4L2VideoDevice::Formats fmts = dev->formats();
>
> if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt)) == fmts.end()) {
> /* If we cannot find a native format, use a default one. */
> cfgPixFmt = formats::NV12;
> status = Adjusted;
> }
>
> V4L2DeviceFormat format;
> format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> format.colorSpace = cfg.colorSpace;
>
> Let alone the fact this looks like a bug (format.fourcc should be
> assigned with cfgPixFmt?) the function can use 'dev'
Yes it looks like a bug, good point. Can I let you send a patch ?
> - In PipelineHandlerRPi::configure()
>
> V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> format.fourcc = fourcc;
> format.colorSpace = cfg.colorSpace;
>
> LOG(RPI, Debug) << "Setting " << stream->name() << " to "
> << format;
>
> ret = stream->dev()->setFormat(&format);
>
> stream->dev() can be used as well as in the following usages where
> formats are translated knowing on which video devices they're going
> to be applied on
>
>
> format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);
> ..
> ret = data->isp_[Isp::Output0].dev()->setFormat(&format);
>
> ----------------------------------------------------------------------------
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>
> - In RkISP1Path::validate()
>
> format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg->pixelFormat);
> ..
> int ret = video_->tryFormat(&format);
>
> We have access to video_
>
> - In RkISP1Path::configure()
>
> outputFormat.fourcc = V4L2PixelFormat::fromPixelFormat(config.pixelFormat);
> ...
> ret = video_->setFormat(&outputFormat);
>
> again video_ is available
>
> ----------------------------------------------------------------------------
>
> src/libcamera/pipeline/simple/converter.cpp
>
> - SimpleConverter::Stream::configure() applies formats on m2m_->capture()
> and m2m_->output()
>
> V4L2PixelFormat videoFormat =
> v4L2PixelFormat::fromPixelFormat(inputCfg.pixelFormat);
> ..
> int ret = m2m_->output()->setFormat(&format);
>
> - SimpleConverter::format() equally has access to m2m_->output()
>
> - SimpleConverter::strideAndFrameSize() has m2m_->capture()
>
> - SimpleCameraConfiguration::validate() knows on what video device the
> format has to be applied
>
> format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> ...
> int ret = data_->video_->tryFormat(&format);
>
> - SimplePipelineHandler::configure() has as well access to the video
> device
>
> V4L2PixelFormat videoFormat =
> V4L2PixelFormat::fromPixelFormat(pipeConfig->captureFormat);
>
> ..
> ret = video->setFormat(&captureFormat);
>
> ----------------------------------------------------------------------------
>
> src/libcamera/pipeline/vimc/vimc.cpp
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>
> - Have very similar patter where the format is converted just before
> being applied to the video device, and so the formats list can be
> accessed.
>
> ----------------------------------------------------------------------------
Sounds good so far :-) I'm looking forward to reviewing the patches.
> > > 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