Re: [Qemu-devel] [PATCH] ui/vnc: VA API based H.264 encoding for VNC framebuffer updates

[Verbeiren, David]

Thanks for your comments, Anthony.

 Anthony Liguori wrote:
> David Verbeiren <address@hidden> writes:
> > This patch provides the VNC server side support. Corresponding VNC
> > client side support is required. To this end, we are also contributing a 
> > patch
> > to the libvncserver project (more specifically its libvncclient subproject)
> > which can be used to test this experimental feature.
> FWIW, the most common VNC client used with QEMU is gtk-vnc.  That's the
> widget used by vinagre, GNOME Boxes, and the various virt-tools
> (virt-manager, virt-viewer).
> 
> Implementing support in gtk-vnc isn't a prerequisite for merging into
> QEMU, but thought you might be interested in the above.

That's a good point. We'll look at enabling gtk-vnc as well.

> You appear to be using an unallocated encoding number though.  You can
> get your own encoding number by contacting Tristan Richardson from
> RealVNC or I'd be happy to use one of my encoding numbers...

Thanks for pointing that out. I'll try to get an encoding number
allocated for this.

> > +    /* Open local X11 display and VA display */
> > +    h264->x11_dpy = XOpenDisplay(":0.0");
> > +    if (!h264->x11_dpy) {
> > +        fprintf(stderr, "Could not open display :0.0 for VA encoding\n");
> > +        return -1;
> > +    }
> > +    int va_major_ver, va_minor_ver;
> > +    h264->va_dpy = vaGetDisplay(h264->x11_dpy);
> > +    va_status = vaInitialize(h264->va_dpy, &va_major_ver, &va_minor_ver);
> > +    CHECK_VASTATUS(va_status, "vaInitialize");
>
> So you need an X server in order to use libva?  That's a little disappointing 
> as it
> would be nice to use this acceleration on a server without X installed. 
> Is there any way to avoid the X interaction?

I believe libva versions currently under development allow headless
operation and removed that dependency on X (e.g. for Wayland support).
So this requirement will go away at some point.
We could conditionally compile for different versions of libva but that
would make the code quite messy...

> > +    static struct {
> > +        unsigned char *data;
> > +        int length;
> > +        int is_intra;
> > +        unsigned long count;
> > +    } frame = { .data = NULL, .length = -1, .is_intra = -1, .count =
> > -1 };
>
> Why is this static?  We can have multiple clients connected at once so I'm 
> pretty sure
> this would break really badly.

The way support for multiple clients is implemented is by sending the
same H264 data to all clients. This avoids having to run encoding
(the heavy part) for each.
When a new client connects, we force sending an I-frame so it will get a
self-contained frame to start from.
Hence this data is in fact unique per qemu process by design.
We'll look at making this cleaner though.

> > +        if (!vs->vd->h264.va_dpy || (vs->vd->h264.config == 
> > VA_INVALID_ID)) {
> > +            if (h264_encoder_init(&vs->vd->h264) != 0) {
> > +                fprintf(stderr, "Failed to initialize VA H264 encoder\n");
> > +                return 0;
> > +            }
> Since we really can't guarantee that libva will initialize okay, we need
> to more gracefully handle falling back to another encoding type.

I'll see if we can move the critical parts of init to an earlier stage
where we could still fall back.

> Is this pseudo-encoding documented somewhere publicly?
> It should be if it already isn't.

No, it currently isn't. Any recommendation on where this could go?

> It looks mostly okay otherwise.

I'll follow your recommendation on the other points you raised and
submit an updated patch.

Thanks,
-David
Intel Corporation NV/SA
Kings Square, Veldkant 31
2550 Kontich
RPM (Bruxelles) 0415.497.718. 
Citibank, Brussels, account 570/1031255/09

This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). Any review or distribution by others is 
strictly prohibited. If you are not the intended recipient, please contact the 
sender and delete all copies.