[Spice-devel] [spice-gtk v1 2/2] channel-display: use libva to set preferred video codecs

[Victor Toso]

Hi,

On Mon, Feb 19, 2018 at 02:58:22PM +0100, Christophe Fergeau wrote:
> On Thu, Feb 15, 2018 at 10:05:05AM +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> > 
> > By using the detection of which video codecs (and profiles!) the
> > hardware supports with VA-API capable driver in previous commit.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-display.c | 33 ++++++++++++++++++++++++++++-----
> >  1 file changed, 28 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 45fe38c..7f47ae7 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -175,6 +175,8 @@ static void spice_display_channel_finalize(GObject *object)
> >          G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize(object);
> >  }
> >  
> > +static void spice_display_send_client_preferred_video_codecs(SpiceChannel *channel, const GArray *codecs);
> > +
> >  static void spice_display_channel_constructed(GObject *object)
> >  {
> >      SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(object)->priv;
> > @@ -572,6 +574,9 @@ static void spice_display_send_client_preferred_video_codecs(SpiceChannel *chann
> >      SpiceMsgOut *out;
> >      SpiceMsgcDisplayPreferredVideoCodecType *msg;
> >  
> > +    if (codecs == NULL || codecs->len == 0)
> > +        return;
> > +
> >      msg = g_malloc0(sizeof(SpiceMsgcDisplayPreferredVideoCodecType) +
> >                      (sizeof(SpiceVideoCodecType) * codecs->len));
> >      msg->num_of_codecs = codecs->len;
> > @@ -615,7 +620,9 @@ void spice_display_change_preferred_video_codec_type(SpiceChannel *channel, gint
> >   */
> >  void spice_display_channel_change_preferred_video_codec_type(SpiceChannel *channel, gint codec_type)
> >  {
> > +    SpiceSession *session;
> >      GArray *codecs;
> > +    const GArray *hwa_codecs;
> >  
> >      g_return_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel));
> >      g_return_if_fail(codec_type >= SPICE_VIDEO_CODEC_TYPE_MJPEG &&
> > @@ -626,13 +633,23 @@ void spice_display_channel_change_preferred_video_codec_type(SpiceChannel *chann
> >          return;
> >      }
> >  
> > -    /* FIXME: We should detect video codecs that client machine can do hw
> > -     * decoding, store this information (as GArray) and send it to the server.
> > -     * This array can be rearranged to have @codec_type in the front (which is
> > -     * the preferred for the client side) */
> > -    CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", gst_opts[codec_type].name);
> > +    session = spice_channel_get_session(channel);
> > +    hwa_codecs = spice_session_get_hw_accel_video_codecs(session);
> > +
> >      codecs = g_array_new(FALSE, FALSE, sizeof(gint));
> >      g_array_append_val(codecs, codec_type);
> > +    if (hwa_codecs != NULL) {
> > +        gint i;
> > +        for (i = 0; i < hwa_codecs->len; i++) {
> > +            gint hwa_codec_type = g_array_index(hwa_codecs, gint, i);
> > +            if (hwa_codec_type == codec_type)
> > +                continue;
> > +
> > +            g_array_append_val(codecs, hwa_codec_type);
> > +        }
> > +    }
> > +
> > +    CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", gst_opts[codec_type].name);
> 
> So this is unconditionally appending all hw accelerated codecs
> to the preferred video codec type? If user asks for codec A,
> and it's not available server side, I assume before we were
> getting an error/a blank screen, and now we'd be falling back
> to codec B or C?

Okay, this is actually pretty complex with some support lacking
yet in the host.

First, the client's request is just a matter of preference,
really, it should not mean that its preference is absolute.
Instead, this should be used as a hint to the server on what
should be best to encode a video stream with, so, if client
requests A but server does not have it, no stream will ever be
encoded with A.

Second, if the current stream is set with A and for some reason
there is a problem, based on stream-report (like all frames
dropped), server should fall back to another video-codec. I do
expect glitches, black stream, etc. depending on how that
video-codec + parameters handle errors.

Third, what I mean at the beginning, we still lack support in the
server+qemu on how to manage the *host preference* for encoding,
which means that today, with upstream, client gets what it wants
if server can encode with it but it'll mostly likely change
in near future.

Finally, you are right that we don't check at the client about
server's capabilities on encoding. Server does not set its
SPICE_DISPLAY_CAP_CODEC_*, only client does that. I think it
makes sense the server to set it too, so we can avoid requesting
something that server cannot handle.

But yeah, this patches are kinda ignoring the video-codec caps
and I'll fix it in the next version!

Thanks again for the review,
        toso


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180219/972396fd/attachment-0001.sig>