| Message ID | 20190121172705.19985-4-jacopo@jmondi.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Jan 21, 2019 at 06:27:02PM +0100, Jacopo Mondi wrote: > Add functions to V4L2Capability to identify if a V4L2 device supports > single or multiplane APIs. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 6 +++-- > src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++++++++++---- > 2 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 2787847..0101a4b 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability { > return reinterpret_cast<const char *>(v4l2_capability::bus_info); > } > > - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } > - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } > + bool isSinglePlane() const; > + bool isMultiPlane() const; Maybe s/Plane/Planar/ ? > + bool isCapture() const; > + bool isOutput() const; Is there a reason to stop inlining the functions ? They should just generate a load and an and operation, which should be less costly than a function call. > bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } > }; > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index aef0996..7cd89d7 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -47,17 +47,46 @@ namespace libcamera { > * \return The string containing the device location > */ > > + > +/** > + * \brief Identify if the device implements V4L2 single plane APIs s/implements/implements the/ s/plane/planar/ ? and for the following line too. > + * \return true if the device supports single plane APIs Don't we start all the return descriptions with a capital letter ? Those comments hold for the other functions below. > + */ > +bool V4L2Capability::isSinglePlane() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT); You could write this return capabilities & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT); I would expect the compiler to perform that optimization itself, but it also doesn't hurt to be explicit. > +} > + > +/** > + * \brief Identify if the device implements V4L2 multiplanar APIs > + * \return true if the device supports multiplanar APIs > + */ > +bool V4L2Capability::isMultiPlane() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > +} > + > /** > - * \fn bool V4L2Capability::isCapture() > * \brief Identify if the device is capable of capturing video > - * \return True if the device can capture video frames > + * \return true if the device can capture video frames > */ > +bool V4L2Capability::isCapture() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > + (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE); > +} > > /** > - * \fn bool V4L2Capability::isOutput() > * \brief Identify if the device is capable of outputting video > - * \return True if the device can output video frames > + * \return true if the device can capture video frames > */ > +bool V4L2Capability::isOutput() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_OUTPUT) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > +} > > /** > * \fn bool V4L2Capability::hasStreaming()
Hi Jacopo, Thanks for your work. On 2019-01-21 18:27:02 +0100, Jacopo Mondi wrote: > Add functions to V4L2Capability to identify if a V4L2 device supports > single or multiplane APIs. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 6 +++-- > src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++++++++++---- > 2 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 2787847..0101a4b 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability { > return reinterpret_cast<const char *>(v4l2_capability::bus_info); > } > > - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } > - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } > + bool isSinglePlane() const; > + bool isMultiPlane() const; > + bool isCapture() const; > + bool isOutput() const; > bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } > }; > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index aef0996..7cd89d7 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -47,17 +47,46 @@ namespace libcamera { > * \return The string containing the device location > */ > > + > +/** > + * \brief Identify if the device implements V4L2 single plane APIs > + * \return true if the device supports single plane APIs > + */ > +bool V4L2Capability::isSinglePlane() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT); > +} > + > +/** > + * \brief Identify if the device implements V4L2 multiplanar APIs > + * \return true if the device supports multiplanar APIs > + */ > +bool V4L2Capability::isMultiPlane() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > +} > + > /** > - * \fn bool V4L2Capability::isCapture() > * \brief Identify if the device is capable of capturing video > - * \return True if the device can capture video frames > + * \return true if the device can capture video frames > */ > +bool V4L2Capability::isCapture() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > + (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE); > +} > > /** > - * \fn bool V4L2Capability::isOutput() > * \brief Identify if the device is capable of outputting video > - * \return True if the device can output video frames > + * \return true if the device can capture video frames This looks odd, s/capture/output/ right? > */ > +bool V4L2Capability::isOutput() const > +{ > + return (capabilities & V4L2_CAP_VIDEO_OUTPUT) || > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > +} > > /** > * \fn bool V4L2Capability::hasStreaming() > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Mon, Jan 21, 2019 at 10:31:59PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Jan 21, 2019 at 06:27:02PM +0100, Jacopo Mondi wrote: > > Add functions to V4L2Capability to identify if a V4L2 device supports > > single or multiplane APIs. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_device.h | 6 +++-- > > src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++++++++++---- > > 2 files changed, 37 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index 2787847..0101a4b 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability { > > return reinterpret_cast<const char *>(v4l2_capability::bus_info); > > } > > > > - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } > > - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } > > + bool isSinglePlane() const; > > + bool isMultiPlane() const; > > Maybe s/Plane/Planar/ ? > Ok > > + bool isCapture() const; > > + bool isOutput() const; > > Is there a reason to stop inlining the functions ? They should just > generate a load and an and operation, which should be less costly than a > function call. > I moved them in the cpp unit, then realized I could have moved them back but I thought it was preferred to have them in the cpp file, as everytime I suggested inlining something it got pushed back. I'll move them back. > > bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } > > }; > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index aef0996..7cd89d7 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -47,17 +47,46 @@ namespace libcamera { > > * \return The string containing the device location > > */ > > > > + > > +/** > > + * \brief Identify if the device implements V4L2 single plane APIs > > s/implements/implements the/ > s/plane/planar/ ? > > and for the following line too. > > > + * \return true if the device supports single plane APIs > > Don't we start all the return descriptions with a capital letter ? > > Those comments hold for the other functions below. I had looked around and the boolean 'true' is usually spelled with a lower case 't'. > > > + */ > > +bool V4L2Capability::isSinglePlane() const > > +{ > > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > > + (capabilities & V4L2_CAP_VIDEO_OUTPUT); > > You could write this > > return capabilities & (V4L2_CAP_VIDEO_CAPTURE | > V4L2_CAP_VIDEO_OUTPUT); > > I would expect the compiler to perform that optimization itself, but it > also doesn't hurt to be explicit. > ok Thanks j > > +} > > + > > +/** > > + * \brief Identify if the device implements V4L2 multiplanar APIs > > + * \return true if the device supports multiplanar APIs > > + */ > > +bool V4L2Capability::isMultiPlane() const > > +{ > > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) || > > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > > +} > > + > > /** > > - * \fn bool V4L2Capability::isCapture() > > * \brief Identify if the device is capable of capturing video > > - * \return True if the device can capture video frames > > + * \return true if the device can capture video frames > > */ > > +bool V4L2Capability::isCapture() const > > +{ > > + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > > + (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE); > > +} > > > > /** > > - * \fn bool V4L2Capability::isOutput() > > * \brief Identify if the device is capable of outputting video > > - * \return True if the device can output video frames > > + * \return true if the device can capture video frames > > */ > > +bool V4L2Capability::isOutput() const > > +{ > > + return (capabilities & V4L2_CAP_VIDEO_OUTPUT) || > > + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > > +} > > > > /** > > * \fn bool V4L2Capability::hasStreaming() > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Jan 22, 2019 at 02:44:16PM +0100, Jacopo Mondi wrote: > On Mon, Jan 21, 2019 at 10:31:59PM +0200, Laurent Pinchart wrote: > > On Mon, Jan 21, 2019 at 06:27:02PM +0100, Jacopo Mondi wrote: > >> Add functions to V4L2Capability to identify if a V4L2 device supports > >> single or multiplane APIs. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/include/v4l2_device.h | 6 +++-- > >> src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++++++++++---- > >> 2 files changed, 37 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >> index 2787847..0101a4b 100644 > >> --- a/src/libcamera/include/v4l2_device.h > >> +++ b/src/libcamera/include/v4l2_device.h > >> @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability { > >> return reinterpret_cast<const char *>(v4l2_capability::bus_info); > >> } > >> > >> - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } > >> - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } > >> + bool isSinglePlane() const; > >> + bool isMultiPlane() const; > > > > Maybe s/Plane/Planar/ ? > > Ok > > >> + bool isCapture() const; > >> + bool isOutput() const; > > > > Is there a reason to stop inlining the functions ? They should just > > generate a load and an and operation, which should be less costly than a > > function call. > > > > I moved them in the cpp unit, then realized I could have moved them back > but I thought it was preferred to have them in the cpp file, as > everytime I suggested inlining something it got pushed back. Inlines should be used with caution as they can generate a fair amount of code in C++, for instance when they copy full objects behind the scene, or for virtual destructors. In this specific case I think they're fine. > I'll move them back. > > >> bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } > >> }; > >> > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index aef0996..7cd89d7 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -47,17 +47,46 @@ namespace libcamera { > >> * \return The string containing the device location > >> */ > >> > >> + > >> +/** > >> + * \brief Identify if the device implements V4L2 single plane APIs > > > > s/implements/implements the/ > > s/plane/planar/ ? > > > > and for the following line too. > > > >> + * \return true if the device supports single plane APIs > > > > Don't we start all the return descriptions with a capital letter ? > > > > Those comments hold for the other functions below. > > I had looked around and the boolean 'true' is usually spelled with a > lower case 't'. 6 cases of \return true, and 6 cases of \return True :-) I'd go for the later to capitalize all \return expressions. > >> + */ > >> +bool V4L2Capability::isSinglePlane() const > >> +{ > >> + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > >> + (capabilities & V4L2_CAP_VIDEO_OUTPUT); > > > > You could write this > > > > return capabilities & (V4L2_CAP_VIDEO_CAPTURE | > > V4L2_CAP_VIDEO_OUTPUT); > > > > I would expect the compiler to perform that optimization itself, but it > > also doesn't hurt to be explicit. > > ok > > >> +} > >> + > >> +/** > >> + * \brief Identify if the device implements V4L2 multiplanar APIs > >> + * \return true if the device supports multiplanar APIs > >> + */ > >> +bool V4L2Capability::isMultiPlane() const > >> +{ > >> + return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) || > >> + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > >> +} > >> + > >> /** > >> - * \fn bool V4L2Capability::isCapture() > >> * \brief Identify if the device is capable of capturing video > >> - * \return True if the device can capture video frames > >> + * \return true if the device can capture video frames > >> */ > >> +bool V4L2Capability::isCapture() const > >> +{ > >> + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || > >> + (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE); > >> +} > >> > >> /** > >> - * \fn bool V4L2Capability::isOutput() > >> * \brief Identify if the device is capable of outputting video > >> - * \return True if the device can output video frames > >> + * \return true if the device can capture video frames > >> */ > >> +bool V4L2Capability::isOutput() const > >> +{ > >> + return (capabilities & V4L2_CAP_VIDEO_OUTPUT) || > >> + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); > >> +} > >> > >> /** > >> * \fn bool V4L2Capability::hasStreaming()
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index 2787847..0101a4b 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -27,8 +27,10 @@ struct V4L2Capability final : v4l2_capability { return reinterpret_cast<const char *>(v4l2_capability::bus_info); } - bool isCapture() const { return capabilities & V4L2_CAP_VIDEO_CAPTURE; } - bool isOutput() const { return capabilities & V4L2_CAP_VIDEO_OUTPUT; } + bool isSinglePlane() const; + bool isMultiPlane() const; + bool isCapture() const; + bool isOutput() const; bool hasStreaming() const { return capabilities & V4L2_CAP_STREAMING; } }; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index aef0996..7cd89d7 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -47,17 +47,46 @@ namespace libcamera { * \return The string containing the device location */ + +/** + * \brief Identify if the device implements V4L2 single plane APIs + * \return true if the device supports single plane APIs + */ +bool V4L2Capability::isSinglePlane() const +{ + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || + (capabilities & V4L2_CAP_VIDEO_OUTPUT); +} + +/** + * \brief Identify if the device implements V4L2 multiplanar APIs + * \return true if the device supports multiplanar APIs + */ +bool V4L2Capability::isMultiPlane() const +{ + return (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE) || + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); +} + /** - * \fn bool V4L2Capability::isCapture() * \brief Identify if the device is capable of capturing video - * \return True if the device can capture video frames + * \return true if the device can capture video frames */ +bool V4L2Capability::isCapture() const +{ + return (capabilities & V4L2_CAP_VIDEO_CAPTURE) || + (capabilities & V4L2_CAP_VIDEO_CAPTURE_MPLANE); +} /** - * \fn bool V4L2Capability::isOutput() * \brief Identify if the device is capable of outputting video - * \return True if the device can output video frames + * \return true if the device can capture video frames */ +bool V4L2Capability::isOutput() const +{ + return (capabilities & V4L2_CAP_VIDEO_OUTPUT) || + (capabilities & V4L2_CAP_VIDEO_OUTPUT_MPLANE); +} /** * \fn bool V4L2Capability::hasStreaming()
Add functions to V4L2Capability to identify if a V4L2 device supports single or multiplane APIs. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_device.h | 6 +++-- src/libcamera/v4l2_device.cpp | 37 +++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 6 deletions(-)