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?
> 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? ;)
> 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?
> +
> /* Xv(3) adaptor properties */
> struct xv_adap {
> char name[128];
> @@ -133,20 +135,23 @@ struct dev {
> /* video encodingss */
> struct encodings {
> char *name;
> + int id;
> int bpp;
> - int xv_id;
> - int dev_id;
> #define SW_DEV 0x1
> #define SW_XV 0x2
> #define SW_MASK (SW_DEV | SW_XV)
> int flags;
> } encs[] = {
> #define ENC_YUY2 0
> - { "yuy2", 16, -1, -1, 0 },
> -#define ENC_UYVY 1
> - { "uyvy", 16, -1, -1, 0 },
> -#define ENC_LAST 2
> - { NULL, 0, 0, 0, 0 }
> + { "yuy2", V_V4L2_PIX_FMT_YUY2, 16, 0 },
> +#define ENC_YUYV 1
> + { "yuyv", V4L2_PIX_FMT_YUYV, 16, 0 },
> +#define ENC_UYVY 2
> + { "uyvy", V4L2_PIX_FMT_UYVY, 16, 0 },
> +#define ENC_YV12 3
> + { "yv12", V4L2_PIX_FMT_YVU420, 12, 0 },
> +#define ENC_LAST 4
> + { NULL, 0, 0, 0 }
> };
>
> struct video {
> @@ -163,8 +168,10 @@ struct video {
> int iofile_fd;
> char *sz_str;
> #define CONV_SWAP 0x1
> +#define CONV_YV12 0x2
> int conv_type;
> - int enc;
> + int enc_in;
> + int enc_out;
> int full_screen;
> int net_wm;
> int width;
> @@ -216,6 +223,7 @@ int stream(struct video *);
> void got_frame(int);
> void got_shutdown(int);
> int find_enc(char *);
> +int find_by_id(int);
> void usage(void);
>
> static volatile sig_atomic_t play, shutdown, hold, wout;
> @@ -242,6 +250,17 @@ find_enc(char *name)
> }
>
> int
> +find_enc_by_id(int id)
> +{
> + int i;
> +
> + for (i = 0; i < ENC_LAST; i++)
> + if (encs[i].id == id)
> + break;
> + return i;
> +}
> +
> +int
> xv_get_info(struct video *vid)
> {
> struct xdsp *x = &vid->xdsp;
> @@ -251,7 +270,6 @@ xv_get_info(struct video *vid)
> struct xv_adap *adap;
> unsigned int nenc, p;
> int num_xvformats, nadaps, i, j, ret;
> - char fmtName[5];
>
> if ((x->dpy = XOpenDisplay(NULL)) == NULL) {
> warnx("cannot open display %s", XDisplayName(NULL));
> @@ -312,16 +330,9 @@ xv_get_info(struct video *vid)
> &num_xvformats);
> adap->nfmts = 0;
> for (j = 0; j < num_xvformats; j++) {
> - snprintf(fmtName, sizeof(fmtName), "%c%c%c%c",
> - xvformats[j].id & 0xff,
> - (xvformats[j].id >> 8) & 0xff,
> - (xvformats[j].id >> 16) & 0xff,
> - (xvformats[j].id >> 24) & 0xff);
> - if (!strcmp(fmtName, "YUY2")) {
> - encs[ENC_YUY2].xv_id = xvformats[j].id;
> - adap->fmts[adap->nfmts++] = xvformats[j].id;
> - } else if (!strcmp(fmtName, "UYVY")) {
> - encs[ENC_UYVY].xv_id = xvformats[j].id;
> + int enc = find_enc_by_id(xvformats[j].id);
> + if (enc < ENC_LAST) {
> + encs[enc].flags |= SW_XV;
> adap->fmts[adap->nfmts++] = xvformats[j].id;
> }
> if (adap->nfmts >= MAX_FMTS)
> @@ -369,20 +380,20 @@ xv_sel_adap(struct video *vid)
> x->cur_adap = i;
> adap = &x->adaps[i];
> for (i = 0; i < adap->nfmts; i++) {
> - if (adap->fmts[i] == encs[vid->enc].xv_id)
> + if (adap->fmts[i] == encs[vid->enc_out].id)
What about a small diff that introduce `enc_out' and `enc_in' to
reduce the number of chunks in this one?
> break;
> }
> if (i >= adap->nfmts) {
> warnx("Xv adaptor '%d' doesn't support %s",
> x->adaps[x->cur_adap].adap_index,
> - encs[vid->enc].name);
> + encs[vid->enc_out].name);
> return 0;
> }
> }
> for (i = 0; i < x->nadaps && x->cur_adap == -1; i++) {
> adap = &x->adaps[i];
> for (j = 0; j < adap->nfmts; j++) {
> - if (adap->fmts[j] == encs[vid->enc].xv_id) {
> + if (adap->fmts[j] == encs[vid->enc_out].id) {
> x->cur_adap = i;
> break;
> }
> @@ -445,7 +456,7 @@ xv_dump_info(struct video *vid)
>
> fprintf(stderr, " encodings: ");
> for (i = 0, j = 0; i < ENC_LAST; i++) {
> - if (encs[i].xv_id != -1 && !(encs[i].flags & SW_XV)) {
> + if (encs[i].id != -1 && (encs[i].flags & SW_XV)) {
> if (j)
> fprintf(stderr, ", ");
> fprintf(stderr, "%s", encs[i].name);
> @@ -502,7 +513,7 @@ xv_init(struct video *vid)
>
> resize_window(vid, 0);
>
> - x->xv_image = XvCreateImage(x->dpy, x->port, encs[vid->enc].xv_id,
> + x->xv_image = XvCreateImage(x->dpy, x->port, encs[vid->enc_out].id,
> vid->frame_buffer, vid->width, vid->height);
>
> return 1;
> @@ -780,36 +791,20 @@ dev_get_encs(struct video *vid)
> fmtdesc.index = 0;
> fmtdesc.type = d->buf_type;
> while (ioctl(d->fd, VIDIOC_ENUM_FMT, &fmtdesc) >= 0) {
> - if (fmtdesc.pixelformat == V4L2_PIX_FMT_YUYV) {
> - i = find_enc("yuy2");
> - if (i < ENC_LAST)
> - encs[i].dev_id = fmtdesc.pixelformat;
> - }
> - if (fmtdesc.pixelformat == V4L2_PIX_FMT_UYVY) {
> - i = find_enc("uyvy");
> - if (i < ENC_LAST)
> - encs[i].dev_id = fmtdesc.pixelformat;
> + i = find_enc_by_id(fmtdesc.pixelformat);
> + if (i < ENC_LAST) {
> + encs[i].flags |= SW_DEV;
> }
> fmtdesc.index++;
> }
> for (i = 0; encs[i].name; i++) {
> - if (encs[i].dev_id != -1)
> + if (encs[i].flags & SW_DEV)
> break;
> }
> if (i >= ENC_LAST) {
> warnx("%s has no usable YUV encodings", d->path);
> return 0;
> }
> - if (encs[ENC_YUY2].dev_id == -1 &&
> - encs[ENC_UYVY].dev_id != -1) {
> - encs[ENC_YUY2].dev_id = encs[ENC_UYVY].dev_id;
> - encs[ENC_YUY2].flags |= SW_DEV;
> - }
> - if (encs[ENC_UYVY].dev_id == -1 &&
> - encs[ENC_YUY2].dev_id != -1) {
> - encs[ENC_UYVY].dev_id = encs[ENC_YUY2].dev_id;
> - encs[ENC_UYVY].flags |= SW_DEV;
> - }
>
> return 1;
> }
> @@ -824,7 +819,7 @@ dev_get_sizes(struct video *vid)
>
> nsizes = 0;
> fsize.index = 0;
> - fsize.pixel_format = encs[vid->enc].dev_id;
> + fsize.pixel_format = encs[vid->enc_in].id;
> while (ioctl(d->fd, VIDIOC_ENUM_FRAMESIZES, &fsize) == 0) {
> switch (fsize.type) {
> case V4L2_FRMSIZE_TYPE_DISCRETE:
> @@ -915,7 +910,7 @@ dev_get_rates(struct video *vid)
>
> for (i = 0; i < d->nsizes; i++) {
> bzero(&ival, sizeof(ival));
> - ival.pixel_format = encs[vid->enc].dev_id;
> + ival.pixel_format = encs[vid->enc_in].id;
> ival.width = d->sizes[i].w;
> ival.height = d->sizes[i].h;
> ival.index = 0;
> @@ -1072,7 +1067,7 @@ dev_dump_info(struct video *vid)
>
> fprintf(stderr, " encodings: ");
> for (i = 0, j = 0; i < ENC_LAST; i++) {
> - if (encs[i].dev_id != -1 && !(encs[i].flags & SW_DEV)) {
> + if (encs[i].flags & SW_DEV) {
> if (j)
> fprintf(stderr, ", ");
> fprintf(stderr, "%s", encs[i].name);
> @@ -1136,7 +1131,7 @@ dev_init(struct video *vid)
> fmt.type = d->buf_type;
> fmt.fmt.pix.width = vid->width;
> fmt.fmt.pix.height = vid->height;
> - fmt.fmt.pix.pixelformat = encs[vid->enc].dev_id;
> + fmt.fmt.pix.pixelformat = encs[vid->enc_in].id;
> fmt.fmt.pix.field = V4L2_FIELD_ANY;
> if (ioctl(d->fd, VIDIOC_S_FMT, &fmt) < 0) {
> warn("VIDIOC_S_FMT");
> @@ -1290,57 +1285,67 @@ choose_enc(struct video *vid)
> {
> int i;
>
> - if (vid->enc < 0) {
> - for (i = 0; vid->enc < 0 && i < ENC_LAST; i++) {
> - if ((vid->mode & M_IN_DEV) && (vid->mode & M_OUT_XV)) {
> - if (encs[i].dev_id != -1 &&
> - encs[i].xv_id != -1 &&
> - (encs[i].flags & SW_MASK) == 0)
> - vid->enc = i;
> - } else if (vid->mode & M_IN_DEV) {
> - if (encs[i].dev_id != -1 &&
> - (encs[i].flags & SW_MASK) == 0)
> - vid->enc = i;
> - } else if (vid->mode & M_OUT_XV) {
> - if (encs[i].xv_id != -1 &&
> - (encs[i].flags & SW_MASK) == 0)
> - vid->enc = i;
> - }
> - }
> - for (i = 0; vid->enc < 0 && i < ENC_LAST; i++) {
> - if ((vid->mode & M_IN_DEV) && (vid->mode & M_OUT_XV)) {
> - if (encs[i].dev_id != -1 &&
> - encs[i].xv_id != -1)
> - vid->enc = i;
> - } else if (vid->mode & M_IN_DEV) {
> - if (encs[i].dev_id != -1)
> - vid->enc = i;
> - } else if (vid->mode & M_OUT_XV) {
> - if (encs[i].xv_id != -1)
> - vid->enc = i;
> + if (vid->enc_in < 0) {
> + for (i = 0; vid->enc_in < 0 && i < ENC_LAST; i++) {
> + if (encs[i].flags & SW_DEV)
> + vid->enc_in = i;
> + }
> + }
> +
> + if (vid->enc_out < 0) {
> + for (i = 0; vid->enc_out < 0 && i < ENC_LAST; i++) {
> + if (encs[i].flags & SW_XV)
> + vid->enc_out = i;
> + }
> + }
> +
> + if ((vid->mode & M_IN_DEV) && (vid->mode & M_OUT_XV)) {
> + for (i = 0; vid->enc_in < 0 && i < ENC_LAST; i++) {
> + if ((encs[i].flags & SW_DEV) &&
> + (encs[i].flags & SW_XV)) {
> + vid->enc_in = i;
> + vid->enc_out = i;
> }
> }
> }
> - if (vid->enc < 0) {
> +
> + if (vid->enc_in < 0 && vid->mode & M_IN_FILE)
> + vid->enc_in = ENC_YUY2;
> +
> + if (vid->mode & M_OUT_XV && vid->enc_in != vid->enc_out) {
> + /* check if conversion is possible */
> + int enc_in = (vid->enc_in == ENC_YUYV) ? ENC_YUY2 : vid->enc_in;
> + int enc_out = (vid->enc_out == ENC_YUYV) ? ENC_YUY2 :
> vid->enc_out;
Please define your variable first then do the assignment. This will
also make your lines fit in 80 chars :)
> +
> + if (enc_in == enc_out)
> + vid->conv_type = 0; /* same pixelformat, different id */
Could we use a define for no conversion? CONV_NONE? This would
implicitly document your code so you can get rid of the comment above :)
> + else if ((enc_in == ENC_YUY2 && enc_out == ENC_UYVY) ||
> + (enc_in == ENC_UYVY && enc_out == ENC_YUY2))
> + vid->conv_type = CONV_SWAP;
> + else if (enc_in == ENC_YUY2 && enc_out == ENC_YV12)
> + vid->conv_type = CONV_YV12;
> + else {
> + warn("can't convert from %s to %s",
> + encs[vid->enc_in].name, encs[vid->enc_out].name);
Is it impossible to convert or is it unsupported?
> + return 0;
> + }
> + }
> +
> + if (vid->enc_in < 0) {
> warnx("could not find a usable encoding");
> return 0;
> }
> - if ((vid->mode & M_IN_DEV) && encs[vid->enc].dev_id == -1) {
> + if ((vid->mode & M_IN_DEV) && !(encs[vid->enc_in].flags & SW_DEV)) {
> warnx("device %s can't supply %s", vid->dev.path,
> - encs[vid->enc].name);
> + encs[vid->enc_in].name);
> return 0;
> }
> - if ((vid->mode & M_OUT_XV) && encs[vid->enc].xv_id == -1) {
> + if ((vid->mode & M_OUT_XV) && !(encs[vid->enc_out].flags & SW_XV)) {
> warnx("Xv adaptor %d can't display %s",
> vid->xdsp.adaps[vid->xdsp.cur_adap].adap_index,
> - encs[vid->enc].name);
> + encs[vid->enc_out].name);
> return 0;
> }
> - if (((vid->mode & M_IN_DEV) &&
> - (encs[vid->enc].flags & SW_MASK) == SW_DEV) ||
> - ((vid->mode & M_OUT_XV) &&
> - (encs[vid->enc].flags & SW_MASK) == SW_XV))
> - vid->conv_type = CONV_SWAP;
>
> return 1;
> }
> @@ -1469,14 +1474,14 @@ setup(struct video *vid)
> if (!parse_size(vid) || !choose_size(vid))
> return 0;
>
> - vid->bpf = vid->width * vid->height * encs[vid->enc].bpp / NBBY;
> + vid->bpf = (vid->width * vid->height * encs[vid->enc_in].bpp) / NBBY;
>
> if (vid->verbose > 0) {
> if (vid->mode & M_IN_DEV)
> dev_dump_info(vid);
> if (vid->mode & M_OUT_XV)
> xv_dump_info(vid);
> - fprintf(stderr, "using %s encoding\n", encs[vid->enc].name);
> + fprintf(stderr, "using %s encoding\n", encs[vid->enc_in].name);
> fprintf(stderr, "using frame size %dx%d (%d bytes)\n",
> vid->width, vid->height, vid->bpf);
> if (vid->fps)
> @@ -1491,12 +1496,7 @@ setup(struct video *vid)
> }
>
> if (vid->conv_type) {
> - if (vid->conv_type == CONV_SWAP) {
> - vid->conv_bufsz = vid->bpf;
> - } else {
> - warnx("invalid conversion type");
> - return 0;
> - }
> + vid->conv_bufsz = (vid->width * vid->height *
> encs[vid->enc_out].bpp) / NBBY;
Please reformat this line to fit in 80 chars :)
> if ((vid->conv_buffer = calloc(1, vid->conv_bufsz)) == NULL) {
> warn("conv_buffer");
> return 0;
> @@ -1539,8 +1539,9 @@ ioctl_input(struct video *vid)
> }
>
> /* copy frame buffer */
> - if (buf.bytesused > vid->bpf)
> + if (buf.bytesused > vid->bpf) {
> return 0;
> + }
> memcpy(vid->frame_buffer, vid->mmap_buffer[buf.index], buf.bytesused);
>
> /* requeue buffer */
> @@ -1632,6 +1633,29 @@ got_shutdown(int s)
> shutdown = 1;
> }
>
> +int yuy2_to_yv12(uint8_t *src, uint8_t *dst, int width, int height) {
This function definition should respect style(9) :)
> + int i, j;
> + int iy = 0, iu, h1, h2, c;
> + int bpf = width * height * 2;
> +
> + for (i = 0; i < bpf; i+= 2)
> + dst[iy++] = src[i]; //Y component
Please do not use C++-style comments. You could also pick better name
for your variables, if `i' represents the rows and `j' the columns then
use `row' and `col' ;) What does `c' represent? And the others?
> + iu = iy + bpf / 8;
> + for (i = 0; i < height / 2; i++) { //row
> + c = i * width * 4;
> + for (j = 0; j < width / 2; j++) { //col
> + h1 = c + j * 4 + 1;
> + h2 = h1 + width * 2;
> + dst[iu++] = (src[h1] + src[h2]) / 2;
> + h1+= 2; h2+= 2;
> + dst[iy++] = (src[h1] + src[h2]) / 2;
> + }
> + }
> +
> + return 0;
> +}
> +
> int
> stream(struct video *vid)
> {
> @@ -1721,7 +1745,8 @@ stream(struct video *vid)
> src = vid->frame_buffer;
> if (vid->conv_type == CONV_SWAP) {
> swab(src, vid->conv_buffer, vid->bpf);
> - src = vid->conv_buffer;
> + } else if (vid->conv_type == CONV_YV12) {
> + yuy2_to_yv12(src, vid->conv_buffer, vid->width,
> vid->height);
> }
>
> if ((vid->mode & M_OUT_FILE) && wout) {
> @@ -1743,6 +1768,7 @@ stream(struct video *vid)
> }
> }
> if (vid->mode & M_OUT_XV) {
> + src = (vid->conv_type) ? vid->conv_buffer :
> vid->frame_buffer;
Please reformat this line to fit in 80 chars ;)
> x->xv_image->data = src;
> if (x->resized) {
> x->resized = 0;
> @@ -1862,7 +1888,8 @@ main(int argc, char *argv[])
> x->cur_adap = -1;
> vid.dev.fd = vid.iofile_fd = -1;
> vid.mode = M_IN_DEV | M_OUT_XV;
> - vid.enc = -1;
> + vid.enc_in = -1;
> + vid.enc_out = -1;
> vid.mmap_on = 1; /* mmap method is default */
> wout = 1;
>
> @@ -1876,8 +1903,8 @@ main(int argc, char *argv[])
> }
> break;
> case 'e':
> - vid.enc = find_enc(optarg);
> - if (vid.enc >= ENC_LAST) {
> + vid.enc_in = find_enc(optarg);
> + if (vid.enc_in >= ENC_LAST) {
> warnx("encoding '%s' is invalid", optarg);
> errs++;
> }
>
>
