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

[Anthony Liguori]

"Verbeiren, David" <address@hidden> writes:

> 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?

The folks at TigerVNC maintain a community spec that includes extensions
that are not part of the original specification:

http://sourceforge.net/apps/mediawiki/tigervnc/index.php?title=Welcome_to_TigerVNC#RFB_Protocol

Regards,

Anthony Liguori

>
>> 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.