[libcamera-devel] JPEG support for libcamera
Jacopo Mondi
jacopo at jmondi.org
Thu Jul 7 17:42:38 CEST 2022
Hi Laurent,
On Thu, Jul 07, 2022 at 11:10:10AM +0300, Laurent Pinchart wrote:
> 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 :-)
>
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
> 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..
> 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'
- 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.
----------------------------------------------------------------------------
>
> > 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