| Message ID | 20180602210145.GB20439@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pavel, Thanks for coming up with this proposal. Please see my comments below. On Sun, Jun 3, 2018 at 6:01 AM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > Ok, can I get any comments on this one? > v4l2_open_complex("/file/with/descriptor", 0) can be used to open > whole pipeline at once, and work if it as if it was one device. I'm not convinced if we should really be piggy backing on libv4l, but it's just a matter of where we put the code added by your patch, so let's put that aside. Who would be calling this function? The scenario that I could think of is: - legacy app would call open(/dev/video?), which would be handled by libv4l open hook (v4l2_open()?), - v4l2_open() would check if given /dev/video? figures in its list of complex pipelines, for example by calling v4l2_open_complex() and seeing if it succeeds, - if it succeeds, the resulting fd would represent the complex pipeline, otherwise it would just open the requested node directly. I guess that could give some basic camera functionality on OMAP3-like hardware. For most of the current generation of imaging subsystems (e.g. Intel IPU3, Rockchip RKISP1) it's not enough. The reason is that there is more to be handled by userspace than just setting controls: - configuring pixel formats, resolutions, crops, etc. through the whole pipeline - I guess that could be preconfigured per use case inside the configuration file, though, - forwarding buffers between capture and processing pipelines, i.e. DQBUF raw frame from CSI2 video node and QBUF to ISP video node, - handling metadata CAPTURE and OUTPUT buffers controlling the 3A feedback loop - this might be optional if all we need is just ability to capture some frames, but required for getting good quality, - actually mapping legacy controls into the above metadata, I guess it's just a matter of adding further code to handle those, though. However, it would build up a separate legacy framework that locks us up into the legacy USB camera model, while we should rather be leaning towards a more flexible framework, such as Android Camera HALv3 or Pipewire. On top of such framework, we could just have a very thin layer to emulate the legacy, single video node, camera. Other than that, I agree that we should be able to have pre-generated configuration files and use them to build and setup the pipeline. That's actually what we have in camera HALs on Chrome OS (Android camera HALv3 model adopted to Chrome OS). Some minor comments for the code follow. [snip] > +int v4l2_get_fd_for_control(int fd, unsigned long control) > +{ > + int index = v4l2_get_index(fd); > + struct v4l2_controls_map *map; > + int lo = 0; > + int hi; > + > + if (index < 0) > + return fd; > + > + map = devices[index].map; > + if (!map) > + return fd; > + hi = map->num_controls; > + > + while (lo < hi) { > + int i = (lo + hi) / 2; > + if (map->map[i].control == control) { > + return map->map[i].fd; > + } > + if (map->map[i].control > control) { > + hi = i; > + continue; > + } > + if (map->map[i].control < control) { > + lo = i+1; > + continue; > + } > + printf("Bad: impossible condition in binary search\n"); > + exit(1); > + } Could we use bsearch() here? > + return fd; > +} > + > int v4l2_ioctl(int fd, unsigned long int request, ...) > { > void *arg; > va_list ap; > int result, index, saved_err; > - int is_capture_request = 0, stream_needs_locking = 0; > + int is_capture_request = 0, stream_needs_locking = 0, > + is_subdev_request = 0; > > va_start(ap, request); > arg = va_arg(ap, void *); > @@ -1076,18 +1115,20 @@ int v4l2_ioctl(int fd, unsigned long int request, ...) > ioctl, causing it to get sign extended, depending upon this behavior */ > request = (unsigned int)request; > > + /* FIXME */ > if (devices[index].convert == NULL) > goto no_capture_request; > > /* Is this a capture request and do we need to take the stream lock? */ > switch (request) { > - case VIDIOC_QUERYCAP: > case VIDIOC_QUERYCTRL: > case VIDIOC_G_CTRL: > case VIDIOC_S_CTRL: > case VIDIOC_G_EXT_CTRLS: > - case VIDIOC_TRY_EXT_CTRLS: > case VIDIOC_S_EXT_CTRLS: > + is_subdev_request = 1; > + case VIDIOC_QUERYCAP: > + case VIDIOC_TRY_EXT_CTRLS: I'm not sure why we are moving those around. Is this perhaps related to the FIXME comment above? If so, it would be helpful to have some short explanation next to it. > case VIDIOC_ENUM_FRAMESIZES: > case VIDIOC_ENUM_FRAMEINTERVALS: > is_capture_request = 1; > @@ -1151,10 +1192,15 @@ int v4l2_ioctl(int fd, unsigned long int request, ...) > } > > if (!is_capture_request) { > + int sub_fd; > no_capture_request: > + sub_fd = fd; > + if (is_subdev_request) { > + sub_fd = v4l2_get_fd_for_control(index, ((struct v4l2_queryctrl *) arg)->id); > + } nit: Looks like something weird going on here with indentation. > result = devices[index].dev_ops->ioctl( > devices[index].dev_ops_priv, > - fd, request, arg); > + sub_fd, request, arg); > saved_err = errno; > v4l2_log_ioctl(request, arg, result); > errno = saved_err; > @@ -1782,3 +1828,195 @@ int v4l2_get_control(int fd, int cid) > (qctrl.maximum - qctrl.minimum) / 2) / > (qctrl.maximum - qctrl.minimum); > } > + > + nit: Double blank line. > +int v4l2_open_pipeline(struct v4l2_controls_map *map, int v4l2_flags) > +{ > + int index; > + int i; > + > + for (i=0; i<map->num_controls; i++) { > + if (map->map[i].fd <= 0) { > + V4L2_LOG_ERR("v4l2_open_pipeline: Bad fd in map.\n"); > + return -1; > + } > + if (i>=1 && map->map[i].control <= map->map[i-1].control) { > + V4L2_LOG_ERR("v4l2_open_pipeline: Controls not sorted.\n"); > + return -1; I guess we could just sort them at startup with qsort()? Best regards, Tomasz
Hi! > Thanks for coming up with this proposal. Please see my comments below. > > > Ok, can I get any comments on this one? > > v4l2_open_complex("/file/with/descriptor", 0) can be used to open > > whole pipeline at once, and work if it as if it was one device. > > I'm not convinced if we should really be piggy backing on libv4l, but > it's just a matter of where we put the code added by your patch, so > let's put that aside. There was some talk about this before, and libv4l2 is what we came with. Only libv4l2 is in position to propagate controls to right devices. > Who would be calling this function? > > The scenario that I could think of is: > - legacy app would call open(/dev/video?), which would be handled by > libv4l open hook (v4l2_open()?), I don't think that kind of legacy apps is in use any more. I'd prefer not to deal with them. > - v4l2_open() would check if given /dev/video? figures in its list of > complex pipelines, for example by calling v4l2_open_complex() and > seeing if it succeeds, I'd rather not have v4l2_open_complex() called on devices. We could test if argument is regular file and then call it... But again, that's next step. > - if it succeeds, the resulting fd would represent the complex > pipeline, otherwise it would just open the requested node directly. > I guess that could give some basic camera functionality on OMAP3-like hardware. It definitely gives camera functionality on OMAP3. I'm using it to take photos with Nokia N900. > For most of the current generation of imaging subsystems (e.g. Intel > IPU3, Rockchip RKISP1) it's not enough. The reason is that there is > more to be handled by userspace than just setting controls: > - configuring pixel formats, resolutions, crops, etc. through the > whole pipeline - I guess that could be preconfigured per use case > inside the configuration file, though, That may be future plan. Note that these can be preconfigured; unlike controls propagation... > - forwarding buffers between capture and processing pipelines, i.e. > DQBUF raw frame from CSI2 video node and QBUF to ISP video node, My hardware does not need that, so I could not test it. I'll rely on someone with required hardware to provide that. (And you can take DQBUF and process it with software, at cost of slightly higher CPU usage, right?) > - handling metadata CAPTURE and OUTPUT buffers controlling the 3A > feedback loop - this might be optional if all we need is just ability > to capture some frames, but required for getting good quality, > - actually mapping legacy controls into the above metadata, I'm not sure what 3A is. If you mean hardware histograms and friends, yes, it would be nice to support that, but, again, statistics can be computed in software. > I guess it's just a matter of adding further code to handle those, > though. However, it would build up a separate legacy framework that > locks us up into the legacy USB camera model, while we should rather > be leaning towards a more flexible framework, such as Android Camera > HALv3 or Pipewire. On top of such framework, we could just have a very > thin layer to emulate the legacy, single video node, camera. Yes, we'll need something more advanced. But.. we also need something to run the devices today, so that kernel drivers can be tested and do not bitrot. That's why I'm doing this work. And I believe we should work in steps before getting there... controls propagation can not be done from external application, so I'm starting with it. > Some minor comments for the code follow. Ok, let me send this, then go through the comments. Best regards, Pavel
On Wed, Jun 6, 2018 at 5:46 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > Thanks for coming up with this proposal. Please see my comments below. > > > > > Ok, can I get any comments on this one? > > > v4l2_open_complex("/file/with/descriptor", 0) can be used to open > > > whole pipeline at once, and work if it as if it was one device. > > > > I'm not convinced if we should really be piggy backing on libv4l, but > > it's just a matter of where we put the code added by your patch, so > > let's put that aside. > > There was some talk about this before, and libv4l2 is what we came > with. Only libv4l2 is in position to propagate controls to right > devices. > > > Who would be calling this function? > > > > The scenario that I could think of is: > > - legacy app would call open(/dev/video?), which would be handled by > > libv4l open hook (v4l2_open()?), > > I don't think that kind of legacy apps is in use any more. I'd prefer > not to deal with them. In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, 19"), Mauro has mentioned a number of those: "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed source ones (Skype, Chrome, ...)" > > > - v4l2_open() would check if given /dev/video? figures in its list of > > complex pipelines, for example by calling v4l2_open_complex() and > > seeing if it succeeds, > > I'd rather not have v4l2_open_complex() called on devices. We could > test if argument is regular file and then call it... But again, that's > next step. > > > - if it succeeds, the resulting fd would represent the complex > > pipeline, otherwise it would just open the requested node directly. What's the answer to my original question of who would be calling v4l2_open_complex(), then? > > > I guess that could give some basic camera functionality on OMAP3-like hardware. > > It definitely gives camera functionality on OMAP3. I'm using it to > take photos with Nokia N900. > > > For most of the current generation of imaging subsystems (e.g. Intel > > IPU3, Rockchip RKISP1) it's not enough. The reason is that there is > > more to be handled by userspace than just setting controls: > > - configuring pixel formats, resolutions, crops, etc. through the > > whole pipeline - I guess that could be preconfigured per use case > > inside the configuration file, though, > > That may be future plan. Note that these can be preconfigured; unlike > controls propagation... > > > - forwarding buffers between capture and processing pipelines, i.e. > > DQBUF raw frame from CSI2 video node and QBUF to ISP video node, > > My hardware does not need that, so I could not test it. I'll rely on > someone with required hardware to provide that. > > (And you can take DQBUF and process it with software, at cost of > slightly higher CPU usage, right?) > > > - handling metadata CAPTURE and OUTPUT buffers controlling the 3A > > feedback loop - this might be optional if all we need is just ability > > to capture some frames, but required for getting good quality, > > - actually mapping legacy controls into the above metadata, > > I'm not sure what 3A is. If you mean hardware histograms and friends, > yes, it would be nice to support that, but, again, statistics can be > computed in software. Auto-exposure, auto-white-balance, auto-focus. In complex camera subsystems these need to be done in software. On most hardware platforms, ISP provides necessary input data (statistics) and software calculates required processing parameters. > > > I guess it's just a matter of adding further code to handle those, > > though. However, it would build up a separate legacy framework that > > locks us up into the legacy USB camera model, while we should rather > > be leaning towards a more flexible framework, such as Android Camera > > HALv3 or Pipewire. On top of such framework, we could just have a very > > thin layer to emulate the legacy, single video node, camera. > > Yes, we'll need something more advanced. > > But.. we also need something to run the devices today, so that kernel > drivers can be tested and do not bitrot. That's why I'm doing this > work. I guess the most important bit I missed then is what is the intended use case for this. It seems to be related to my earlier, unanswered question about who would be calliing v4l2_open_complex(), though. What userspace applications would be used for this testing? Best regards, Tomasz
Hi! > > > Who would be calling this function? > > > > > > The scenario that I could think of is: > > > - legacy app would call open(/dev/video?), which would be handled by > > > libv4l open hook (v4l2_open()?), > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > not to deal with them. > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > 19"), Mauro has mentioned a number of those: > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > source ones (Skype, Chrome, ...)" Yep, ok. Still would prefer not to deal with them. (Opening additional fds behind application's back is quite nasty, those apps should really switch to v4l2_ variants). > > > - v4l2_open() would check if given /dev/video? figures in its list of > > > complex pipelines, for example by calling v4l2_open_complex() and > > > seeing if it succeeds, > > > > I'd rather not have v4l2_open_complex() called on devices. We could > > test if argument is regular file and then call it... But again, that's > > next step. > > > > > - if it succeeds, the resulting fd would represent the complex > > > pipeline, otherwise it would just open the requested node directly. > > What's the answer to my original question of who would be calling > v4l2_open_complex(), then? Application ready to deal with additional fds being opened. contrib/test/sdlcam will be the first one. We may do some magic to do v4l2_open_complex() in v4l2_open(), but I believe that should be separate step. > > > - handling metadata CAPTURE and OUTPUT buffers controlling the 3A > > > feedback loop - this might be optional if all we need is just ability > > > to capture some frames, but required for getting good quality, > > > - actually mapping legacy controls into the above metadata, > > > > I'm not sure what 3A is. If you mean hardware histograms and friends, > > yes, it would be nice to support that, but, again, statistics can be > > computed in software. > > Auto-exposure, auto-white-balance, auto-focus. In complex camera > subsystems these need to be done in software. On most hardware > platforms, ISP provides necessary input data (statistics) and software > calculates required processing parameters. Ok, so... statistics support would be nice, but that is really separate problem. v4l2 already contains auto-exposure and auto-white-balance. I have patches for auto-focus. But hardware statistics are not used. > > Yes, we'll need something more advanced. > > > > But.. we also need something to run the devices today, so that kernel > > drivers can be tested and do not bitrot. That's why I'm doing this > > work. > > I guess the most important bit I missed then is what is the intended > use case for this. It seems to be related to my earlier, unanswered > question about who would be calliing v4l2_open_complex(), though. > > What userspace applications would be used for this testing? Main use case is kernel testing. Secondary use case is taking .jpg photos using sdlcam. Test apps such as qv4l2 would be nice to have, and maybe I'll experiment with capturing video somehow one day. I'm pretty sure it will not be easy. Oh and I guess a link to how well it works? See https://www.youtube.com/watch?v=fH6zuK2OOVU . Best regards, Pavel
Hi! > > + while (lo < hi) { > > + int i = (lo + hi) / 2; > > + if (map->map[i].control == control) { > > + return map->map[i].fd; > > + } > > + if (map->map[i].control > control) { > > + hi = i; > > + continue; > > + } > > + if (map->map[i].control < control) { > > + lo = i+1; > > + continue; > > + } > > + printf("Bad: impossible condition in binary search\n"); > > + exit(1); > > + } > > Could we use bsearch() here? We could, but it will mean passing function pointers etc. It would make sense if we want to do sorting. > > @@ -1076,18 +1115,20 @@ int v4l2_ioctl(int fd, unsigned long int request, ...) > > ioctl, causing it to get sign extended, depending upon this behavior */ > > request = (unsigned int)request; > > > > + /* FIXME */ > > if (devices[index].convert == NULL) > > goto no_capture_request; > > > > /* Is this a capture request and do we need to take the stream lock? */ > > switch (request) { > > - case VIDIOC_QUERYCAP: > > case VIDIOC_QUERYCTRL: > > case VIDIOC_G_CTRL: > > case VIDIOC_S_CTRL: > > case VIDIOC_G_EXT_CTRLS: > > - case VIDIOC_TRY_EXT_CTRLS: > > case VIDIOC_S_EXT_CTRLS: > > + is_subdev_request = 1; > > + case VIDIOC_QUERYCAP: > > + case VIDIOC_TRY_EXT_CTRLS: > > I'm not sure why we are moving those around. Is this perhaps related > to the FIXME comment above? If so, it would be helpful to have some > short explanation next to it. I want to do controls propagation only on ioctls that manipulate controls, so I need to group them together. The FIXME comment is not related. > > > > if (!is_capture_request) { > > + int sub_fd; > > no_capture_request: > > + sub_fd = fd; > > + if (is_subdev_request) { > > + sub_fd = v4l2_get_fd_for_control(index, ((struct v4l2_queryctrl *) arg)->id); > > + } > > nit: Looks like something weird going on here with indentation. Fixed. > > @@ -1782,3 +1828,195 @@ int v4l2_get_control(int fd, int cid) > > (qctrl.maximum - qctrl.minimum) / 2) / > > (qctrl.maximum - qctrl.minimum); > > } > > + > > + > > nit: Double blank line. Fixed. > > + if (i>=1 && map->map[i].control <= map->map[i-1].control) { > > + V4L2_LOG_ERR("v4l2_open_pipeline: Controls not sorted.\n"); > > + return -1; > > I guess we could just sort them at startup with qsort()? We could... but I'd prefer them sorted on-disk, as it is written very seldom, but needs to be readed on every device open. Thanks for review, Pavel
HI! > > > Thanks for coming up with this proposal. Please see my comments below. > > > > > > > Ok, can I get any comments on this one? > > > > v4l2_open_complex("/file/with/descriptor", 0) can be used to open > > > > whole pipeline at once, and work if it as if it was one device. > > > > > > I'm not convinced if we should really be piggy backing on libv4l, but > > > it's just a matter of where we put the code added by your patch, so > > > let's put that aside. > > > > There was some talk about this before, and libv4l2 is what we came > > with. Only libv4l2 is in position to propagate controls to right > > devices. > > > > > Who would be calling this function? > > > > > > The scenario that I could think of is: > > > - legacy app would call open(/dev/video?), which would be handled by > > > libv4l open hook (v4l2_open()?), > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > not to deal with them. > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > 19"), Mauro has mentioned a number of those: > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > source ones (Skype, Chrome, ...)" Thanks for thread pointer... I may be able to get in using hangouts. Anyway, there's big difference between open("/dev/video0") and v4l2_open("/dev/video0"). I don't care about the first one, but yes we should be able to support the second one eventually. And I don't think Mauro says apps like Camorama are of open() kind. Best regards, Pavel
On Wed, Jun 6, 2018 at 7:51 PM Pavel Machek <pavel@ucw.cz> wrote: > > HI! > > > > > Thanks for coming up with this proposal. Please see my comments below. > > > > > > > > > Ok, can I get any comments on this one? > > > > > v4l2_open_complex("/file/with/descriptor", 0) can be used to open > > > > > whole pipeline at once, and work if it as if it was one device. > > > > > > > > I'm not convinced if we should really be piggy backing on libv4l, but > > > > it's just a matter of where we put the code added by your patch, so > > > > let's put that aside. > > > > > > There was some talk about this before, and libv4l2 is what we came > > > with. Only libv4l2 is in position to propagate controls to right > > > devices. > > > > > > > Who would be calling this function? > > > > > > > > The scenario that I could think of is: > > > > - legacy app would call open(/dev/video?), which would be handled by > > > > libv4l open hook (v4l2_open()?), > > > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > > not to deal with them. > > > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > > 19"), Mauro has mentioned a number of those: > > > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > > source ones (Skype, Chrome, ...)" > > Thanks for thread pointer... I may be able to get in using hangouts. > > Anyway, there's big difference between open("/dev/video0") and > v4l2_open("/dev/video0"). I don't care about the first one, but yes we > should be able to support the second one eventually. > > And I don't think Mauro says apps like Camorama are of open() kind. I don't think there is much difference between open() and v4l2_open(), since the former can be changed to the latter using LD_PRELOAD. If we simply add v4l2_open_complex() to libv4l, we would have to get developers of the applications (regardless of whether they use open() or v4l2_open()) to also support v4l2_open_complex(). For testing purposes of kernel developers it would work indeed, but I wonder if it goes anywhere beyond that. If all we need is some code to be able to test kernel camera drivers, I don't think there is any big problem in adding v4l2_open_complex() to libv4l. However, we must either ensure that either: a) it's not going to be widely used OR b) it is designed well enough to cover the complex cases I mentioned and which are likely to represent most of the hardware in the wild. Best regards, Tomasz
Em Wed, 6 Jun 2018 12:01:50 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > > > Who would be calling this function? > > > > > > > > The scenario that I could think of is: > > > > - legacy app would call open(/dev/video?), which would be handled by > > > > libv4l open hook (v4l2_open()?), > > > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > > not to deal with them. > > > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > > 19"), Mauro has mentioned a number of those: > > > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > > source ones (Skype, Chrome, ...)" > > Yep, ok. Still would prefer not to deal with them. I guess we'll end by needing to handle them. Anyway, now that PCs are starting to come with complex cameras[1], we'll need to address this with a focus on adding support for existing apps. [1] we had a report from one specific model but I heard from a reliable source that there are already other devices with similar issues. > (Opening additional fds behind application's back is quite nasty, > those apps should really switch to v4l2_ variants). Only closed source apps use the LD_PRELOAD hack. All the others use v4l2_ variants, but, as Nicolas mentioned at the other thread, there are a number of problems with the current approach. Perhaps is time for us to not be limited to the current ABI, writing a new API from scratch, and then adding a compatibility layer to be used by apps that rely on v4l2_ variants, in order to avoid breaking the ABI and keep providing LD_PRELOAD. We can then convert the apps we use/care most to use the new ABI. > > > > > - v4l2_open() would check if given /dev/video? figures in its list of > > > > complex pipelines, for example by calling v4l2_open_complex() and > > > > seeing if it succeeds, > > > > > > I'd rather not have v4l2_open_complex() called on devices. We could > > > test if argument is regular file and then call it... But again, that's > > > next step. > > > > > > > - if it succeeds, the resulting fd would represent the complex > > > > pipeline, otherwise it would just open the requested node directly. > > > > What's the answer to my original question of who would be calling > > v4l2_open_complex(), then? > > Application ready to deal with additional fds being > opened. contrib/test/sdlcam will be the first one. > > We may do some magic to do v4l2_open_complex() in v4l2_open(), but I > believe that should be separate step. In order to avoid breaking the ABI for existing apps, v4l2_open() should internally call v4l2_open_complex() (or whatever we call it at the new API design). > > > > - handling metadata CAPTURE and OUTPUT buffers controlling the 3A > > > > feedback loop - this might be optional if all we need is just ability > > > > to capture some frames, but required for getting good quality, > > > > - actually mapping legacy controls into the above metadata, > > > > > > I'm not sure what 3A is. If you mean hardware histograms and friends, > > > yes, it would be nice to support that, but, again, statistics can be > > > computed in software. > > > > Auto-exposure, auto-white-balance, auto-focus. In complex camera > > subsystems these need to be done in software. On most hardware > > platforms, ISP provides necessary input data (statistics) and software > > calculates required processing parameters. > > Ok, so... statistics support would be nice, but that is really > separate problem. > > v4l2 already contains auto-exposure and auto-white-balance. I have > patches for auto-focus. But hardware statistics are not used. Feel free to submit the auto-focus patches anytime. With all 3A algos there (even on a non-optimal way using hw stats), it will make easier for us when designing a solution that would work for both IMAP3 and ISP (and likely be generic enough for other hardware). For the full complex hardware solution, though, it is probably better to fork v4l-utils into a separate project, in order to do the development without affecting current users of it, merging it back only after we'll be sure that existing apps will keep working with v4l2_foo() functions and LD_PRELOAD. Anyway, this is something that it makes sense to discuss during the Complex Camera Workshop. > > > Yes, we'll need something more advanced. > > > > > > But.. we also need something to run the devices today, so that kernel > > > drivers can be tested and do not bitrot. That's why I'm doing this > > > work. > > > > I guess the most important bit I missed then is what is the intended > > use case for this. It seems to be related to my earlier, unanswered > > question about who would be calliing v4l2_open_complex(), though. > > > > What userspace applications would be used for this testing? > > Main use case is kernel testing. > > Secondary use case is taking .jpg photos using sdlcam. If a v4l2_complex_open() will, for now, be something that we don't export publicly, using it only for the tools already at libv4l2, I don't see much troubles on adding it, but I would hate to have to stick with this ABI. Otherwise, we should analyze it after having a bigger picture. So, better to wait for the Complex Camera Workshop before adding this. Btw, would you be able to join us there (either locally or remotely via Google Hangouts)? > Test apps such as qv4l2 would be nice to have, and maybe I'll > experiment with capturing video somehow one day. I'm pretty sure it > will not be easy. Capture video is a must have for PCs. The final solution should take it into account. > > Oh and I guess a link to how well it works? See > https://www.youtube.com/watch?v=fH6zuK2OOVU . > > Best regards, > Pavel Thanks, Mauro
Em Wed, 6 Jun 2018 12:51:16 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > > > > The scenario that I could think of is: > > > > - legacy app would call open(/dev/video?), which would be handled by > > > > libv4l open hook (v4l2_open()?), > > > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > > not to deal with them. > > > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > > 19"), Mauro has mentioned a number of those: > > > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > > source ones (Skype, Chrome, ...)" > > Thanks for thread pointer... I may be able to get in using hangouts. > > Anyway, there's big difference between open("/dev/video0") and > v4l2_open("/dev/video0"). I don't care about the first one, but yes we > should be able to support the second one eventually. > > And I don't think Mauro says apps like Camorama are of open() kind. All open source apps we care use v4l2_open() & friends. the ones that use just open() work via LD_PRELOAD. It is a hack, but it was needed when libv4l was added (as there were lots of apps to be touched). Also, we had problems on that time with closed source app developers. I guess nowadays, among v4l-specific apps, only closed source ones use just open(). Haven't check how browsers open cameras, though. A quick look at the Fedora 60 dependencies, though, doesn't show libv4l: https://rpmfind.net/linux/RPM/fedora/devel/rawhide/x86_64/f/firefox-60.0.1-5.fc29.x86_64.html It might be statically linking libv4l, or maybe they rely on something else (like java/flash/...), but I guess it is more likely that they're just using open() somehow. The same kind of issue may also be present on other browsers and on java libraries. Thanks, Mauro
Hi! > > Thanks for thread pointer... I may be able to get in using hangouts. > > > > Anyway, there's big difference between open("/dev/video0") and > > v4l2_open("/dev/video0"). I don't care about the first one, but yes we > > should be able to support the second one eventually. > > > > And I don't think Mauro says apps like Camorama are of open() kind. > > I don't think there is much difference between open() and v4l2_open(), > since the former can be changed to the latter using LD_PRELOAD. Well, if everyone thinks opening more than one fd in v4l2_open() is okay, I can do that. Probably "if argument is regular file and has .mc extension, use open_complex"? > If we simply add v4l2_open_complex() to libv4l, we would have to get > developers of the applications (regardless of whether they use open() > or v4l2_open()) to also support v4l2_open_complex(). For testing > purposes of kernel developers it would work indeed, but I wonder if it > goes anywhere beyond that. I'd like people to think "is opening multiple fds okay at this moment?" before switching to v4l2_open_complex(). But I guess I'm too careful. > If all we need is some code to be able to test kernel camera drivers, > I don't think there is any big problem in adding v4l2_open_complex() > to libv4l. However, we must either ensure that either: > a) it's not going to be widely used > OR > b) it is designed well enough to cover the complex cases I mentioned > and which are likely to represent most of the hardware in the wild. .mc descriptors should be indeed extensible enough for that. Best regards, Pavel
Hi! > > > > I don't think that kind of legacy apps is in use any more. I'd prefer > > > > not to deal with them. > > > > > > In another thread ("[ANN v2] Complex Camera Workshop - Tokyo - Jun, > > > 19"), Mauro has mentioned a number of those: > > > > > > "open source ones (Camorama, Cheese, Xawtv, Firefox, Chromium, ...) and closed > > > source ones (Skype, Chrome, ...)" > > > > Yep, ok. Still would prefer not to deal with them. > > I guess we'll end by needing to handle them. Anyway, now that PCs are > starting to come with complex cameras[1], we'll need to address this > with a focus on adding support for existing apps. > > [1] we had a report from one specific model but I heard from a reliable > source that there are already other devices with similar issues. Well, now that the issue hit PCs, we'll get help from Linux distributors. > > (Opening additional fds behind application's back is quite nasty, > > those apps should really switch to v4l2_ variants). > > Only closed source apps use the LD_PRELOAD hack. All the others > use v4l2_ variants, but, as Nicolas mentioned at the other > thread, there are a number of problems with the current approach. > > Perhaps is time for us to not be limited to the current ABI, writing > a new API from scratch, and then adding a compatibility layer to be > used by apps that rely on v4l2_ variants, in order to avoid breaking > the ABI and keep providing LD_PRELOAD. We can then convert the apps > we use/care most to use the new ABI. I believe we can support current features using existing libv4l2 abi -- complex cameras should work as well as dumb ones do. ...something like that is certainly needed for testing. But yes, long-term better interface is needed -- and code should not be in library but in separate process. > > Application ready to deal with additional fds being > > opened. contrib/test/sdlcam will be the first one. > > > > We may do some magic to do v4l2_open_complex() in v4l2_open(), but I > > believe that should be separate step. > > In order to avoid breaking the ABI for existing apps, v4l2_open() should > internally call v4l2_open_complex() (or whatever we call it at the new > API design). Ok... everyone wants that. I can do that. > > Ok, so... statistics support would be nice, but that is really > > separate problem. > > > > v4l2 already contains auto-exposure and auto-white-balance. I have > > patches for auto-focus. But hardware statistics are not used. > > Feel free to submit the auto-focus patches anytime. With all 3A > algos there (even on a non-optimal way using hw stats), it will > make easier for us when designing a solution that would work for > both IMAP3 and ISP (and likely be generic enough for other hardware). Let me finish the control propagation, first. My test hardware supporting autofocus is N900, so I need control propagation to be able to test autofocus. > > Secondary use case is taking .jpg photos using sdlcam. > > If a v4l2_complex_open() will, for now, be something that we don't > export publicly, using it only for the tools already at libv4l2, > I don't see much troubles on adding it, but I would hate to have to > stick with this ABI. Otherwise, we should analyze it after having > a bigger picture. So, better to wait for the Complex Camera > Workshop before adding this. > > Btw, would you be able to join us there (either locally or > remotely via Google Hangouts)? I don't plan going to Japan. Hangouts... I'm not big fan of Google, but I'll try. I'm in GMT+2. > > Test apps such as qv4l2 would be nice to have, and maybe I'll > > experiment with capturing video somehow one day. I'm pretty sure it > > will not be easy. > > Capture video is a must have for PCs. The final solution should > take it into account. N900 is quite slow for JPEG compression (and I don't really want solution depending on hardware extras like DSP)... so that will be fun. Pavel
Hi! > I guess that could give some basic camera functionality on OMAP3-like hardware. Yeah, and that is the goal. > For most of the current generation of imaging subsystems (e.g. Intel > IPU3, Rockchip RKISP1) it's not enough. The reason is that there is > more to be handled by userspace than just setting controls: > - configuring pixel formats, resolutions, crops, etc. through the > whole pipeline - I guess that could be preconfigured per use case > inside the configuration file, though, > - forwarding buffers between capture and processing pipelines, i.e. > DQBUF raw frame from CSI2 video node and QBUF to ISP video node, > - handling metadata CAPTURE and OUTPUT buffers controlling the 3A > feedback loop - this might be optional if all we need is just ability > to capture some frames, but required for getting good quality, > - actually mapping legacy controls into the above metadata, I just wanted to add few things: It seems IPU3 and RKISP1 is really similar to what I have on N900. Forwarding frames between parts of processing pipeline is not neccessary, but the other parts are there. There are also two points where you can gather the image data, either (almost) raw GRBG10 data from the sensor, or scaled YUV data ready for display. [And how to display that data without CPU involvement is another, rather big, topic.] Anyway, legacy applications expect simple webcams with bad pictures, low resolution, and no AF support. And we should be able to provide them with just that. Best regards, Pavel
diff --git a/lib/include/libv4l2.h b/lib/include/libv4l2.h index ea1870d..f170c3d 100644 --- a/lib/include/libv4l2.h +++ b/lib/include/libv4l2.h @@ -109,6 +109,12 @@ LIBV4L_PUBLIC int v4l2_get_control(int fd, int cid); (note the fd is left open in this case). */ LIBV4L_PUBLIC int v4l2_fd_open(int fd, int v4l2_flags); +/* v4l2_open_complex: open complex pipeline. Name of pipeline descriptor + is passed to v4l2_open_complex(). It opens devices described there and + handles mapping controls between devices. + */ +LIBV4L_PUBLIC int v4l2_open_complex(char *name, int v4l2_flags); + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h index 1924c91..1ee697a 100644 --- a/lib/libv4l2/libv4l2-priv.h +++ b/lib/libv4l2/libv4l2-priv.h @@ -104,6 +104,7 @@ struct v4l2_dev_info { void *plugin_library; void *dev_ops_priv; const struct libv4l_dev_ops *dev_ops; + struct v4l2_controls_map *map; }; /* From v4l2-plugin.c */ @@ -130,4 +131,20 @@ static inline void v4l2_plugin_cleanup(void *plugin_lib, void *plugin_priv, extern const char *v4l2_ioctls[]; void v4l2_log_ioctl(unsigned long int request, void *arg, int result); + +struct v4l2_control_map { + unsigned long control; + int fd; +}; + +struct v4l2_controls_map { + int main_fd; + int num_fds; + int num_controls; + struct v4l2_control_map map[]; +}; + +int v4l2_open_pipeline(struct v4l2_controls_map *map, int v4l2_flags); +LIBV4L_PUBLIC int v4l2_get_fd_for_control(int fd, unsigned long control); + #endif diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c index 2db25d1..39f69db 100644 --- a/lib/libv4l2/libv4l2.c +++ b/lib/libv4l2/libv4l2.c @@ -70,6 +70,8 @@ #include <sys/types.h> #include <sys/mman.h> #include <sys/stat.h> +#include <dirent.h> + #include "libv4l2.h" #include "libv4l2-priv.h" #include "libv4l-plugin.h" @@ -787,6 +789,8 @@ no_capture: if (index >= devices_used) devices_used = index + 1; + devices[index].map = NULL; + /* Note we always tell v4lconvert to optimize src fmt selection for our default fps, the only exception is the app explicitly selecting a frame rate using the S_PARM ioctl after a S_FMT */ @@ -1056,12 +1060,47 @@ static int v4l2_s_fmt(int index, struct v4l2_format *dest_fmt) return 0; } +int v4l2_get_fd_for_control(int fd, unsigned long control) +{ + int index = v4l2_get_index(fd); + struct v4l2_controls_map *map; + int lo = 0; + int hi; + + if (index < 0) + return fd; + + map = devices[index].map; + if (!map) + return fd; + hi = map->num_controls; + + while (lo < hi) { + int i = (lo + hi) / 2; + if (map->map[i].control == control) { + return map->map[i].fd; + } + if (map->map[i].control > control) { + hi = i; + continue; + } + if (map->map[i].control < control) { + lo = i+1; + continue; + } + printf("Bad: impossible condition in binary search\n"); + exit(1); + } + return fd; +} + int v4l2_ioctl(int fd, unsigned long int request, ...) { void *arg; va_list ap; int result, index, saved_err; - int is_capture_request = 0, stream_needs_locking = 0; + int is_capture_request = 0, stream_needs_locking = 0, + is_subdev_request = 0; va_start(ap, request); arg = va_arg(ap, void *); @@ -1076,18 +1115,20 @@ int v4l2_ioctl(int fd, unsigned long int request, ...) ioctl, causing it to get sign extended, depending upon this behavior */ request = (unsigned int)request; + /* FIXME */ if (devices[index].convert == NULL) goto no_capture_request; /* Is this a capture request and do we need to take the stream lock? */ switch (request) { - case VIDIOC_QUERYCAP: case VIDIOC_QUERYCTRL: case VIDIOC_G_CTRL: case VIDIOC_S_CTRL: case VIDIOC_G_EXT_CTRLS: - case VIDIOC_TRY_EXT_CTRLS: case VIDIOC_S_EXT_CTRLS: + is_subdev_request = 1; + case VIDIOC_QUERYCAP: + case VIDIOC_TRY_EXT_CTRLS: case VIDIOC_ENUM_FRAMESIZES: case VIDIOC_ENUM_FRAMEINTERVALS: is_capture_request = 1; @@ -1151,10 +1192,15 @@ int v4l2_ioctl(int fd, unsigned long int request, ...) } if (!is_capture_request) { + int sub_fd; no_capture_request: + sub_fd = fd; + if (is_subdev_request) { + sub_fd = v4l2_get_fd_for_control(index, ((struct v4l2_queryctrl *) arg)->id); + } result = devices[index].dev_ops->ioctl( devices[index].dev_ops_priv, - fd, request, arg); + sub_fd, request, arg); saved_err = errno; v4l2_log_ioctl(request, arg, result); errno = saved_err; @@ -1782,3 +1828,195 @@ int v4l2_get_control(int fd, int cid) (qctrl.maximum - qctrl.minimum) / 2) / (qctrl.maximum - qctrl.minimum); } + + +int v4l2_open_pipeline(struct v4l2_controls_map *map, int v4l2_flags) +{ + int index; + int i; + + for (i=0; i<map->num_controls; i++) { + if (map->map[i].fd <= 0) { + V4L2_LOG_ERR("v4l2_open_pipeline: Bad fd in map.\n"); + return -1; + } + if (i>=1 && map->map[i].control <= map->map[i-1].control) { + V4L2_LOG_ERR("v4l2_open_pipeline: Controls not sorted.\n"); + return -1; + } + } + + i = v4l2_fd_open(map->main_fd, v4l2_flags); + index = v4l2_get_index(map->main_fd); + devices[index].map = map; + return i; +} + +static void scan_devices(char **device_names, int *device_fds, int num) +{ + struct dirent **namelist; + int n; + char *class_v4l = "/sys/class/video4linux"; + + n = scandir(class_v4l, &namelist, NULL, alphasort); + if (n < 0) { + perror("scandir"); + return; + } + + while (n--) { + if (namelist[n]->d_name[0] != '.') { + char filename[1024], content[1024]; + sprintf(filename, "%s/%s/name", class_v4l, namelist[n]->d_name); + FILE *f = fopen(filename, "r"); + if (!f) { + printf("Strange, can't open %s", filename); + } else { + fgets(content, 1024, f); + fclose(f); + + int i; + for (i = num-1; i >=0; i--) { + if (!strcmp(content, device_names[i])) { + sprintf(filename, "/dev/%s", namelist[n]->d_name); + device_fds[i] = open(filename, O_RDWR); + if (device_fds[i] < 0) { + V4L2_LOG_ERR("Error opening %s: %m\n", filename); + } + } + } + } + } + free(namelist[n]); + } + free(namelist); + +} + +int v4l2_open_complex(char *name, int v4l2_flags) +{ +#define perr(s) V4L2_LOG_ERR("open_complex: " s "\n") +#define BUF 256 + FILE *f = fopen(name, "r"); + + int res = -1; + char buf[BUF]; + int version, num_modes, num_devices, num_controls; + int dev, control; + + if (!f) { + perr("open of .cv file failed: %m"); + goto err; + } + + if (fscanf(f, "Complex Video: %d\n", &version) != 1) { + perr(".cv file does not have required header"); + goto close; + } + + if (version != 0) { + perr(".cv file has unknown version"); + goto close; + } + + if (fscanf(f, "#modes: %d\n", &num_modes) != 1) { + perr("could not parse modes"); + goto close; + } + + if (num_modes != 1) { + perr("only single mode is supported for now"); + goto close; + } + + if (fscanf(f, "Mode: %s\n", buf) != 1) { + perr("could not parse mode name"); + goto close; + } + + if (fscanf(f, " #devices: %d\n", &num_devices) != 1) { + perr("could not parse number of devices"); + goto close; + } +#define MAX_DEVICES 16 + char *device_names[MAX_DEVICES] = { NULL, }; + int device_fds[MAX_DEVICES]; + if (num_devices > MAX_DEVICES) { + perr("too many devices"); + goto close; + } + + for (dev = 0; dev < num_devices; dev++) { + int tmp; + if (fscanf(f, "%d: ", &tmp) != 1) { + perr("could not parse device"); + goto free_devices; + } + if (tmp != dev) { + perr("bad device number"); + goto free_devices; + } + fgets(buf, BUF, f); + device_names[dev] = strdup(buf); + device_fds[dev] = -1; + } + + scan_devices(device_names, device_fds, num_devices); + + for (dev = 0; dev < num_devices; dev++) { + if (device_fds[dev] == -1) { + perr("Could not open all required devices"); + goto close_devices; + } + } + + if (fscanf(f, " #controls: %d\n", &num_controls) != 1) { + perr("can not parse number of controls"); + goto close_devices; + } + + struct v4l2_controls_map *map = malloc(sizeof(struct v4l2_controls_map) + + num_controls*sizeof(struct v4l2_control_map)); + + map->num_controls = num_controls; + map->num_fds = num_devices; + map->main_fd = device_fds[0]; + + for (control = 0; control < num_controls; control++) { + unsigned long num; + int dev; + if (fscanf(f, "0x%lx: %d\n", &num, &dev) != 2) { + perr("could not parse control"); + goto free_map; + } + if ((dev < 0) || (dev >= num_devices)) { + perr("device out of range"); + goto free_map; + } + map->map[control].control = num; + map->map[control].fd = device_fds[dev]; + } + if (fscanf(f, "%s", buf) > 0) { + perr("junk at end of file"); + goto free_map; + } + + res = v4l2_open_pipeline(map, v4l2_flags); + + if (res < 0) { +free_map: + free(map); +close_devices: + for (dev = 0; dev < num_devices; dev++) + close(device_fds[dev]); + } +free_devices: + for (dev = 0; dev < num_devices; dev++) { + free(device_names[dev]); + } +close: + fclose(f); +err: + return res; +} + diff --git a/lib/libv4lconvert/control/libv4lcontrol.c b/lib/libv4lconvert/control/libv4lcontrol.c index 59f28b1..c1e6f93 100644 --- a/lib/libv4lconvert/control/libv4lcontrol.c +++ b/lib/libv4lconvert/control/libv4lcontrol.c @@ -863,6 +863,7 @@ int v4lcontrol_vidioc_queryctrl(struct v4lcontrol_data *data, void *arg) struct v4l2_queryctrl *ctrl = arg; int retval; uint32_t orig_id = ctrl->id; + int fd; /* if we have an exact match return it */ for (i = 0; i < V4LCONTROL_COUNT; i++) @@ -872,8 +873,9 @@ int v4lcontrol_vidioc_queryctrl(struct v4lcontrol_data *data, void *arg) return 0; } + fd = v4l2_get_fd_for_control(data->fd, ctrl->id); /* find out what the kernel driver would respond. */ - retval = data->dev_ops->ioctl(data->dev_ops_priv, data->fd, + retval = data->dev_ops->ioctl(data->dev_ops_priv, fd, VIDIOC_QUERYCTRL, arg); if ((data->priv_flags & V4LCONTROL_SUPPORTS_NEXT_CTRL) && @@ -903,6 +905,7 @@ int v4lcontrol_vidioc_g_ctrl(struct v4lcontrol_data *data, void *arg) { int i; struct v4l2_control *ctrl = arg; + int fd; for (i = 0; i < V4LCONTROL_COUNT; i++) if ((data->controls & (1 << i)) && @@ -911,7 +914,8 @@ int v4lcontrol_vidioc_g_ctrl(struct v4lcontrol_data *data, void *arg) return 0; } - return data->dev_ops->ioctl(data->dev_ops_priv, data->fd, + fd = v4l2_get_fd_for_control(data->fd, ctrl->id); + return data->dev_ops->ioctl(data->dev_ops_priv, fd, VIDIOC_G_CTRL, arg); } @@ -994,6 +998,7 @@ int v4lcontrol_vidioc_s_ctrl(struct v4lcontrol_data *data, void *arg) { int i; struct v4l2_control *ctrl = arg; + int fd; for (i = 0; i < V4LCONTROL_COUNT; i++) if ((data->controls & (1 << i)) && @@ -1008,7 +1013,8 @@ int v4lcontrol_vidioc_s_ctrl(struct v4lcontrol_data *data, void *arg) return 0; } - return data->dev_ops->ioctl(data->dev_ops_priv, data->fd, + fd = v4l2_get_fd_for_control(data->fd, ctrl->id); + return data->dev_ops->ioctl(data->dev_ops_priv, fd, VIDIOC_S_CTRL, arg); }
Hi! Ok, can I get any comments on this one? v4l2_open_complex("/file/with/descriptor", 0) can be used to open whole pipeline at once, and work if it as if it was one device. Thanks, Pavel