On Fri, Jan 25, 2019 at 12:52:38PM +0200, Artturi Alm wrote:
> On Fri, Jan 25, 2019 at 11:16:49AM +0100, Raphael Graf wrote:
> > Hi Martin,
> > Thanks for your feedback!
> >
> > On Wed, Jan 23, 2019 at 12:54:51PM -0200, Martin Pieuchot wrote:
> > > Hello Raphael,
> > >
> > > On 16/01/19(Wed) 12:41, Raphael Graf wrote:
> > > > Here is an attempt to make video(1) work with the modesetting driver.
> > > > See https://marc.info/?l=openbsd-bugs&m=152231686416039&w=2
> > > >
> > > > The general idea:
> > > > If there is no common encoding for input (device) and output (Xv), the
> > > > encoding is converted to something supported by the output.
> > > > No conversion is done if the output is sent to file (options -o/-O).
> > > >
> > > > The option -e is now used as the input format. The output format
> > > > is selected automatically (if output goes to Xv).
> > > >
> > > > The following encodings are supported: yuy2, yuyv, uyuy, yv12
> > >
> > > Did you test all of them?
> >
> > Yes, I have tested all encodings with these two devices:
> >
> > uvideo0 at uhub0 port 6 configuration 1 interface 0
> > "Chicony Electronics Co., Ltd. product 0x480c" rev 2.00/31.34 addr 2
> >
> > uvideo0 at uhub1 port 8 configuration 1 interface 0 "SC20A38485AA4629RX
> > Integrated Camera" rev 2.00/0.21 addr 4
> >
> > >
> > > > yuy2 and yuyv are treated as two different encodings, they share the
> > > > same
> > > > pixelformat, but have different ids. (This behaviour is different from
> > > > the
> > > > current description of '-e' in the manpage)
> > >
> > > What about fixing the manpage then? ;)
> >
> > The new diff below contains the changes to the manpage.
> >
> > >
> > > > I have tested on two lenovo laptops using both drivers (modesetting and
> > > > intel).
> > > >
> > > > On my laptop (lenovo X1 Carbon), with modesetting diver:
> > > > yuyv is converted to yv12 before sending to Xv.
> > > >
> > > > On the same laptop With intel driver:
> > > > yuyv is converted to yuy2 (no-op)
> > >
> > > Nice work, some comments below.
> > >
> > > Make sure the code you're writing follows style(9) :o)
> > >
> > > > Index: video.c
> > > > ===================================================================
> > > > RCS file: /cvs/xenocara/app/video/video.c,v
> > > > retrieving revision 1.26
> > > > diff -u -p -u -p -r1.26 video.c
> > > > --- video.c 4 Jan 2019 17:45:00 -0000 1.26
> > > > +++ video.c 16 Jan 2019 11:30:21 -0000
> > > > @@ -38,6 +38,8 @@
> > > > #include <X11/extensions/Xvlib.h>
> > > > #include <X11/Xatom.h>
> > > >
> > > > +#define V_V4L2_PIX_FMT_YUY2 v4l2_fourcc('Y', 'U', 'Y', '2')
> > >
> > > Why do we need this define and not the others? Aren't we missing an
> > > include?
> >
> > Actually, I don't know why this define is missing in videoio.h.
> > This YUY2 format (id:0x32595559) is reported by Xv.
> > The uvideo(4) driver reports YUYV which has a define in videoio.h.
> > (YUY2 and YUYV are equivalent)
> >
> > In the new diff, I have changed the variable names enc_in/enc_out to
> > enc/enc_xv
> > as the second 'enc' is only used for Xv output.
> > I also fixed the style(9) issues and added a CONV_NONE define.
> >
> > Note that I'm not sure if the algorithm used in the yuy2_to_yv12 function is
> > correct in respect to color theory..
> > (It is a conversion from 16 to 12 bits/pixel)
> >
> > Sorry, it is still a big diff, feedback is welcome.
> >
> >
>
> Hi,
>
> got one question below.
>
> -Artturi
>
> [...snip...]
>
> > @@ -1633,6 +1631,34 @@ got_shutdown(int s)
> > }
> >
> > int
> > +yuy2_to_yv12(uint8_t *src, uint8_t *dst, int width, int height)
> > +{
> > + int row, col, c, o;
> > + int iy = 0;
> > + int iu = width * height;
> > + int iv = iu + iu / 4;
> > +
> > + for (row = 0; row < height * 2; row += 2) {
> > + c = row * width;
> > + for (col = 0; col < width * 2 ; col += 4) {
> > + o = c + col;
> > + dst[iy++] = src[o];
> > + dst[iy++] = src[o + 2];
> > + if ((row & 0x03) == 0) {
> > + o++;
> > + dst[iv++] = (src[o] + src[o + width * 2]) / 2;
> > + o += 2;
> > + dst[iu++] = (src[o] + src[o + width * 2]) / 2;
> > + if (o + width * 2 >= iu*2)
> > + printf("hello\n");
>
> is that a placeholder for something relevant, or what does hello mean here?
Oups, sorry, can you ignore that for now? I'll remove this in the next
iteration.
>
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int
> > stream(struct video *vid)
> > {
> > struct xdsp *x = &vid->xdsp;
>
> [...snip...]
