Re: [RFC, libv4l]: Make libv4l2 usable on devices with complex pipeline

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]