[libcamera-devel,v2,0/4] Add MediaDevice and associated MediaObject
mbox series

Message ID 20181224153721.8359-1-jacopo@jmondi.org
Headers show
Series
  • Add MediaDevice and associated MediaObject
Related show

Message

Jacopo Mondi Dec. 24, 2018, 3:37 p.m. UTC 
Hello,
   this is v2 of MediaDevice and friends.

v1 available for reference here:
https://lists.libcamera.org/pipermail/libcamera-devel/2018-December/000120.html

While reviewing Kieran's and Niklas' patches I noticed some comments I made
on their series actually applied on my v1, so here it is the outcome of
a small private self-review. I'm sure I have missed something anyhow...

It's mostly about using 'const' a lot more for values returned by reference
by getter functions, a more extensive use of 'auto' keyboard to shorten
iterators declarations and a few cosmetic changes.

Both branches (v1 and v2) are available for comparison
(jmondi/master/media_device_v1 and jmondi/master/media_device_v2) and a diff
is collected here: https://paste.debian.net/1057251/

On top of the first two patches that were included in v1, I have added what
I proposed in the review of Niklas' series: a MediaDeviceDesc object and a
match function to be added to the MediaDevice. Niklas, what do you think
about this? Does this help clarifying the proposal I made in the review
comments? (those patches are now broken out to eases discussion and review,
but will be ideally squashed on the [02/04] later).

I have some private tests for IPU3, and the matching code looks like the
following:

	MediaDeviceDesc cio2_desc("ipu3-cio2");
	cio2_desc.addEntity("ipu3-csi2 0");
	cio2_desc.addEntity("ipu3-cio2 0");
	cio2_desc.addEntity("ipu3-csi2 1");
	cio2_desc.addEntity("ipu3-cio2 1");
	cio2_desc.addEntity("ipu3-csi2 2");
	cio2_desc.addEntity("ipu3-cio2 2");
	cio2_desc.addEntity("ipu3-csi2 3");
	cio2_desc.addEntity("ipu3-cio2 3");
	cio2_desc.addEntity("ov13858 2-0010");
	cio2_desc.addEntity("ov5670 4-0036");

	bool match = cio2.match(cio2_desc);
	if (match) {
		LOG(Info) << "CIO2 matched";
	} else {
		LOG(Error) << "Cannot match CIO2 with the provided desc";
		return -1;
	}

If we adopt something like this, in place of cio2.match() we would have a
MediaEnumerator, as proposed by Niklas, but the idea stays the same: is the
MediaDevice that matches the MediaDeviceDesc, and the MediaEnumerator receives
a MediaDeviceDesc and tries it on all its open MediaDevices.

Thanks
   j

v1 -> v2:
- diff collected here: https://paste.debian.net/1057251/
- patches 3 and 4 are new, broken out for review, to be squashed later.

Jacopo Mondi (4):
  libcamera: Add MediaObject class hierarchy
  libcamera: Add MediaDevice class
  libcamera: media device: Add MediaDeviceDesc
  libcamera: media device: Add 'match()' method

 src/libcamera/include/media_device.h | 115 +++++
 src/libcamera/include/media_object.h | 117 +++++
 src/libcamera/media_device.cpp       | 615 +++++++++++++++++++++++++++
 src/libcamera/media_object.cpp       | 302 +++++++++++++
 src/libcamera/meson.build            |   2 +
 5 files changed, 1151 insertions(+)
 create mode 100644 src/libcamera/include/media_device.h
 create mode 100644 src/libcamera/include/media_object.h
 create mode 100644 src/libcamera/media_device.cpp
 create mode 100644 src/libcamera/media_object.cpp

--
2.20.1

Comments

