| Message ID | 20240713184327.23013-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote: > V4L2VideoDevice is using the caps to determine which kind of buffers to > use with the video-device in 2 different cases: > > 1. V4L2VideoDevice::open() > 2. V4L2VideoDevice::[get|try|set]Format() > > And the order in which the caps are checked is different between > these 2 cases. This is a problem for /dev/video# nodes which support > both video-capture and metadata buffers. open() sets bufferType_ to > V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as > [get|try|set]Format() will call [get|set]FormatMeta() which does not > work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. > > Switch [get|try|set]Format() to use the bufferType_ to determine on what > sort of buffers they should be operating, leaving the V4L2VideoDevice > code with only a single place where the decision is made what sort > of buffers it should operate on for a specific /dev/video# node. > > This will also allow to modify open() in the future to take a bufferType > argument to allow overriding the default bufferType it selects for > /dev/video# nodes which are capable of supporting more then 1 buffer type. Does this mean that, at the moment, on a device that supports both capture and metadata buffer types, video-capture is selected unconditionally and the metadata interface cannot be used ? If you plan to add an option to override the bufferType_ at open() time, does it mean it is needed to go through a open-close-open cycle to switch buffer type or: 1) The device can be opened multiple times and bufferType_ gets overriden ? (I'm not sure this is a good ideas as the last caller of open() selects the bufferType_) 2) Should we instead allow overriding the bufferType_ to use with an optional parameter to [get|try|set]Format() instead ? > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- > 1 file changed, 41 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4947aa3d..a8dc5355 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const > */ > int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > { > - if (caps_.isMeta()) > - return getFormatMeta(format); > - else if (caps_.isMultiplanar()) > - return getFormatMultiplane(format); > - else > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > return getFormatSingleplane(format); > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + return getFormatMultiplane(format); > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + return getFormatMeta(format); > + default: > + return -EINVAL; > + } > } > > /** > @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > */ > int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > { > - if (caps_.isMeta()) > - return trySetFormatMeta(format, false); > - else if (caps_.isMultiplanar()) > - return trySetFormatMultiplane(format, false); > - else > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > return trySetFormatSingleplane(format, false); > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + return trySetFormatMultiplane(format, false); > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + return trySetFormatMeta(format, false); > + default: > + return -EINVAL; > + } > } > > /** > @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > { > int ret = 0; I don't think it is necessary to initialize ret, but it wasn't already so Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > - if (caps_.isMeta()) > - ret = trySetFormatMeta(format, true); > - else if (caps_.isMultiplanar()) > - ret = trySetFormatMultiplane(format, true); > - else > + > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > ret = trySetFormatSingleplane(format, true); > + break; > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + ret = trySetFormatMultiplane(format, true); > + break; > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + ret = trySetFormatMeta(format, true); > + break; > + default: > + ret = -EINVAL; > + break; > + } > > /* Cache the set format on success. */ > if (ret) > -- > 2.45.2 >
Hi Jacopo, On 7/16/24 11:18 AM, Jacopo Mondi wrote: > Hi Hans > > On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote: >> V4L2VideoDevice is using the caps to determine which kind of buffers to >> use with the video-device in 2 different cases: >> >> 1. V4L2VideoDevice::open() >> 2. V4L2VideoDevice::[get|try|set]Format() >> >> And the order in which the caps are checked is different between >> these 2 cases. This is a problem for /dev/video# nodes which support >> both video-capture and metadata buffers. open() sets bufferType_ to >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as >> [get|try|set]Format() will call [get|set]FormatMeta() which does not >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. >> >> Switch [get|try|set]Format() to use the bufferType_ to determine on what >> sort of buffers they should be operating, leaving the V4L2VideoDevice >> code with only a single place where the decision is made what sort >> of buffers it should operate on for a specific /dev/video# node. >> >> This will also allow to modify open() in the future to take a bufferType >> argument to allow overriding the default bufferType it selects for >> /dev/video# nodes which are capable of supporting more then 1 buffer type. > > Does this mean that, at the moment, on a device that supports both > capture and metadata buffer types, video-capture is selected > unconditionally and the metadata interface cannot be used ? Yes, this is an improvement over the current situation where neither works :) Note the metadata API's are still not entirely set in stone in the kernel and are still disabled (behind an #ifdef) upstream atm. But the IPU6 driver already advertises V4L2_CAP_META_CAPTURE. So for now using video-capture instead of the not yet enabled metadata support is definitely the right thing to do. > If you plan to add an option to override the bufferType_ at open() > time, does it mean it is needed to go through a open-close-open cycle > to switch buffer type or: At least the IPU6 has multiple dma engines represented by multiple /dev/video# nodes. So the idea would be to open one for video capture and one for metadata capture and then open both requesting capture buffers while opening one and metadata buffers while opening the other. But yes switching type on a single node would require a close + open, note that changing type not only requires changing the bufferType_ of V4L2VideoDevice if switching between output/capture it also requires changing the fdBufferNotifier_ . So requiring a close() + open() to switch type does not seem like a strange requirement. > 1) The device can be opened multiple times and bufferType_ gets > overriden ? (I'm not sure this is a good ideas as the last caller of > open() selects the bufferType_) /dev/video# nodes can be opened multiple times, but only to allow querying caps / setting controls. Only one fd referring to it can requestBuffers at a time. So basically from a libcamera pov /dev/video# nodes should not be opened multiple times. > 2) Should we instead allow overriding the bufferType_ to use with an > optional parameter to [get|try|set]Format() instead ? bufferType_ is also used internally by the V4L2VideoDevice code for enumPixelformats(), setSelection(), requestBuffers(), createBuffer(), exportDmabufFd(), etc. The normal way of operating really is for there to be a fixed bufferType for one specific /dev/video# nodes. Which is why I'm suggesting to (in the future when needed for metadata support) to add an optional bufferType argument to open(), which basically fixates the bufferType at open() time for a /dev/video# node which can support multiple types. Regards, Hans > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- >> 1 file changed, 41 insertions(+), 15 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index 4947aa3d..a8dc5355 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const >> */ >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >> { >> - if (caps_.isMeta()) >> - return getFormatMeta(format); >> - else if (caps_.isMultiplanar()) >> - return getFormatMultiplane(format); >> - else >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> return getFormatSingleplane(format); >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + return getFormatMultiplane(format); >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + return getFormatMeta(format); >> + default: >> + return -EINVAL; >> + } >> } >> >> /** >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >> */ >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >> { >> - if (caps_.isMeta()) >> - return trySetFormatMeta(format, false); >> - else if (caps_.isMultiplanar()) >> - return trySetFormatMultiplane(format, false); >> - else >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> return trySetFormatSingleplane(format, false); >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + return trySetFormatMultiplane(format, false); >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + return trySetFormatMeta(format, false); >> + default: >> + return -EINVAL; >> + } >> } >> >> /** >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) >> { >> int ret = 0; > > I don't think it is necessary to initialize ret, but it wasn't already > so > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> - if (caps_.isMeta()) >> - ret = trySetFormatMeta(format, true); >> - else if (caps_.isMultiplanar()) >> - ret = trySetFormatMultiplane(format, true); >> - else >> + >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> ret = trySetFormatSingleplane(format, true); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + ret = trySetFormatMultiplane(format, true); >> + break; >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + ret = trySetFormatMeta(format, true); >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> >> /* Cache the set format on success. */ >> if (ret) >> -- >> 2.45.2 >> >
Hi Hans On Tue, Jul 16, 2024 at 12:16:39PM GMT, Hans de Goede wrote: > Hi Jacopo, > > On 7/16/24 11:18 AM, Jacopo Mondi wrote: > > Hi Hans > > > > On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote: > >> V4L2VideoDevice is using the caps to determine which kind of buffers to > >> use with the video-device in 2 different cases: > >> > >> 1. V4L2VideoDevice::open() > >> 2. V4L2VideoDevice::[get|try|set]Format() > >> > >> And the order in which the caps are checked is different between > >> these 2 cases. This is a problem for /dev/video# nodes which support > >> both video-capture and metadata buffers. open() sets bufferType_ to > >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as > >> [get|try|set]Format() will call [get|set]FormatMeta() which does not > >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. > >> > >> Switch [get|try|set]Format() to use the bufferType_ to determine on what > >> sort of buffers they should be operating, leaving the V4L2VideoDevice > >> code with only a single place where the decision is made what sort > >> of buffers it should operate on for a specific /dev/video# node. > >> > >> This will also allow to modify open() in the future to take a bufferType > >> argument to allow overriding the default bufferType it selects for > >> /dev/video# nodes which are capable of supporting more then 1 buffer type. > > > > Does this mean that, at the moment, on a device that supports both > > capture and metadata buffer types, video-capture is selected > > unconditionally and the metadata interface cannot be used ? > > Yes, this is an improvement over the current situation where > neither works :) > > Note the metadata API's are still not entirely set in stone in > the kernel and are still disabled (behind an #ifdef) upstream atm. Ah so we're not talking about the usual META_CAPTURE and META_OUTPUT interfaces used to exchange ISP parameters, but we're here looking at capturing sensor's produced metadata (which require streams and routing support which are currently disabled behind an #ifdef) > But the IPU6 driver already advertises V4L2_CAP_META_CAPTURE. So > for now using video-capture instead of the not yet enabled metadata > support is definitely the right thing to do. > > > If you plan to add an option to override the bufferType_ at open() > > time, does it mean it is needed to go through a open-close-open cycle > > to switch buffer type or: > > At least the IPU6 has multiple dma engines represented by multiple > /dev/video# nodes. So the idea would be to open one for video capture > and one for metadata capture and then open both requesting capture > buffers while opening one and metadata buffers while opening the other. I think I got lost in the sequence of "open" and "while opening" but I presume you mean that a video node can be used either for video-capture and for meta-data capture, depending on the buffer type it is initialized with (I had a brief look at ipu6-isys-video.c and I see the vb2 queue being initialized with the VIDEO_CAPTURE type unconditionally, I wonder how the META_CAPTURE interface will be selected in future, but that's a different topic). > > But yes switching type on a single node would require a close + open, > note that changing type not only requires changing the bufferType_ > of V4L2VideoDevice if switching between output/capture it also > requires changing the fdBufferNotifier_ . So requiring a close() > + open() to switch type does not seem like a strange requirement. > > > 1) The device can be opened multiple times and bufferType_ gets > > overriden ? (I'm not sure this is a good ideas as the last caller of > > open() selects the bufferType_) > > /dev/video# nodes can be opened multiple times, but only to allow > querying caps / setting controls. Only one fd referring to it > can requestBuffers at a time. So basically from a libcamera pov > /dev/video# nodes should not be opened multiple times. > ok > > 2) Should we instead allow overriding the bufferType_ to use with an > > optional parameter to [get|try|set]Format() instead ? > > bufferType_ is also used internally by the V4L2VideoDevice code for > enumPixelformats(), setSelection(), requestBuffers(), createBuffer(), > exportDmabufFd(), etc. > > The normal way of operating really is for there to be a fixed bufferType > for one specific /dev/video# nodes. Which is why I'm suggesting to > (in the future when needed for metadata support) to add an optional > bufferType argument to open(), which basically fixates the bufferType > at open() time for a /dev/video# node which can support multiple types. We should probably make sure then that if a video device is 'forced' to use a buffer type at open() time, all the follwing open() call won't force it to a different buffer type ? But this is indeed for later. Thanks j > > Regards, > > Hans > > > > > > > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- > >> 1 file changed, 41 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index 4947aa3d..a8dc5355 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const > >> */ > >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > >> { > >> - if (caps_.isMeta()) > >> - return getFormatMeta(format); > >> - else if (caps_.isMultiplanar()) > >> - return getFormatMultiplane(format); > >> - else > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> return getFormatSingleplane(format); > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + return getFormatMultiplane(format); > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + return getFormatMeta(format); > >> + default: > >> + return -EINVAL; > >> + } > >> } > >> > >> /** > >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > >> */ > >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > >> { > >> - if (caps_.isMeta()) > >> - return trySetFormatMeta(format, false); > >> - else if (caps_.isMultiplanar()) > >> - return trySetFormatMultiplane(format, false); > >> - else > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> return trySetFormatSingleplane(format, false); > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + return trySetFormatMultiplane(format, false); > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + return trySetFormatMeta(format, false); > >> + default: > >> + return -EINVAL; > >> + } > >> } > >> > >> /** > >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > >> { > >> int ret = 0; > > > > I don't think it is necessary to initialize ret, but it wasn't already > > so > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > >> - if (caps_.isMeta()) > >> - ret = trySetFormatMeta(format, true); > >> - else if (caps_.isMultiplanar()) > >> - ret = trySetFormatMultiplane(format, true); > >> - else > >> + > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> ret = trySetFormatSingleplane(format, true); > >> + break; > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + ret = trySetFormatMultiplane(format, true); > >> + break; > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + ret = trySetFormatMeta(format, true); > >> + break; > >> + default: > >> + ret = -EINVAL; > >> + break; > >> + } > >> > >> /* Cache the set format on success. */ > >> if (ret) > >> -- > >> 2.45.2 > >> > > >
Hi Hans, Thank you for the patch. On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote: > V4L2VideoDevice is using the caps to determine which kind of buffers to > use with the video-device in 2 different cases: > > 1. V4L2VideoDevice::open() > 2. V4L2VideoDevice::[get|try|set]Format() > > And the order in which the caps are checked is different between > these 2 cases. This is a problem for /dev/video# nodes which support > both video-capture and metadata buffers. open() sets bufferType_ to > V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as > [get|try|set]Format() will call [get|set]FormatMeta() which does not > work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. > > Switch [get|try|set]Format() to use the bufferType_ to determine on what > sort of buffers they should be operating, leaving the V4L2VideoDevice > code with only a single place where the decision is made what sort > of buffers it should operate on for a specific /dev/video# node. > > This will also allow to modify open() in the future to take a bufferType > argument to allow overriding the default bufferType it selects for > /dev/video# nodes which are capable of supporting more then 1 buffer type. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- > 1 file changed, 41 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4947aa3d..a8dc5355 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const > */ > int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > { > - if (caps_.isMeta()) > - return getFormatMeta(format); > - else if (caps_.isMultiplanar()) > - return getFormatMultiplane(format); > - else > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > return getFormatSingleplane(format); > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + return getFormatMultiplane(format); > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + return getFormatMeta(format); > + default: > + return -EINVAL; This can never happen, so you could fold the default vase with any of the above. I suppose returning -EINVAL could help catching future errors when we'll update open() to support more types but forget to update the code here. Is that a real risk ? > + } > } > > /** > @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > */ > int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > { > - if (caps_.isMeta()) > - return trySetFormatMeta(format, false); > - else if (caps_.isMultiplanar()) > - return trySetFormatMultiplane(format, false); > - else > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > return trySetFormatSingleplane(format, false); > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + return trySetFormatMultiplane(format, false); > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + return trySetFormatMeta(format, false); > + default: > + return -EINVAL; Same here. > + } > } > > /** > @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > { > int ret = 0; While at it you can drop the initialization of the ret variable. > - if (caps_.isMeta()) > - ret = trySetFormatMeta(format, true); > - else if (caps_.isMultiplanar()) > - ret = trySetFormatMultiplane(format, true); > - else > + > + switch (bufferType_) { > + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > ret = trySetFormatSingleplane(format, true); > + break; > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > + ret = trySetFormatMultiplane(format, true); > + break; > + case V4L2_BUF_TYPE_META_CAPTURE: > + case V4L2_BUF_TYPE_META_OUTPUT: > + ret = trySetFormatMeta(format, true); > + break; > + default: > + ret = -EINVAL; > + break; And here. If you think we should drop the default case, please send a new version. Otherwise I can drop the initialization of ret when applying. Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > > /* Cache the set format on success. */ > if (ret)
Hi Laurent, On 7/19/24 9:37 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote: >> V4L2VideoDevice is using the caps to determine which kind of buffers to >> use with the video-device in 2 different cases: >> >> 1. V4L2VideoDevice::open() >> 2. V4L2VideoDevice::[get|try|set]Format() >> >> And the order in which the caps are checked is different between >> these 2 cases. This is a problem for /dev/video# nodes which support >> both video-capture and metadata buffers. open() sets bufferType_ to >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as >> [get|try|set]Format() will call [get|set]FormatMeta() which does not >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. >> >> Switch [get|try|set]Format() to use the bufferType_ to determine on what >> sort of buffers they should be operating, leaving the V4L2VideoDevice >> code with only a single place where the decision is made what sort >> of buffers it should operate on for a specific /dev/video# node. >> >> This will also allow to modify open() in the future to take a bufferType >> argument to allow overriding the default bufferType it selects for >> /dev/video# nodes which are capable of supporting more then 1 buffer type. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- >> 1 file changed, 41 insertions(+), 15 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index 4947aa3d..a8dc5355 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const >> */ >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >> { >> - if (caps_.isMeta()) >> - return getFormatMeta(format); >> - else if (caps_.isMultiplanar()) >> - return getFormatMultiplane(format); >> - else >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> return getFormatSingleplane(format); >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + return getFormatMultiplane(format); >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + return getFormatMeta(format); >> + default: >> + return -EINVAL; > > This can never happen, so you could fold the default vase with any of > the above. I suppose returning -EINVAL could help catching future errors > when we'll update open() to support more types. Right I'm not a fan of just adding a default: to some existing case when there is no clear default existing case. > but forget to update the > code here. Is that a real risk ? Humans make mistakes so yes IMHO this is real risk and I would prefer to keep this bit as is. > >> + } >> } >> >> /** >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >> */ >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >> { >> - if (caps_.isMeta()) >> - return trySetFormatMeta(format, false); >> - else if (caps_.isMultiplanar()) >> - return trySetFormatMultiplane(format, false); >> - else >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> return trySetFormatSingleplane(format, false); >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + return trySetFormatMultiplane(format, false); >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + return trySetFormatMeta(format, false); >> + default: >> + return -EINVAL; > > Same here. > >> + } >> } >> >> /** >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) >> { >> int ret = 0; > > While at it you can drop the initialization of the ret variable. Ack. > >> - if (caps_.isMeta()) >> - ret = trySetFormatMeta(format, true); >> - else if (caps_.isMultiplanar()) >> - ret = trySetFormatMultiplane(format, true); >> - else >> + >> + switch (bufferType_) { >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> ret = trySetFormatSingleplane(format, true); >> + break; >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> + ret = trySetFormatMultiplane(format, true); >> + break; >> + case V4L2_BUF_TYPE_META_CAPTURE: >> + case V4L2_BUF_TYPE_META_OUTPUT: >> + ret = trySetFormatMeta(format, true); >> + break; >> + default: >> + ret = -EINVAL; >> + break; > > And here. > > If you think we should drop the default case, please send a new version. > Otherwise I can drop the initialization of ret when applying. Either > way, As mendtioned above I prefer to keep the default case. If you can drop the initialization of ret while applying this that would be great. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks & Regsrds, Hans >> >> /* Cache the set format on success. */ >> if (ret) >
Hi Hans, On Fri, Jul 19, 2024 at 03:07:26PM +0200, Hans de Goede wrote: > On 7/19/24 9:37 AM, Laurent Pinchart wrote: > > On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote: > >> V4L2VideoDevice is using the caps to determine which kind of buffers to > >> use with the video-device in 2 different cases: > >> > >> 1. V4L2VideoDevice::open() > >> 2. V4L2VideoDevice::[get|try|set]Format() > >> > >> And the order in which the caps are checked is different between > >> these 2 cases. This is a problem for /dev/video# nodes which support > >> both video-capture and metadata buffers. open() sets bufferType_ to > >> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as > >> [get|try|set]Format() will call [get|set]FormatMeta() which does not > >> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. > >> > >> Switch [get|try|set]Format() to use the bufferType_ to determine on what > >> sort of buffers they should be operating, leaving the V4L2VideoDevice > >> code with only a single place where the decision is made what sort > >> of buffers it should operate on for a specific /dev/video# node. > >> > >> This will also allow to modify open() in the future to take a bufferType > >> argument to allow overriding the default bufferType it selects for > >> /dev/video# nodes which are capable of supporting more then 1 buffer type. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- > >> 1 file changed, 41 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index 4947aa3d..a8dc5355 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const > >> */ > >> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > >> { > >> - if (caps_.isMeta()) > >> - return getFormatMeta(format); > >> - else if (caps_.isMultiplanar()) > >> - return getFormatMultiplane(format); > >> - else > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> return getFormatSingleplane(format); > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + return getFormatMultiplane(format); > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + return getFormatMeta(format); > >> + default: > >> + return -EINVAL; > > > > This can never happen, so you could fold the default vase with any of > > the above. I suppose returning -EINVAL could help catching future errors > > when we'll update open() to support more types. > > Right I'm not a fan of just adding a default: to some existing case > when there is no clear default existing case. > > > but forget to update the > > code here. Is that a real risk ? > > Humans make mistakes so yes IMHO this is real risk and I would > prefer to keep this bit as is. OK. > >> + } > >> } > >> > >> /** > >> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) > >> */ > >> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > >> { > >> - if (caps_.isMeta()) > >> - return trySetFormatMeta(format, false); > >> - else if (caps_.isMultiplanar()) > >> - return trySetFormatMultiplane(format, false); > >> - else > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> return trySetFormatSingleplane(format, false); > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + return trySetFormatMultiplane(format, false); > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + return trySetFormatMeta(format, false); > >> + default: > >> + return -EINVAL; > > > > Same here. > > > >> + } > >> } > >> > >> /** > >> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > >> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > >> { > >> int ret = 0; > > > > While at it you can drop the initialization of the ret variable. > > Ack. > > >> - if (caps_.isMeta()) > >> - ret = trySetFormatMeta(format, true); > >> - else if (caps_.isMultiplanar()) > >> - ret = trySetFormatMultiplane(format, true); > >> - else > >> + > >> + switch (bufferType_) { > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: > >> ret = trySetFormatSingleplane(format, true); > >> + break; > >> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > >> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > >> + ret = trySetFormatMultiplane(format, true); > >> + break; > >> + case V4L2_BUF_TYPE_META_CAPTURE: > >> + case V4L2_BUF_TYPE_META_OUTPUT: > >> + ret = trySetFormatMeta(format, true); > >> + break; > >> + default: > >> + ret = -EINVAL; > >> + break; > > > > And here. > > > > If you think we should drop the default case, please send a new version. > > Otherwise I can drop the initialization of ret when applying. Either > > way, > > As mendtioned above I prefer to keep the default case. If you can > drop the initialization of ret while applying this that would be > great. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks & Regsrds, Pushed. Unless I'm mistaken, initial IPU6 support is now fully merged. > >> > >> /* Cache the set format on success. */ > >> if (ret)
Hi, On 7/21/24 6:33 PM, Laurent Pinchart wrote: > Hi Hans, > > On Fri, Jul 19, 2024 at 03:07:26PM +0200, Hans de Goede wrote: >> On 7/19/24 9:37 AM, Laurent Pinchart wrote: >>> On Sat, Jul 13, 2024 at 08:43:27PM +0200, Hans de Goede wrote: >>>> V4L2VideoDevice is using the caps to determine which kind of buffers to >>>> use with the video-device in 2 different cases: >>>> >>>> 1. V4L2VideoDevice::open() >>>> 2. V4L2VideoDevice::[get|try|set]Format() >>>> >>>> And the order in which the caps are checked is different between >>>> these 2 cases. This is a problem for /dev/video# nodes which support >>>> both video-capture and metadata buffers. open() sets bufferType_ to >>>> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as >>>> [get|try|set]Format() will call [get|set]FormatMeta() which does not >>>> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. >>>> >>>> Switch [get|try|set]Format() to use the bufferType_ to determine on what >>>> sort of buffers they should be operating, leaving the V4L2VideoDevice >>>> code with only a single place where the decision is made what sort >>>> of buffers it should operate on for a specific /dev/video# node. >>>> >>>> This will also allow to modify open() in the future to take a bufferType >>>> argument to allow overriding the default bufferType it selects for >>>> /dev/video# nodes which are capable of supporting more then 1 buffer type. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- >>>> 1 file changed, 41 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >>>> index 4947aa3d..a8dc5355 100644 >>>> --- a/src/libcamera/v4l2_videodevice.cpp >>>> +++ b/src/libcamera/v4l2_videodevice.cpp >>>> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const >>>> */ >>>> int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >>>> { >>>> - if (caps_.isMeta()) >>>> - return getFormatMeta(format); >>>> - else if (caps_.isMultiplanar()) >>>> - return getFormatMultiplane(format); >>>> - else >>>> + switch (bufferType_) { >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >>>> return getFormatSingleplane(format); >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >>>> + return getFormatMultiplane(format); >>>> + case V4L2_BUF_TYPE_META_CAPTURE: >>>> + case V4L2_BUF_TYPE_META_OUTPUT: >>>> + return getFormatMeta(format); >>>> + default: >>>> + return -EINVAL; >>> >>> This can never happen, so you could fold the default vase with any of >>> the above. I suppose returning -EINVAL could help catching future errors >>> when we'll update open() to support more types. >> >> Right I'm not a fan of just adding a default: to some existing case >> when there is no clear default existing case. >> >>> but forget to update the >>> code here. Is that a real risk ? >> >> Humans make mistakes so yes IMHO this is real risk and I would >> prefer to keep this bit as is. > > OK. > >>>> + } >>>> } >>>> >>>> /** >>>> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) >>>> */ >>>> int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >>>> { >>>> - if (caps_.isMeta()) >>>> - return trySetFormatMeta(format, false); >>>> - else if (caps_.isMultiplanar()) >>>> - return trySetFormatMultiplane(format, false); >>>> - else >>>> + switch (bufferType_) { >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >>>> return trySetFormatSingleplane(format, false); >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >>>> + return trySetFormatMultiplane(format, false); >>>> + case V4L2_BUF_TYPE_META_CAPTURE: >>>> + case V4L2_BUF_TYPE_META_OUTPUT: >>>> + return trySetFormatMeta(format, false); >>>> + default: >>>> + return -EINVAL; >>> >>> Same here. >>> >>>> + } >>>> } >>>> >>>> /** >>>> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) >>>> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) >>>> { >>>> int ret = 0; >>> >>> While at it you can drop the initialization of the ret variable. >> >> Ack. >> >>>> - if (caps_.isMeta()) >>>> - ret = trySetFormatMeta(format, true); >>>> - else if (caps_.isMultiplanar()) >>>> - ret = trySetFormatMultiplane(format, true); >>>> - else >>>> + >>>> + switch (bufferType_) { >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT: >>>> ret = trySetFormatSingleplane(format, true); >>>> + break; >>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: >>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >>>> + ret = trySetFormatMultiplane(format, true); >>>> + break; >>>> + case V4L2_BUF_TYPE_META_CAPTURE: >>>> + case V4L2_BUF_TYPE_META_OUTPUT: >>>> + ret = trySetFormatMeta(format, true); >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + break; >>> >>> And here. >>> >>> If you think we should drop the default case, please send a new version. >>> Otherwise I can drop the initialization of ret when applying. Either >>> way, >> >> As mendtioned above I prefer to keep the default case. If you can >> drop the initialization of ret while applying this that would be >> great. >> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Thanks & Regsrds, > > Pushed. Thank you. > Unless I'm mistaken, initial IPU6 support is now fully merged. Correct \o/ Regards, Hans
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 4947aa3d..a8dc5355 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const */ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) { - if (caps_.isMeta()) - return getFormatMeta(format); - else if (caps_.isMultiplanar()) - return getFormatMultiplane(format); - else + switch (bufferType_) { + case V4L2_BUF_TYPE_VIDEO_CAPTURE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT: return getFormatSingleplane(format); + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: + return getFormatMultiplane(format); + case V4L2_BUF_TYPE_META_CAPTURE: + case V4L2_BUF_TYPE_META_OUTPUT: + return getFormatMeta(format); + default: + return -EINVAL; + } } /** @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format) */ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) { - if (caps_.isMeta()) - return trySetFormatMeta(format, false); - else if (caps_.isMultiplanar()) - return trySetFormatMultiplane(format, false); - else + switch (bufferType_) { + case V4L2_BUF_TYPE_VIDEO_CAPTURE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT: return trySetFormatSingleplane(format, false); + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: + return trySetFormatMultiplane(format, false); + case V4L2_BUF_TYPE_META_CAPTURE: + case V4L2_BUF_TYPE_META_OUTPUT: + return trySetFormatMeta(format, false); + default: + return -EINVAL; + } } /** @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) { int ret = 0; - if (caps_.isMeta()) - ret = trySetFormatMeta(format, true); - else if (caps_.isMultiplanar()) - ret = trySetFormatMultiplane(format, true); - else + + switch (bufferType_) { + case V4L2_BUF_TYPE_VIDEO_CAPTURE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT: ret = trySetFormatSingleplane(format, true); + break; + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: + ret = trySetFormatMultiplane(format, true); + break; + case V4L2_BUF_TYPE_META_CAPTURE: + case V4L2_BUF_TYPE_META_OUTPUT: + ret = trySetFormatMeta(format, true); + break; + default: + ret = -EINVAL; + break; + } /* Cache the set format on success. */ if (ret)
V4L2VideoDevice is using the caps to determine which kind of buffers to use with the video-device in 2 different cases: 1. V4L2VideoDevice::open() 2. V4L2VideoDevice::[get|try|set]Format() And the order in which the caps are checked is different between these 2 cases. This is a problem for /dev/video# nodes which support both video-capture and metadata buffers. open() sets bufferType_ to V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as [get|try|set]Format() will call [get|set]FormatMeta() which does not work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers. Switch [get|try|set]Format() to use the bufferType_ to determine on what sort of buffers they should be operating, leaving the V4L2VideoDevice code with only a single place where the decision is made what sort of buffers it should operate on for a specific /dev/video# node. This will also allow to modify open() in the future to take a bufferType argument to allow overriding the default bufferType it selects for /dev/video# nodes which are capable of supporting more then 1 buffer type. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 15 deletions(-)