| Message ID | 20190226162641.12116-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 26/02/2019 16:26, Jacopo Mondi wrote: > Add support for devices that provide video meta-data to v4l2_device.cpp > and re-arrange bufferType handling in open() method. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 4 +++ > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 1d31d1b403bc..52eb6785cc15 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE_MPLANE); > } > + bool isMeta() const > + { > + return device_caps() & V4L2_CAP_META_CAPTURE; > + } > bool isOutput() const > { > return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 24e115554a99..8be8af7a2893 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > * \return True if the device can output video frames > */ > > +/** > + * \fn bool V4L2Capability::isMeta() > + * \brief Identify if the device is capable of providing video meta-data > + * > + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 I think I saw Niklas suggest to make this a Todo: rather than fixme - but I don't think we need to worry too much over terminology that should be removed within a couple of weeks :D as We'll update this after the 5.0 is released. Likewise, I think the conditionals below might get some more thought put into them when we add in META_OUTPUT - but that's all future work. This patch gets us closer. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + * > + * \return True if the device can provide video meta-data > + */ > + > /** > * \fn bool V4L2Capability::hasStreaming() > * \brief Determine if the device can perform Streaming I/O > @@ -280,33 +289,32 @@ int V4L2Device::open() > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > - if (!caps_.isCapture() && !caps_.isOutput()) { > - LOG(V4L2, Debug) << "Device is not a supported type"; > - return -EINVAL; > - } > - > if (!caps_.hasStreaming()) { > LOG(V4L2, Error) << "Device does not support streaming I/O"; > return -EINVAL; > } > > - if (caps_.isCapture()) > + /* > + * Set buffer type and wait for read notifications on CAPTURE devices > + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > + */ > + if (caps_.isCapture()) { > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > : V4L2_BUF_TYPE_VIDEO_CAPTURE; > - else > + } else if (caps_.isOutput()) { > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > : V4L2_BUF_TYPE_VIDEO_OUTPUT; > - > - /* > - * We wait for Read notifications on CAPTURE devices (POLLIN), and > - * Write notifications for OUTPUT devices (POLLOUT). > - */ > - if (caps_.isCapture()) > + } else if (caps_.isMeta()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > - else > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > + } else { > + LOG(V4L2, Debug) << "Device is not a supported type"; > + return -EINVAL; > + } > > fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > fdEvent_->setEnabled(false); >
On 26/02/2019 23:24, Kieran Bingham wrote: > Hi Jacopo, > > On 26/02/2019 16:26, Jacopo Mondi wrote: >> Add support for devices that provide video meta-data to v4l2_device.cpp >> and re-arrange bufferType handling in open() method. >> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> src/libcamera/include/v4l2_device.h | 4 +++ >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ >> 2 files changed, 27 insertions(+), 15 deletions(-) >> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h >> index 1d31d1b403bc..52eb6785cc15 100644 >> --- a/src/libcamera/include/v4l2_device.h >> +++ b/src/libcamera/include/v4l2_device.h >> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { >> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | >> V4L2_CAP_VIDEO_CAPTURE_MPLANE); >> } >> + bool isMeta() const One last thought here, I'm a bit concerned that if we have isMeta and isOutput handling this - we might end up with an ugly 2 x 2 matrix in if statements. Any pretty way to make this better? Or would it be cleaner to move towards: bool isMetaCapture() bool isMetaOutput() This bit feels like a pain point - but I don't think that should stop this patch right now. >> + { >> + return device_caps() & V4L2_CAP_META_CAPTURE; >> + } >> bool isOutput() const >> { >> return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index 24e115554a99..8be8af7a2893 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) >> * \return True if the device can output video frames >> */ >> >> +/** >> + * \fn bool V4L2Capability::isMeta() >> + * \brief Identify if the device is capable of providing video meta-data >> + * >> + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 > > I think I saw Niklas suggest to make this a Todo: rather than fixme - > but I don't think we need to worry too much over terminology that should > be removed within a couple of weeks :D as We'll update this after the > 5.0 is released. > > > Likewise, I think the conditionals below might get some more thought put > into them when we add in META_OUTPUT - but that's all future work. > > This patch gets us closer. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> + * >> + * \return True if the device can provide video meta-data >> + */ >> + >> /** >> * \fn bool V4L2Capability::hasStreaming() >> * \brief Determine if the device can perform Streaming I/O >> @@ -280,33 +289,32 @@ int V4L2Device::open() >> << "Opened device " << caps_.bus_info() << ": " >> << caps_.driver() << ": " << caps_.card(); >> >> - if (!caps_.isCapture() && !caps_.isOutput()) { >> - LOG(V4L2, Debug) << "Device is not a supported type"; >> - return -EINVAL; >> - } >> - >> if (!caps_.hasStreaming()) { >> LOG(V4L2, Error) << "Device does not support streaming I/O"; >> return -EINVAL; >> } >> >> - if (caps_.isCapture()) >> + /* >> + * Set buffer type and wait for read notifications on CAPTURE devices >> + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). >> + */ >> + if (caps_.isCapture()) { >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); >> bufferType_ = caps_.isMultiplanar() >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; >> - else >> + } else if (caps_.isOutput()) { >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); >> bufferType_ = caps_.isMultiplanar() >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; >> - >> - /* >> - * We wait for Read notifications on CAPTURE devices (POLLIN), and >> - * Write notifications for OUTPUT devices (POLLOUT). >> - */ >> - if (caps_.isCapture()) >> + } else if (caps_.isMeta()) { >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); >> - else >> - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); >> + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; >> + } else { >> + LOG(V4L2, Debug) << "Device is not a supported type"; >> + return -EINVAL; >> + } >> >> fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); >> fdEvent_->setEnabled(false); >> >
Hi Kieran, On Tue, Feb 26, 2019 at 11:28:09PM +0000, Kieran Bingham wrote: > > > On 26/02/2019 23:24, Kieran Bingham wrote: > > Hi Jacopo, > > > > On 26/02/2019 16:26, Jacopo Mondi wrote: > >> Add support for devices that provide video meta-data to v4l2_device.cpp > >> and re-arrange bufferType handling in open() method. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/include/v4l2_device.h | 4 +++ > >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > >> 2 files changed, 27 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >> index 1d31d1b403bc..52eb6785cc15 100644 > >> --- a/src/libcamera/include/v4l2_device.h > >> +++ b/src/libcamera/include/v4l2_device.h > >> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > >> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > >> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > >> } > >> + bool isMeta() const > > One last thought here, > > I'm a bit concerned that if we have isMeta and isOutput handling this - > we might end up with an ugly 2 x 2 matrix in if statements. > > Any pretty way to make this better? > > Or would it be cleaner to move towards: > > bool isMetaCapture() > bool isMetaOutput() In prevision of introducing support for META_OUTPUT, I have now named this 'isMetaCapture()'. I'm not so concerned about the if matrix, it will look like: if (caps_.isCapture()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : V4L2_BUF_TYPE_VIDEO_CAPTURE; } else if (caps_.isOutput()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE : V4L2_BUF_TYPE_VIDEO_OUTPUT; } else if (caps_.isMetaCapture()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; } else if (caps_.isMetaOutput()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; } else { LOG(V4L2, Debug) << "Device is not a supported type"; return -EINVAL; } We could rename isCapture/isOutput to isVideoCapture/isVideoOutput for simmetry, but I don't think that's a big deal, isn't it? Thanks j > > This bit feels like a pain point - but I don't think that should stop > this patch right now. > > > > > >> + { > >> + return device_caps() & V4L2_CAP_META_CAPTURE; > >> + } > >> bool isOutput() const > >> { > >> return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index 24e115554a99..8be8af7a2893 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > >> * \return True if the device can output video frames > >> */ > >> > >> +/** > >> + * \fn bool V4L2Capability::isMeta() > >> + * \brief Identify if the device is capable of providing video meta-data > >> + * > >> + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 > > > > I think I saw Niklas suggest to make this a Todo: rather than fixme - > > but I don't think we need to worry too much over terminology that should > > be removed within a couple of weeks :D as We'll update this after the > > 5.0 is released. > > > > > > Likewise, I think the conditionals below might get some more thought put > > into them when we add in META_OUTPUT - but that's all future work. > > > > This patch gets us closer. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > >> + * > >> + * \return True if the device can provide video meta-data > >> + */ > >> + > >> /** > >> * \fn bool V4L2Capability::hasStreaming() > >> * \brief Determine if the device can perform Streaming I/O > >> @@ -280,33 +289,32 @@ int V4L2Device::open() > >> << "Opened device " << caps_.bus_info() << ": " > >> << caps_.driver() << ": " << caps_.card(); > >> > >> - if (!caps_.isCapture() && !caps_.isOutput()) { > >> - LOG(V4L2, Debug) << "Device is not a supported type"; > >> - return -EINVAL; > >> - } > >> - > >> if (!caps_.hasStreaming()) { > >> LOG(V4L2, Error) << "Device does not support streaming I/O"; > >> return -EINVAL; > >> } > >> > >> - if (caps_.isCapture()) > >> + /* > >> + * Set buffer type and wait for read notifications on CAPTURE devices > >> + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > >> + */ > >> + if (caps_.isCapture()) { > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > >> - else > >> + } else if (caps_.isOutput()) { > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > >> - > >> - /* > >> - * We wait for Read notifications on CAPTURE devices (POLLIN), and > >> - * Write notifications for OUTPUT devices (POLLOUT). > >> - */ > >> - if (caps_.isCapture()) > >> + } else if (caps_.isMeta()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> - else > >> - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > >> + } else { > >> + LOG(V4L2, Debug) << "Device is not a supported type"; > >> + return -EINVAL; > >> + } > >> > >> fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > >> fdEvent_->setEnabled(false); > >> > > > > -- > Regards > -- > Kieran
Hi Jacopo, Thank you for the patch. On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > Add support for devices that provide video meta-data to v4l2_device.cpp > and re-arrange bufferType handling in open() method. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 4 +++ > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 1d31d1b403bc..52eb6785cc15 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE_MPLANE); > } > + bool isMeta() const > + { > + return device_caps() & V4L2_CAP_META_CAPTURE; > + } > bool isOutput() const > { > return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | Even if you don't introduce support for V4L2_CAP_META_OUTPUT now, isCapture() should still return true for V4L2_CAP_META_CAPTURE to ease transition later. Don't forget to update the isOutput() and isCapture() documentation to not mention video frames anymore, but just output and capture. > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 24e115554a99..8be8af7a2893 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > * \return True if the device can output video frames > */ > > +/** > + * \fn bool V4L2Capability::isMeta() > + * \brief Identify if the device is capable of providing video meta-data > + * > + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 Maybe \todo ? s/v4.20/v5.0/ > + * > + * \return True if the device can provide video meta-data > + */ > + > /** > * \fn bool V4L2Capability::hasStreaming() > * \brief Determine if the device can perform Streaming I/O > @@ -280,33 +289,32 @@ int V4L2Device::open() > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > - if (!caps_.isCapture() && !caps_.isOutput()) { > - LOG(V4L2, Debug) << "Device is not a supported type"; > - return -EINVAL; > - } > - > if (!caps_.hasStreaming()) { > LOG(V4L2, Error) << "Device does not support streaming I/O"; > return -EINVAL; > } > > - if (caps_.isCapture()) > + /* > + * Set buffer type and wait for read notifications on CAPTURE devices > + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > + */ > + if (caps_.isCapture()) { > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > : V4L2_BUF_TYPE_VIDEO_CAPTURE; > - else > + } else if (caps_.isOutput()) { > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > : V4L2_BUF_TYPE_VIDEO_OUTPUT; > - > - /* > - * We wait for Read notifications on CAPTURE devices (POLLIN), and > - * Write notifications for OUTPUT devices (POLLOUT). > - */ > - if (caps_.isCapture()) > + } else if (caps_.isMeta()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > - else > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > + } else { > + LOG(V4L2, Debug) << "Device is not a supported type"; > + return -EINVAL; > + } > > fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > fdEvent_->setEnabled(false);
Hi Laurent, On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > > Add support for devices that provide video meta-data to v4l2_device.cpp > > and re-arrange bufferType handling in open() method. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_device.h | 4 +++ > > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > > 2 files changed, 27 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index 1d31d1b403bc..52eb6785cc15 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > > return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > > V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > } > > + bool isMeta() const > > + { > > + return device_caps() & V4L2_CAP_META_CAPTURE; > > + } > > bool isOutput() const > > { > > return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | > > Even if you don't introduce support for V4L2_CAP_META_OUTPUT now, > isCapture() should still return true for V4L2_CAP_META_CAPTURE to ease > transition later. Don't forget to update the isOutput() and isCapture() > documentation to not mention video frames anymore, but just output and > capture. > As done in v4, I introduced an isMetaCapture() method, preparing for an isMetaOutput() one. In this way isCapture() and isOutput() would refer to video streams only and the bufferType intialization looks like: * Set buffer type and wait for read notifications on CAPTURE devices * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). */ if (caps_.isCapture()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : V4L2_BUF_TYPE_VIDEO_CAPTURE; } else if (caps_.isOutput()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE : V4L2_BUF_TYPE_VIDEO_OUTPUT; } else if (caps_.isMetaCapture()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; } else if (caps_.isMetaOutput()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; } else { LOG(V4L2, Debug) << "Device is not a supported type"; return -EINVAL; } > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 24e115554a99..8be8af7a2893 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > > * \return True if the device can output video frames > > */ > > > > +/** > > + * \fn bool V4L2Capability::isMeta() > > + * \brief Identify if the device is capable of providing video meta-data > > + * > > + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 > > Maybe \todo ? > > s/v4.20/v5.0/ > > > + * > > + * \return True if the device can provide video meta-data > > + */ > > + > > /** > > * \fn bool V4L2Capability::hasStreaming() > > * \brief Determine if the device can perform Streaming I/O > > @@ -280,33 +289,32 @@ int V4L2Device::open() > > << "Opened device " << caps_.bus_info() << ": " > > << caps_.driver() << ": " << caps_.card(); > > > > - if (!caps_.isCapture() && !caps_.isOutput()) { > > - LOG(V4L2, Debug) << "Device is not a supported type"; > > - return -EINVAL; > > - } > > - > > if (!caps_.hasStreaming()) { > > LOG(V4L2, Error) << "Device does not support streaming I/O"; > > return -EINVAL; > > } > > > > - if (caps_.isCapture()) > > + /* > > + * Set buffer type and wait for read notifications on CAPTURE devices > > + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > > + */ > > + if (caps_.isCapture()) { > > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > bufferType_ = caps_.isMultiplanar() > > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > > : V4L2_BUF_TYPE_VIDEO_CAPTURE; > > - else > > + } else if (caps_.isOutput()) { > > + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > bufferType_ = caps_.isMultiplanar() > > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > : V4L2_BUF_TYPE_VIDEO_OUTPUT; > > - > > - /* > > - * We wait for Read notifications on CAPTURE devices (POLLIN), and > > - * Write notifications for OUTPUT devices (POLLOUT). > > - */ > > - if (caps_.isCapture()) > > + } else if (caps_.isMeta()) { > > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > - else > > - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > > + } else { > > + LOG(V4L2, Debug) << "Device is not a supported type"; > > + return -EINVAL; > > + } > > > > fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > > fdEvent_->setEnabled(false); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote: > On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > > On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > >> Add support for devices that provide video meta-data to v4l2_device.cpp > >> and re-arrange bufferType handling in open() method. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/include/v4l2_device.h | 4 +++ > >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > >> 2 files changed, 27 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >> index 1d31d1b403bc..52eb6785cc15 100644 > >> --- a/src/libcamera/include/v4l2_device.h > >> +++ b/src/libcamera/include/v4l2_device.h > >> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > >> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > >> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > >> } > >> + bool isMeta() const > >> + { > >> + return device_caps() & V4L2_CAP_META_CAPTURE; > >> + } > >> bool isOutput() const > >> { > >> return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | > > > > Even if you don't introduce support for V4L2_CAP_META_OUTPUT now, > > isCapture() should still return true for V4L2_CAP_META_CAPTURE to ease > > transition later. Don't forget to update the isOutput() and isCapture() > > documentation to not mention video frames anymore, but just output and > > capture. > > > > As done in v4, I introduced an isMetaCapture() method, preparing for > an isMetaOutput() one. In this way isCapture() and isOutput() would > refer to video streams only and the bufferType intialization looks > like: That's confusing. Let's have isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE and you can then also add, if desired, isMetaCapture() -> isMeta() && isCapture() ... > > * Set buffer type and wait for read notifications on CAPTURE devices > * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > */ > if (caps_.isCapture()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > : V4L2_BUF_TYPE_VIDEO_CAPTURE; > } else if (caps_.isOutput()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > bufferType_ = caps_.isMultiplanar() > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > : V4L2_BUF_TYPE_VIDEO_OUTPUT; > } else if (caps_.isMetaCapture()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > } else if (caps_.isMetaOutput()) { > fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; > } else { > LOG(V4L2, Debug) << "Device is not a supported type"; > return -EINVAL; > } > > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index 24e115554a99..8be8af7a2893 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > >> * \return True if the device can output video frames > >> */ > >> > >> +/** > >> + * \fn bool V4L2Capability::isMeta() > >> + * \brief Identify if the device is capable of providing video meta-data > >> + * > >> + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 > > > > Maybe \todo ? > > > > s/v4.20/v5.0/ > > > >> + * > >> + * \return True if the device can provide video meta-data > >> + */ > >> + > >> /** > >> * \fn bool V4L2Capability::hasStreaming() > >> * \brief Determine if the device can perform Streaming I/O > >> @@ -280,33 +289,32 @@ int V4L2Device::open() > >> << "Opened device " << caps_.bus_info() << ": " > >> << caps_.driver() << ": " << caps_.card(); > >> > >> - if (!caps_.isCapture() && !caps_.isOutput()) { > >> - LOG(V4L2, Debug) << "Device is not a supported type"; > >> - return -EINVAL; > >> - } > >> - > >> if (!caps_.hasStreaming()) { > >> LOG(V4L2, Error) << "Device does not support streaming I/O"; > >> return -EINVAL; > >> } > >> > >> - if (caps_.isCapture()) > >> + /* > >> + * Set buffer type and wait for read notifications on CAPTURE devices > >> + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > >> + */ > >> + if (caps_.isCapture()) { > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > >> - else > >> + } else if (caps_.isOutput()) { > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > >> - > >> - /* > >> - * We wait for Read notifications on CAPTURE devices (POLLIN), and > >> - * Write notifications for OUTPUT devices (POLLOUT). > >> - */ > >> - if (caps_.isCapture()) > >> + } else if (caps_.isMeta()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> - else > >> - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > >> + } else { > >> + LOG(V4L2, Debug) << "Device is not a supported type"; > >> + return -EINVAL; > >> + } > >> > >> fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > >> fdEvent_->setEnabled(false);
Hi Laurent, On Thu, Feb 28, 2019 at 07:25:49PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote: > > On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > > > On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > > >> Add support for devices that provide video meta-data to v4l2_device.cpp > > >> and re-arrange bufferType handling in open() method. > > >> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> src/libcamera/include/v4l2_device.h | 4 +++ > > >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > > >> 2 files changed, 27 insertions(+), 15 deletions(-) > > >> > > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > >> index 1d31d1b403bc..52eb6785cc15 100644 > > >> --- a/src/libcamera/include/v4l2_device.h > > >> +++ b/src/libcamera/include/v4l2_device.h > > >> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > > >> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > > >> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > [snip] > > As done in v4, I introduced an isMetaCapture() method, preparing for > > an isMetaOutput() one. In this way isCapture() and isOutput() would > > refer to video streams only and the bufferType intialization looks > > like: > > That's confusing. Let's have > > isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE_MPLANE > isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | > V4L2_CAP_VIDEO_OUTPUT_MPLANE > isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT > isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | > V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE > > and you can then also add, if desired, > > isMetaCapture() -> isMeta() && isCapture() > ... > I think the code below here, which is the only place where those functions are used would become less nice if this case. Those functions are only used to assign bufferType_ and fdEvent_, let's keep it straightforward as the below series of if/else > > > > * Set buffer type and wait for read notifications on CAPTURE devices > > * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > > */ > > if (caps_.isCapture()) { > > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > bufferType_ = caps_.isMultiplanar() > > ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > > : V4L2_BUF_TYPE_VIDEO_CAPTURE; > > } else if (caps_.isOutput()) { > > fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > bufferType_ = caps_.isMultiplanar() > > ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > : V4L2_BUF_TYPE_VIDEO_OUTPUT; > > } else if (caps_.isMetaCapture()) { > > fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > > } else if (caps_.isMetaOutput()) { > > fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; > > } else { > > LOG(V4L2, Debug) << "Device is not a supported type"; > > return -EINVAL; > > } > > > > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > >> index 24e115554a99..8be8af7a2893 100644 > > >> --- a/src/libcamera/v4l2_device.cpp > > >> +++ b/src/libcamera/v4l2_device.cpp > > >> @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) > > >> * \return True if the device can output video frames > > >> */ > > >> > > >> +/** > > >> + * \fn bool V4L2Capability::isMeta() > > >> + * \brief Identify if the device is capable of providing video meta-data > > >> + * > > >> + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 > > > > > > Maybe \todo ? > > > > > > s/v4.20/v5.0/ > > > > > >> + * > > >> + * \return True if the device can provide video meta-data > > >> + */ > > >> + > > >> /** > > >> * \fn bool V4L2Capability::hasStreaming() > > >> * \brief Determine if the device can perform Streaming I/O > > >> @@ -280,33 +289,32 @@ int V4L2Device::open() > > >> << "Opened device " << caps_.bus_info() << ": " > > >> << caps_.driver() << ": " << caps_.card(); > > >> > > >> - if (!caps_.isCapture() && !caps_.isOutput()) { > > >> - LOG(V4L2, Debug) << "Device is not a supported type"; > > >> - return -EINVAL; > > >> - } > > >> - > > >> if (!caps_.hasStreaming()) { > > >> LOG(V4L2, Error) << "Device does not support streaming I/O"; > > >> return -EINVAL; > > >> } > > >> > > >> - if (caps_.isCapture()) > > >> + /* > > >> + * Set buffer type and wait for read notifications on CAPTURE devices > > >> + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > > >> + */ > > >> + if (caps_.isCapture()) { > > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > >> bufferType_ = caps_.isMultiplanar() > > >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > > >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > > >> - else > > >> + } else if (caps_.isOutput()) { > > >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > >> bufferType_ = caps_.isMultiplanar() > > >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > > >> - > > >> - /* > > >> - * We wait for Read notifications on CAPTURE devices (POLLIN), and > > >> - * Write notifications for OUTPUT devices (POLLOUT). > > >> - */ > > >> - if (caps_.isCapture()) > > >> + } else if (caps_.isMeta()) { > > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > >> - else > > >> - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > >> + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > > >> + } else { > > >> + LOG(V4L2, Debug) << "Device is not a supported type"; > > >> + return -EINVAL; > > >> + } > > >> > > >> fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > > >> fdEvent_->setEnabled(false); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Feb 28, 2019 at 08:15:57PM +0100, Jacopo Mondi wrote: > On Thu, Feb 28, 2019 at 07:25:49PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote: > >> On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > >>> On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > >>>> Add support for devices that provide video meta-data to v4l2_device.cpp > >>>> and re-arrange bufferType handling in open() method. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> src/libcamera/include/v4l2_device.h | 4 +++ > >>>> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > >>>> 2 files changed, 27 insertions(+), 15 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >>>> index 1d31d1b403bc..52eb6785cc15 100644 > >>>> --- a/src/libcamera/include/v4l2_device.h > >>>> +++ b/src/libcamera/include/v4l2_device.h > >>>> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > >>>> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > >>>> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > [snip] > > >> As done in v4, I introduced an isMetaCapture() method, preparing for > >> an isMetaOutput() one. In this way isCapture() and isOutput() would > >> refer to video streams only and the bufferType intialization looks > >> like: > > > > That's confusing. Let's have > > > > isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | > > V4L2_CAP_VIDEO_CAPTURE_MPLANE > > isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | > > V4L2_CAP_VIDEO_OUTPUT_MPLANE > > isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT > > isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | > > V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE > > > > and you can then also add, if desired, > > > > isMetaCapture() -> isMeta() && isCapture() > > ... > > > > I think the code below here, which is the only place where those > functions are used would become less nice if this case. > > Those functions are only used to assign bufferType_ and fdEvent_, > let's keep it straightforward as the below series of if/else The code below is fine, my point is that isCapture() should not mean isVideoCapture(). bool isCapture() const { return device_caps() & (V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE); } bool isOutput() const { return device_caps() & (V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE); } bool isMeta() const { return device_caps() & (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT); } bool isVideo() const { return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE); } bool isMetaCapture() const { return isMeta() && isCapture(); } bool isMetaOutput() const { return isMeta() && isOutput(); } bool isVideoCapture() const { return isVideo() && isCapture(); } bool isVideoOutput() const { return isVideo() && isOutput(); } and then use those four last functions below. > >> > >> * Set buffer type and wait for read notifications on CAPTURE devices > >> * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > >> */ > >> if (caps_.isCapture()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > >> } else if (caps_.isOutput()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> bufferType_ = caps_.isMultiplanar() > >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > >> } else if (caps_.isMetaCapture()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >> bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > >> } else if (caps_.isMetaOutput()) { > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >> bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; > >> } else { > >> LOG(V4L2, Debug) << "Device is not a supported type"; > >> return -EINVAL; > >> } > >>
Hi Laurent, On Thu, Feb 28, 2019 at 11:45:37PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Feb 28, 2019 at 08:15:57PM +0100, Jacopo Mondi wrote: > > On Thu, Feb 28, 2019 at 07:25:49PM +0200, Laurent Pinchart wrote: > > > On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote: > > >> On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > > >>> On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > > >>>> Add support for devices that provide video meta-data to v4l2_device.cpp > > >>>> and re-arrange bufferType handling in open() method. > > >>>> > > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>>> --- > > >>>> src/libcamera/include/v4l2_device.h | 4 +++ > > >>>> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > > >>>> 2 files changed, 27 insertions(+), 15 deletions(-) > > >>>> > > >>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > >>>> index 1d31d1b403bc..52eb6785cc15 100644 > > >>>> --- a/src/libcamera/include/v4l2_device.h > > >>>> +++ b/src/libcamera/include/v4l2_device.h > > >>>> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > > >>>> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > > >>>> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > > > [snip] > > > > >> As done in v4, I introduced an isMetaCapture() method, preparing for > > >> an isMetaOutput() one. In this way isCapture() and isOutput() would > > >> refer to video streams only and the bufferType intialization looks > > >> like: > > > > > > That's confusing. Let's have > > > > > > isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | > > > V4L2_CAP_VIDEO_CAPTURE_MPLANE > > > isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | > > > V4L2_CAP_VIDEO_OUTPUT_MPLANE > > > isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT > > > isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | > > > V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE > > > > > > and you can then also add, if desired, > > > > > > isMetaCapture() -> isMeta() && isCapture() > > > ... > > > > > > > I think the code below here, which is the only place where those > > functions are used would become less nice if this case. > > > > Those functions are only used to assign bufferType_ and fdEvent_, > > let's keep it straightforward as the below series of if/else > > The code below is fine, my point is that isCapture() should not mean > isVideoCapture(). > > bool isCapture() const > { > return device_caps() & (V4L2_CAP_META_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE_MPLANE); > } > > bool isOutput() const > { > return device_caps() & (V4L2_CAP_META_OUTPUT | > V4L2_CAP_VIDEO_OUTPUT | > V4L2_CAP_VIDEO_OUTPUT_MPLANE); > } > > bool isMeta() const > { > return device_caps() & (V4L2_CAP_META_CAPTURE | > V4L2_CAP_META_OUTPUT); > } > > bool isVideo() const > { > return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_CAPTURE_MPLANE | > V4L2_CAP_VIDEO_OUTPUT | > V4L2_CAP_VIDEO_OUTPUT_MPLANE); > } > > bool isMetaCapture() const > { > return isMeta() && isCapture(); > } > > bool isMetaOutput() const > { > return isMeta() && isOutput(); > } > > bool isVideoCapture() const > { > return isVideo() && isCapture(); > } > > bool isVideoOutput() const > { > return isVideo() && isOutput(); > } > > and then use those four last functions below. I see. I don't feel like adding 4 functions and one level of indirection is super useful considering this is used in a single place, but if that would help moving forward I'll change this. > > > >> > > >> * Set buffer type and wait for read notifications on CAPTURE devices > > >> * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > > >> */ > > >> if (caps_.isCapture()) { > > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > >> bufferType_ = caps_.isMultiplanar() > > >> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > > >> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > > >> } else if (caps_.isOutput()) { > > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > >> bufferType_ = caps_.isMultiplanar() > > >> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > >> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > > >> } else if (caps_.isMetaCapture()) { > > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > > >> bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > > >> } else if (caps_.isMetaOutput()) { > > >> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > > >> bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; > > >> } else { > > >> LOG(V4L2, Debug) << "Device is not a supported type"; > > >> return -EINVAL; > > >> } > > >> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Mar 01, 2019 at 10:40:46AM +0100, Jacopo Mondi wrote: > On Thu, Feb 28, 2019 at 11:45:37PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 28, 2019 at 08:15:57PM +0100, Jacopo Mondi wrote: > >> On Thu, Feb 28, 2019 at 07:25:49PM +0200, Laurent Pinchart wrote: > >>> On Thu, Feb 28, 2019 at 09:49:53AM +0100, Jacopo Mondi wrote: > >>>> On Wed, Feb 27, 2019 at 07:54:18PM +0200, Laurent Pinchart wrote: > >>>>> On Tue, Feb 26, 2019 at 05:26:36PM +0100, Jacopo Mondi wrote: > >>>>>> Add support for devices that provide video meta-data to v4l2_device.cpp > >>>>>> and re-arrange bufferType handling in open() method. > >>>>>> > >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>> --- > >>>>>> src/libcamera/include/v4l2_device.h | 4 +++ > >>>>>> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ > >>>>>> 2 files changed, 27 insertions(+), 15 deletions(-) > >>>>>> > >>>>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >>>>>> index 1d31d1b403bc..52eb6785cc15 100644 > >>>>>> --- a/src/libcamera/include/v4l2_device.h > >>>>>> +++ b/src/libcamera/include/v4l2_device.h > >>>>>> @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { > >>>>>> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > >>>>>> V4L2_CAP_VIDEO_CAPTURE_MPLANE); > >> > >> [snip] > >> > >>>> As done in v4, I introduced an isMetaCapture() method, preparing for > >>>> an isMetaOutput() one. In this way isCapture() and isOutput() would > >>>> refer to video streams only and the bufferType intialization looks > >>>> like: > >>> > >>> That's confusing. Let's have > >>> > >>> isCapture() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_VIDEO_CAPTURE | > >>> V4L2_CAP_VIDEO_CAPTURE_MPLANE > >>> isOutput() -> V4L2_CAP_META_OUTPUT | V4L2_CAP_VIDEO_OUTPUT | > >>> V4L2_CAP_VIDEO_OUTPUT_MPLANE > >>> isMeta() -> V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT > >>> isVideo() -> V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE | > >>> V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE > >>> > >>> and you can then also add, if desired, > >>> > >>> isMetaCapture() -> isMeta() && isCapture() > >>> ... > >>> > >> > >> I think the code below here, which is the only place where those > >> functions are used would become less nice if this case. > >> > >> Those functions are only used to assign bufferType_ and fdEvent_, > >> let's keep it straightforward as the below series of if/else > > > > The code below is fine, my point is that isCapture() should not mean > > isVideoCapture(). > > > > bool isCapture() const > > { > > return device_caps() & (V4L2_CAP_META_CAPTURE | > > V4L2_CAP_VIDEO_CAPTURE | > > V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > } > > > > bool isOutput() const > > { > > return device_caps() & (V4L2_CAP_META_OUTPUT | > > V4L2_CAP_VIDEO_OUTPUT | > > V4L2_CAP_VIDEO_OUTPUT_MPLANE); > > } > > > > bool isMeta() const > > { > > return device_caps() & (V4L2_CAP_META_CAPTURE | > > V4L2_CAP_META_OUTPUT); > > } > > > > bool isVideo() const > > { > > return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | > > V4L2_CAP_VIDEO_CAPTURE_MPLANE | > > V4L2_CAP_VIDEO_OUTPUT | > > V4L2_CAP_VIDEO_OUTPUT_MPLANE); > > } > > > > bool isMetaCapture() const > > { > > return isMeta() && isCapture(); > > } > > > > bool isMetaOutput() const > > { > > return isMeta() && isOutput(); > > } > > > > bool isVideoCapture() const > > { > > return isVideo() && isCapture(); > > } > > > > bool isVideoOutput() const > > { > > return isVideo() && isOutput(); > > } > > > > and then use those four last functions below. > > I see. I don't feel like adding 4 functions and one level of > indirection is super useful considering this is > used in a single place, but if that would help moving forward I'll > change this. You can get rid of the level of indirection if you prefer (although the compiler should really optimize this), but what I really want is s/isCapture/isVideoCapture/ and s/isOutput/isVideoOutput/. > >>>> > >>>> * Set buffer type and wait for read notifications on CAPTURE devices > >>>> * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). > >>>> */ > >>>> if (caps_.isCapture()) { > >>>> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >>>> bufferType_ = caps_.isMultiplanar() > >>>> ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE > >>>> : V4L2_BUF_TYPE_VIDEO_CAPTURE; > >>>> } else if (caps_.isOutput()) { > >>>> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >>>> bufferType_ = caps_.isMultiplanar() > >>>> ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > >>>> : V4L2_BUF_TYPE_VIDEO_OUTPUT; > >>>> } else if (caps_.isMetaCapture()) { > >>>> fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); > >>>> bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; > >>>> } else if (caps_.isMetaOutput()) { > >>>> fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); > >>>> bufferType_ = V4L2_BUF_TYPE_META_OUTPUT; > >>>> } else { > >>>> LOG(V4L2, Debug) << "Device is not a supported type"; > >>>> return -EINVAL; > >>>> } > >>>>
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index 1d31d1b403bc..52eb6785cc15 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -53,6 +53,10 @@ struct V4L2Capability final : v4l2_capability { return device_caps() & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_CAPTURE_MPLANE); } + bool isMeta() const + { + return device_caps() & V4L2_CAP_META_CAPTURE; + } bool isOutput() const { return device_caps() & (V4L2_CAP_VIDEO_OUTPUT | diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 24e115554a99..8be8af7a2893 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -79,6 +79,15 @@ LOG_DEFINE_CATEGORY(V4L2) * \return True if the device can output video frames */ +/** + * \fn bool V4L2Capability::isMeta() + * \brief Identify if the device is capable of providing video meta-data + * + * FIXME: add support for META_OUTPUT, introduced in Linux v4.20 + * + * \return True if the device can provide video meta-data + */ + /** * \fn bool V4L2Capability::hasStreaming() * \brief Determine if the device can perform Streaming I/O @@ -280,33 +289,32 @@ int V4L2Device::open() << "Opened device " << caps_.bus_info() << ": " << caps_.driver() << ": " << caps_.card(); - if (!caps_.isCapture() && !caps_.isOutput()) { - LOG(V4L2, Debug) << "Device is not a supported type"; - return -EINVAL; - } - if (!caps_.hasStreaming()) { LOG(V4L2, Error) << "Device does not support streaming I/O"; return -EINVAL; } - if (caps_.isCapture()) + /* + * Set buffer type and wait for read notifications on CAPTURE devices + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT). + */ + if (caps_.isCapture()) { + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : V4L2_BUF_TYPE_VIDEO_CAPTURE; - else + } else if (caps_.isOutput()) { + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); bufferType_ = caps_.isMultiplanar() ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE : V4L2_BUF_TYPE_VIDEO_OUTPUT; - - /* - * We wait for Read notifications on CAPTURE devices (POLLIN), and - * Write notifications for OUTPUT devices (POLLOUT). - */ - if (caps_.isCapture()) + } else if (caps_.isMeta()) { fdEvent_ = new EventNotifier(fd_, EventNotifier::Read); - else - fdEvent_ = new EventNotifier(fd_, EventNotifier::Write); + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE; + } else { + LOG(V4L2, Debug) << "Device is not a supported type"; + return -EINVAL; + } fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); fdEvent_->setEnabled(false);
Add support for devices that provide video meta-data to v4l2_device.cpp and re-arrange bufferType handling in open() method. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_device.h | 4 +++ src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++------------ 2 files changed, 27 insertions(+), 15 deletions(-)