Niklas Söderlund Dec. 28, 2018, 3:35 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2018-12-24 16:37:17 +0100, Jacopo Mondi wrote:
> Hello,
>    this is v2 of MediaDevice and friends.
> 
> v1 available for reference here:
> https://lists.libcamera.org/pipermail/libcamera-devel/2018-December/000120.html
> 
> While reviewing Kieran's and Niklas' patches I noticed some comments I made
> on their series actually applied on my v1, so here it is the outcome of
> a small private self-review. I'm sure I have missed something anyhow...
> 
> It's mostly about using 'const' a lot more for values returned by reference
> by getter functions, a more extensive use of 'auto' keyboard to shorten
> iterators declarations and a few cosmetic changes.
> 
> Both branches (v1 and v2) are available for comparison
> (jmondi/master/media_device_v1 and jmondi/master/media_device_v2) and a diff
> is collected here: https://paste.debian.net/1057251/
> 
> On top of the first two patches that were included in v1, I have added what
> I proposed in the review of Niklas' series: a MediaDeviceDesc object and a
> match function to be added to the MediaDevice. Niklas, what do you think
> about this? Does this help clarifying the proposal I made in the review
> comments? (those patches are now broken out to eases discussion and review,
> but will be ideally squashed on the [02/04] later).
> 
> I have some private tests for IPU3, and the matching code looks like the
> following:
> 
> 	MediaDeviceDesc cio2_desc("ipu3-cio2");
> 	cio2_desc.addEntity("ipu3-csi2 0");
> 	cio2_desc.addEntity("ipu3-cio2 0");
> 	cio2_desc.addEntity("ipu3-csi2 1");
> 	cio2_desc.addEntity("ipu3-cio2 1");
> 	cio2_desc.addEntity("ipu3-csi2 2");
> 	cio2_desc.addEntity("ipu3-cio2 2");
> 	cio2_desc.addEntity("ipu3-csi2 3");
> 	cio2_desc.addEntity("ipu3-cio2 3");
> 	cio2_desc.addEntity("ov13858 2-0010");
> 	cio2_desc.addEntity("ov5670 4-0036");
> 
> 	bool match = cio2.match(cio2_desc);
> 	if (match) {
> 		LOG(Info) << "CIO2 matched";
> 	} else {
> 		LOG(Error) << "Cannot match CIO2 with the provided desc";
> 		return -1;
> 	}
> 
> If we adopt something like this, in place of cio2.match() we would have a
> MediaEnumerator, as proposed by Niklas, but the idea stays the same: is the
> MediaDevice that matches the MediaDeviceDesc, and the MediaEnumerator receives
> a MediaDeviceDesc and tries it on all its open MediaDevices.
> 
> Thanks
>    j
> 
> v1 -> v2:
> - diff collected here: https://paste.debian.net/1057251/
> - patches 3 and 4 are new, broken out for review, to be squashed later.
> 
> Jacopo Mondi (4):
>   libcamera: Add MediaObject class hierarchy
>   libcamera: Add MediaDevice class
>   libcamera: media device: Add MediaDeviceDesc
>   libcamera: media device: Add 'match()' method

Maybe my client is misbehaving but I'm missing all the patches in this 
series except 00/04.

> 
>  src/libcamera/include/media_device.h | 115 +++++
>  src/libcamera/include/media_object.h | 117 +++++
>  src/libcamera/media_device.cpp       | 615 +++++++++++++++++++++++++++
>  src/libcamera/media_object.cpp       | 302 +++++++++++++
>  src/libcamera/meson.build            |   2 +
>  5 files changed, 1151 insertions(+)
>  create mode 100644 src/libcamera/include/media_device.h
>  create mode 100644 src/libcamera/include/media_object.h
>  create mode 100644 src/libcamera/media_device.cpp
>  create mode 100644 src/libcamera/media_object.cpp
> 
> --
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Dec. 28, 2018, 7:55 a.m. UTC | #2 
Hi Niklas,

