Re: [PATCH v2] [media] uvcvideo: Add quirk to force the Oculus DK2 IR tracker to grayscale

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philipp,

On Monday 06 October 2014 23:45:54 Philipp Zabel wrote:
> On Mon, Oct 6, 2014 at 4:34 PM, Laurent Pinchart wrote:
> >> > @@ -311,6 +311,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> >> >         struct uvc_format_desc *fmtdesc;
> >> >         struct uvc_frame *frame;
> >> >         const unsigned char *start = buffer;
> >> > +       bool force_yuy2_to_y8 = false;
> > 
> > To keep things a bit more generic, how about an unsigned int
> > width_multiplier initialized to 1 and set to 2 when the quirk applies ?
> 
> [...]
> 
> >> > @@ -333,6 +334,22 @@ static int uvc_parse_format(struct uvc_device
> >> > *dev,
> >> > 
> >> >                 /* Find the format descriptor from its GUID. */
> >> >                 fmtdesc = uvc_format_by_guid(&buffer[5]);
> >> > 
> >> > +               format->bpp = buffer[21];
> >> > +
> >> > +               if (dev->quirks & UVC_QUIRK_FORCE_Y8) {
> >> > +                       if (fmtdesc && fmtdesc->fcc ==
> >> > V4L2_PIX_FMT_YUYV
> >> > &&
> >> > +                           format->bpp == 16) {
> > 
> > I wonder if that check is really needed, all YUYV formats should have
> > 16bpp.
> >
> >> > +                               force_yuy2_to_y8 = true;
> >> > +                               fmtdesc = &uvc_fmts[9];
> > 
> > The hardcoded index here is hair-raising :-) How about something like the
> > following instead ?
> > 
> >                 }
> >                 
> >                 format->bpp = buffer[21];
> > 
> > +
> > +               /* Some devices report a format that doesn't match what
> > they
> > +                * really send.
> > +                */
> > +               if (dev->quirks & UVC_QUIRK_FORCE_Y8) {
> > +                       if (format->fcc == V4L2_PIX_FMT_YUYV) {
> > +                               strlcpy(format->name, "Greyscale 8-bit (Y8
> >  )",
> >  +                                       sizeof(format->name));
> > +                               format->fcc = V4L2_PIX_FMT_GREY;
> > +                               format->bpp = 8;
> > +                               width_multiplier = 2;
> > +                       }
> > +               }
> > +
> > 
> >                 if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
> >                         ftype = UVC_VS_FRAME_UNCOMPRESSED;
> >                 } else {
> > 
> > I know it duplicates the format string, but as we're trying to move them
> > to the V4L2 core anyway, I don't see that as being a big problem.
> 
> [...]
> 
> >> > @@ -455,6 +471,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >> > 
> >> >                 frame->bFrameIndex = buffer[3];
> >> >                 frame->bmCapabilities = buffer[4];
> >> >                 frame->wWidth = get_unaligned_le16(&buffer[5]);
> >> > 
> >> > +               if (force_yuy2_to_y8)
> >> > +                       frame->wWidth *= 2;
> > 
> > This would become
> > 
> > +               frame->wWidth = get_unaligned_le16(&buffer[5])
> > +                             * width_multiplier;
> > 
> > If you're fine with that there's no need to resubmit, I'll modify the
> > patch when applying it to my tree.
> 
> Thank you, I'm fine with your suggested changes.
> Especially the format setting part looks a lot more civilized now.

Great. I'll add the patch to my next pull request. Thank you.

-- 
Regards,

Laurent Pinchart