[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Add methods to get/set format

[Jacopo Mondi]

Hi Kieran,

On Tue, Jan 29, 2019 at 02:01:03PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 28/01/2019 16:07, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Mon, Jan 28, 2019 at 03:42:27PM +0000, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> I only have variable/layout concerns here - which is certainly not a
> >> blocker at the moment.
> >>
> >> Also - if the outcome of the discussion below changes this patches
> >> expectations - then we'll need to adapt other patches too - so that
> >> would be a global patch fix at that point.
> >>
> >> Thus I don't think that discussion blocks this patch.
> >>
> >>
> >> On 28/01/2019 15:11, Jacopo Mondi wrote:
> >>> Add methods to set and get the image format programmed on a V4L2Device
> >>> for both the single and multi planar use case.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >>
> >>> ---
> >>>  src/libcamera/include/v4l2_device.h |  10 +++
> >>>  src/libcamera/v4l2_device.cpp       | 128 ++++++++++++++++++++++++++++
> >>>  2 files changed, 138 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >>> index c70959e..fe54220 100644
> >>> --- a/src/libcamera/include/v4l2_device.h
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -86,10 +86,20 @@ public:
> >>>  	const char *deviceName() const { return caps_.card(); }
> >>>  	const char *busName() const { return caps_.bus_info(); }
> >>>
> >>> +	int format(V4L2DeviceFormat *fmt);
> >>> +	int setFormat(V4L2DeviceFormat *fmt);
> >>> +
> >>>  private:
> >>>  	std::string deviceNode_;
> >>>  	int fd_;
> >>>  	V4L2Capability caps_;
> >>> +	enum v4l2_buf_type bufferType_;
> >>> +
> >>> +	int getFormatSingleplane(V4L2DeviceFormat *fmt);
> >>> +	int setFormatSingleplane(V4L2DeviceFormat *fmt);
> >>> +
> >>> +	int getFormatMultiplane(V4L2DeviceFormat *fmt);
> >>> +	int setFormatMultiplane(V4L2DeviceFormat *fmt);
> >>
> >>
> >> Can I confirm <or prompt discussion> on our class layouts?
> >>
> >> Have we defined a layout anywhere?
> >>
> >> Here this creates:
> >>
> >>
> >> class ClassName
> >> {
> >> public:
> >> 	ClassName();
> >> 	~ClassName();
> >> 	void publicFunctions();
> >>
> >> 	int publicData;
> >> 	bool publicBool;
> >>
> >> private:
> >> 	int privateData;
> >> 	bool privateBool;
> >>
> >> 	int privateFunctions();
> >> 	int morePrivateFunctions();
> >> }
> >>
> >> Which is:
> >> 	PublicFunctions
> >> 	PublicData
> >> 	PrivateData
> >> 	PrivateFunctions
> >>
> >>
> >> So the 'data' both public and private is sandwiched in the middle.
> >> I don't mind this - but in my mind I thought we were doing:
> >>
> >>
> >> class ClassName
> >> {
> >> public:
> >> 	ClassName();
> >> 	~ClassName();
> >> 	void publicFunctions();
> >>
> >> 	int publicData;
> >> 	bool publicBool;
> >>
> >> private:
> >> 	int privateFunctions();
> >> 	int morePrivateFunctions();
> >>
> >> 	int privateData;
> >> 	bool privateBool;
> >> }
> >>
> >> Which is:
> >> 	PublicFunctions
> >> 	PublicData
> >> 	PrivateFunctions
> >> 	PrivateData
> >>
> >> so that the public, and private (or protected) sections follow the same
> >> sequence?
> >>
> >> Any comments from the team here?
> >
> > Quoting the Google's C++ style guide (which I refer to because we
> > actually started from there when we defined our coding guidelines):
> > https://google.github.io/styleguide/cppguide.html#Declaration_Order
> >
> > ... generally prefer the following order: types (including typedef, using,
> > and nested structs and classes), constants, factory functions, constructors,
> > assignment operators, destructor, all other methods, data members.
> >
> > So your
> >  	PublicFunctions
> >  	PublicData
> >  	PrivateFunctions
> >  	PrivateData
> >
> > Is accurate.
> >
> > I fear we would need a library-wide cleanup, but I can start by making
> > this right at least.
> >
>
> Aha - great - something was right in my head :)
>
> I'd be happy with a push to master with this ordering corrected.
>

Thanks, fixed and pushed!

Thanks
  j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190129/02b50338/attachment.sig>