On Fri, Dec 28, 2018 at 04:35:08AM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2018-12-24 16:37:17 +0100, Jacopo Mondi wrote:
> > Hello,
> >    this is v2 of MediaDevice and friends.
> >
> > v1 available for reference here:
> > https://lists.libcamera.org/pipermail/libcamera-devel/2018-December/000120.html
> >
> > While reviewing Kieran's and Niklas' patches I noticed some comments I made
> > on their series actually applied on my v1, so here it is the outcome of
> > a small private self-review. I'm sure I have missed something anyhow...
> >
> > It's mostly about using 'const' a lot more for values returned by reference
> > by getter functions, a more extensive use of 'auto' keyboard to shorten
> > iterators declarations and a few cosmetic changes.
> >
> > Both branches (v1 and v2) are available for comparison
> > (jmondi/master/media_device_v1 and jmondi/master/media_device_v2) and a diff
> > is collected here: https://paste.debian.net/1057251/
> >
> > On top of the first two patches that were included in v1, I have added what
> > I proposed in the review of Niklas' series: a MediaDeviceDesc object and a
> > match function to be added to the MediaDevice. Niklas, what do you think
> > about this? Does this help clarifying the proposal I made in the review
> > comments? (those patches are now broken out to eases discussion and review,
> > but will be ideally squashed on the [02/04] later).
> >
> > I have some private tests for IPU3, and the matching code looks like the
> > following:
> >
> > 	MediaDeviceDesc cio2_desc("ipu3-cio2");
> > 	cio2_desc.addEntity("ipu3-csi2 0");
> > 	cio2_desc.addEntity("ipu3-cio2 0");
> > 	cio2_desc.addEntity("ipu3-csi2 1");
> > 	cio2_desc.addEntity("ipu3-cio2 1");
> > 	cio2_desc.addEntity("ipu3-csi2 2");
> > 	cio2_desc.addEntity("ipu3-cio2 2");
> > 	cio2_desc.addEntity("ipu3-csi2 3");
> > 	cio2_desc.addEntity("ipu3-cio2 3");
> > 	cio2_desc.addEntity("ov13858 2-0010");
> > 	cio2_desc.addEntity("ov5670 4-0036");
> >
> > 	bool match = cio2.match(cio2_desc);
> > 	if (match) {
> > 		LOG(Info) << "CIO2 matched";
> > 	} else {
> > 		LOG(Error) << "Cannot match CIO2 with the provided desc";
> > 		return -1;
> > 	}
> >
> > If we adopt something like this, in place of cio2.match() we would have a
> > MediaEnumerator, as proposed by Niklas, but the idea stays the same: is the
> > MediaDevice that matches the MediaDeviceDesc, and the MediaEnumerator receives
> > a MediaDeviceDesc and tries it on all its open MediaDevices.
> >
> > Thanks
> >    j
> >
> > v1 -> v2:
> > - diff collected here: https://paste.debian.net/1057251/
> > - patches 3 and 4 are new, broken out for review, to be squashed later.
> >
> > Jacopo Mondi (4):
> >   libcamera: Add MediaObject class hierarchy
> >   libcamera: Add MediaDevice class
> >   libcamera: media device: Add MediaDeviceDesc
> >   libcamera: media device: Add 'match()' method
>
> Maybe my client is misbehaving but I'm missing all the patches in this
> series except 00/04.
>

Oh damn, I actually just sent [00/04]
Sorry about this.

I'll re-send now.

> >
> >  src/libcamera/include/media_device.h | 115 +++++
> >  src/libcamera/include/media_object.h | 117 +++++
> >  src/libcamera/media_device.cpp       | 615 +++++++++++++++++++++++++++
> >  src/libcamera/media_object.cpp       | 302 +++++++++++++
> >  src/libcamera/meson.build            |   2 +
> >  5 files changed, 1151 insertions(+)
> >  create mode 100644 src/libcamera/include/media_device.h
> >  create mode 100644 src/libcamera/include/media_object.h
> >  create mode 100644 src/libcamera/media_device.cpp
> >  create mode 100644 src/libcamera/media_object.cpp
> >
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund