| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | mchehab has quit IRC (Remote host closed the connection)
ChanServ sets mode: +v mchehab | [07:22] |
| .................... (idle for 1h38mn) | ||
| hverkuil | pinchartl: I'm available to discuss ENUM_FMT (except from 12:15-13:30). I replied to the ML yesterday.
mchehab: if we can't get to an agreement on the documentation changes quickly, is it possible to merge the PR with the understanding that there will be a follow-up patch to update the documentation once we agree? There is at least one other series that depends on this, and I would really like to get these changes in for v5.8. It's good stuff. | [09:02] |
| ....... (idle for 31mn) | ||
| mchehab | I'm available too | [09:36] |
| pinchartl: yes
pinchartl: when do you want to discuss about ENUM_FMT? | [09:45] | |
| pinchartl | mchehab: I'm available now | [09:47] |
| hverkuil | me too, but only for 20 minutes. | [09:48] |
| pinchartl | hverkuil: thanks for the e-mail reply
mchehab: Sakari, Hans and I are more of less in agreement here. should I submit a new version with the minor changes that were requested, or is that still not acceptable for you ? | [09:48] |
| mchehab | I liked Sakari's proposition of calling it "video node centric" x "MC-centric"
also, once we reach an agreement, I'll rebase the glossary patchset for us to set this into a stone | [09:51] |
| hverkuil | Oh wow, we agree on something! Champagne! | [09:52] |
| pinchartl | and how about the fact that we focus on the flag instead of the explanation ?
is that enough ? "If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` capability (also known as a 'video node centric' driver) ..." | [09:52] |
| mchehab | works for me too
I would still break it on different paragraphs sticking lots of information on one paragraph seem to imply that the next phrase depends on the first one, with is not the case there | [09:52] |
| hverkuil | I think it is two paragraphs already, if I read the patch right. | [09:54] |
| mchehab | there's one pagragraph for video node centric, with lots of unrelated things, and another one for MC-centric | [09:54] |
| pinchartl | one for V4L2_CAP_IO_MC, one for !V4L2_CAP_IO_MC
they're not unrelated, they describe what happens in each of the two cases | [09:54] |
| mchehab | they are unrelated:
+If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability +<device-capabilities>`, applications may initialize the ``mbus_code`` field to +a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the +``mbus_code`` field is not zero, drivers shall restrict enumeration to only the +image formats that can produce (for video output devices) or be produced from +(for video capture devices) that media bus code. Regardless of the value of +the ``mbus_code`` field, the enumerated image formats shall not depend on the +active configuration of the video device or device pipeline. Enumeration shall +otherwise operate as previously described. that's why you had to add a "Regardless", for example, on the above paragraph I would place this on a list, like: | [09:56] |
| pinchartl | no, that sentence is only applicable to the V4L2_CAP_IO_MC case | [09:57] |
| mchehab | 1) If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` capability (also known as a 'video node centric' driver) | [09:57] |
| pinchartl | in !V4L2_CAP_IO_MC, the enumerated formats depend on the configuration of the pipeline | [09:57] |
| mchehab | then place each unrelated info there on a separate paragraph.
So, the last one would be: "After switching to another input or output the list of enumerated formats may be different." I would even keep the .. note: | [09:57] |
| pinchartl | mchehab: how about this
you rewrite the text any way you want | [09:58] |
| mchehab | markup for that, as this is something we want to bold | [09:58] |
| pinchartl | I don't care
I need the API extension, I don't care about the doc | [09:58] |
| mchehab | well, I can do that, but this won't prevent having some discussion about that
if my text won't be ok for someone | [10:00] |
| pinchartl | I won't be part of that discussion, so it will be easier to get it merged as you'll be the one in charge | [10:00] |
| hverkuil | I think the numerated list is not a bad idea to improve readability. mchehab: I'll review your proposal, I think we pretty much agree on the text, it is more about typographical issues to improve readability. | [10:01] |
| mchehab | hverkuil: OK | [10:01] |
| pinchartl | just make sure it gets in v5.
v5.8 | [10:01] |
| hverkuil | Yes :-) | [10:01] |
| mchehab | sure, there's nothing preventing it for 5.8 | [10:02] |
| pinchartl | please don't keep my authorship of the patch though
you can attribute it to yourself | [10:03] |
| mchehab | pinchartl: I'll apply your patch as-is, with your SOB and another patch on the top of it with the canges
changes | [10:04] |
| pinchartl | that works for me | [10:04] |
| .... (idle for 15mn) | ||
| mchehab | patch sent
feel free to review it basically splits application-facing info on one paragraph and driver-facing info on another paragraph. I opted to keep the ".. note" markup | [10:19] |
| pinchartl | mchehab: as I said I won't review it. I won't not even read it, as there's a high risk I would be tempted to review it :-) | [10:21] |
| mchehab | :-)
I don't mind if you review it... it is up to you, really | [10:21] |
| pinchartl | no, I'm serious | [10:22] |
| mchehab | ok
whatever works for you | [10:22] |
| pinchartl | I'm more annoyed about the V4L2_CID_LOCATION control
that's one we also need and it looks like an agreement on that won't be possible | [10:22] |
| mchehab | well, let's discuss it | [10:23] |
| pinchartl | I've read the mail thread, I don't see a point in discussing it frankly
all the arguments have been made, I have nothing to add | [10:24] |
| mchehab | an alternative that would work for me is to make this a device-specific control | [10:24] |
| pinchartl | it needs to be generic
it needs a generic DT property too | [10:24] |
| mchehab | so, we need to make it more generic than accepting just two mount angles | [10:25] |
| pinchartl | and it would be insane to parse it manually in all sensor drivers | [10:25] |
| mchehab | two mount angles apply only to small devices like cell phones
if sensors are mounted on something like a car, the mount angle could be different than 0 or 180 degrees | [10:26] |
| pinchartl | it would help if you did some research beforehand. the car example is completely bogus | [10:26] |
| mchehab | why? | [10:27] |
| pinchartl | because the location of cameras won't be expressed in DT in that case. there's way too much information and precision that is required, it will be fully handled in userspace
plus it won't be just the angle that is needed, but precise location information precise information about the optics and cameras can be mounted on movable parts it's out of scope fo V4L2_CID_LOCATION s/fo/for/ | [10:28] |
| mchehab | that's exactly my point: V4L2_CID_LOCATION doesn't provide location
it may address a current need for one specific usecase, but it is not generic enough | [10:30] |
| pinchartl | it provides location in a limited context, for a set of use cases where it's applicable. it doesn't pretend to solve wolrd hunger or make sandwiches | [10:31] |
| mchehab | I don't mind merging a location control, or naming it with something else that would point that this has a very limited scope | [10:31] |
| pinchartl | I could go with a rename | [10:31] |
| mchehab | ok | [10:32] |
| pinchartl | what name would be acceptable for you ? | [10:32] |
| mchehab | feel free to propose one. on my last e-mail, I proposed V4L2_CID_CAMERA_VIEWING_ANGLE, being "0" for front, "180" for back and allowing the usage of different angles in the future, in case someone would need it | [10:34] |
| pinchartl | viewing angle sounds more like field of view, doesn't it ? | [10:34] |
| mchehab | field of view works equally well for me | [10:34] |
| pinchartl | I mean that field of view is something entirely different :-) | [10:35] |
| mchehab | or whatever other name that would have a similar meaning
ah, yeah, sorry... | [10:35] |
| pinchartl | we know we will have needs for more precise position information
as Hans mentioned, with stereo cameras, knowing the exact position is crucial | [10:36] |
| mchehab | yeah, it should be different than "field of view" | [10:36] |
| pinchartl | it it usually then expressed as a 2D displacement from an origin
and a rotation matrix and I foresee possible needs for that in the future | [10:36] |
| mchehab | yeah, let's reserve "LOCATION" for when we need those | [10:37] |
| pinchartl | however, for devices that have a single camera sensor in each location (front and back for instance, and thus no need for relative positioning of two stereo sensors), this is overkill | [10:37] |
| mchehab | fully agreed | [10:37] |
| pinchartl | "location" won't be a good name then
you will need two controls one for the translation vector, and one for the rotation matrix location is a simpler alternative, for the vast majority of devices that don't need something more complex | [10:37] |
| mchehab | CID_ROTATION_MATRIX doesn't sound a good name, as for now we're measuring it only on 2D (and not 3D)
on the other hand, it won't hurt to define it in 3D, as one dimention will always be 0 in the case of a mobile phone (with is what I guess you're trying to map) | [10:39] |
| pinchartl | mchehab: https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.poseRotation
https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.poseTranslation we'll eventually need those | [10:39] |
| mchehab | hmm... LENS_FACING is not a bad name | [10:40] |
| pinchartl | https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.facing | [10:40] |
| mchehab | CID_LENS_FACING would work for me | [10:40] |
| pinchartl | ok. I'll ask Jacopo to rename it. just double-checking, is the scope then fine, only the name needs to be changed ? | [10:41] |
| mchehab | android definition is a little obsolete, as there are some devices with back screens too ;-)
pinchartl: there's actually the need of a second change: this control can be "R/W" if the sensor is movable | [10:41] |
| pinchartl | we have no experience with movable sensors
I'd rather define it as read-only first | [10:42] |
| mchehab | some devices now have a flipping sensor | [10:42] |
| pinchartl | and when we'll need to support such devices, we'll decide how to proceed | [10:42] |
| mchehab | works for me | [10:42] |
| pinchartl | there's a high chance that the hardware used to move the camera will not bu controlled by drivers/media/
we don't know what it would be at this point so trying to standardize an API to support this use case without knowing what's needed doesn't seem right at the moment | [10:42] |
| mchehab | pinchartl: ok, but, on such case, the sensor should report an updated information after the sensor move its direction...
and should emit an event | [10:43] |
| pinchartl | it's easy to make a read-only control read-write later, it's more difficult to reconsider the read-write API if someone starts using it and we then realize it's not good enough, right ? | [10:44] |
| mchehab | true, but if an application using it would check this control only once when it starts, such app will take wrong decisions when we add support for flippable sensors
so, I would prefer to have at least some note there stating that some sensors could have the LENS_FACING changed at runtime for application developers to take it into account when writing apps | [10:45] |
| pinchartl | do you really think they would ?
I mean, can you imagine an application being written to use that control that would already take that into account ? even if we specify that it can change I'll be honest here about a year and a half ago, I changed focus, from kernelspace to userspace working on libcamera it was the first I was using the V4L2 and MC APIs for real | [10:46] |
| mchehab | at least app developers won't have much arguments when we add support for R/W, if the doc has a notice about that | [10:49] |
| pinchartl | not in test tools, or custom applications for a particular embedded project
but in something more generic, and real and I was surprised by the number of deficiences in the APIs they can be classified in two categories missing features | [10:49] |
| mchehab | pinchartl: yeah, I know the feeling. I had similar issues when I took over maintainership of some userspace apps like camorama and zbar | [10:49] |
| pinchartl | which prompted for the V4L2_ENUM_FMT extension
but also lots of useless features we have designed all these APIs with very little consideration of how they can be used in practice there's lots of features to enable "generic userspace applications" that are in practice useless | [10:49] |
| mchehab | for non-MC this is not that bad | [10:51] |
| pinchartl | you would be surprised | [10:51] |
| mchehab | but yeah, one of the issues is with regards to controls
implementing proper support for them on a real userspace app is not nice | [10:51] |
| pinchartl | specifying a volatile lens facing control would be in that category in my opinion. it may look nice when you look at it in isolation, but nobody will use it | [10:52] |
| mchehab | well, if the flip motor would be controlled via V4L2, it makes sense to use a volatile control
otherwise, if this would be controlled by a separate API, I agree with you | [10:54] |
| pinchartl | for instance, libcamera won't run on kernels older than v5.0, as only then did vb2 get enough support for dynamic buffer memory management
before that, workarounds were needed, it wasn't pretty and resulted in wasted memory and full frame memcpy | [10:55] |
| mchehab | yeah, a large amount of code on userspace driver counterpart is related to backward compatibility stuff
the more we change, the worse it becomes that's why before adding a new control we should have some discussions, as a future change will imply on backward-compatibility code being written on both KS and US (KS -kernelspace - US- userspace) | [10:56] |
| pinchartl | and we should also stop thinking we know better, and start trusting userspace developers :-) | [10:58] |
| mchehab | answered Jacopo's email about the lens facing control. Let's keep the remaining discussions (if needed) via e-mail
with regards the ENUM_FMT, I have the series locally applied, together with my patch. I'm intending to push the patch series tomorrow, giving some time for people to look the patch | [11:06] |
| pinchartl | thank you
I'll then merge support for i.MX7 and sunxi in libcamera, they depend on it as well as support for qcom camss (and in case you missed it, https://www.raspberrypi.org/blog/an-open-source-camera-stack-for-raspberry-pi-using-libcamera/) | [11:08] |
| mchehab | nice! yeah, I missed that. I'll seek for some time to test it. I have one RPi3 device here with me with camera module 2
(I have a v1 module, but it is in Brazil) | [11:10] |
| pinchartl | I recommend you use their rpi-5.4.y kernel branch for now
I've tested the code on mainline and it works | [11:10] |
| mchehab | great!
good work! congrats! | [11:11] |
| pinchartl | but it requires a few DT patches that haven't been posted yet, it's messy at the moment
thanks ~4 months of development on our side, 3 years in total for raspberry pi | [11:11] |
| mchehab | that's indeed a great achievement! Yeah, it could take some time to merge everything at rpi tree
and maybe even more for upstream Kernel to support everything on RPi, but that will eventually happen some day | [11:13] |
| ........ (idle for 35mn) | ||
| hverkuil | mchehab: pinchartl: LENS_FACING sounds good to me. With that change that series should be good to go.
mchehab: I posted a v2 of your ENUM_FMT patch. Looking at the HTML page it's quite nice and understandable (at least IMHO). pinchartl: historically the problem with V4L2 camera support in particular is that none of the major vendors were using it, either because they had their own implementation (OMX) or because it was heavily hacked. And they never came back to us telling us what was missing. The libcamera development is really the first time this happens since the Nokia days. The only way to develop a good API is if the people doing the userspace development work with us. good kernel API The V4L2 APIs for HDMI receivers are working well because they were designed with a real Cisco use-case and application software in mind. They are used every day in zillions of Cisco video conferencing devices. I hope that the kernel - libcamera cooperation will substantially the V4L2 API as well when it comes to cameras. | [11:48] |
| mchehab: pinchartl: regarding camera locations in cars: I agree with pinchartl that that will almost certainly be handled in userspace, not through DT bindings. But regardless, this is something that you would have to talk about to a specialist on this topic. Trying to predict what they would need would be complete guesswork. Let's not go there. | [12:03] | |
| mchehab | hverkuil: thanks! just replied to it. sounds ok, but there's one undefined behavior for the MC case
(mbus_code not zero, but either not valid or not used by a driver) | [12:03] |
| hverkuil | That would return -EINVAL for index 0. I.e., an empty list. I'll post a v3 clarifying that. | [12:04] |
| mchehab | OK
yeah, that's what I would be expecting, but better to cover it to avoid implementation mistakes. | [12:06] |
| hverkuil | mchehab: posted v3 | [12:12] |
| pinchartl | hverkuil: we've been a bit too arrogant though, telling vendors they "just" had to collaborate with us, without realizing that the gap to be bridged was getting wider and wider
up to the point where it's too big to bridge for any single vendor and we don't have resources on our side to assign 3 people working full time during one year | [12:17] |
| .... (idle for 15mn) | ||
| hverkuil | I don't have a lot of sympathy for vendors. If you want good mainline support for your devices, you need to invest money and manpower into that. Nobody will do it for you. Most of the API work is done by a handful of devs that are paid by companies that are actually willing to make that investment. And yes, due to the lack of investment since Nokia burned its platform the gap is now very wide. I don't see what we could possibly have
done differently. libcamera is really the first time that a decent investment is being made. Basically you get what you pay for. The time that hobbyists would just fix it for you is long gone. | [12:33] |
| pinchartl | that won't happen without someone driving the effort | [12:37] |
| mchehab | hverkuil: I agree with you: anyone wanting to do something different should submit patches and discuss things upstream | [12:41] |
| hverkuil | I completely agree with that. After all, that's how I got involved on behalf of Cisco. But that was from within Cisco (or Tandberg at the time). | [12:47] |
| pinchartl | so the plan is wait, see and complain that nothing happens ? :-) | [12:47] |
| ...... (idle for 28mn) | ||
| hverkuil | It's great if someone is willing/able to talk with vendors and have them invest in this. I.e. 'be a champion' in management speak. But without that, yeah, nothing will happen. What else would you expect? It might be different if there was a 'customer-vendor' relationship with contracts etc., but that's not the case. | [13:15] |
| ...... (idle for 26mn) | ||
| mchehab: FYI: I have patches ready for v4l-utils once the CAP_IO_MC series is merged. I'll take care of merging those. | [13:41] | |
| mchehab | ok
hverkuil: going ahead and merging what I have upstream if changes would be needed, they could be done on a separate patch | [13:42] |
| ................................................... (idle for 4h13mn) | ||
| *** | syoung has quit IRC (Quit: leaving) | [17:57] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |