[Spice-devel] [spice-common v1] protocol: add preferred video codec message

[Victor Toso]

Hi,

On Mon, Oct 24, 2016 at 11:06:10AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > Client might want to choose a preferred video codec for streaming for
> > different reasons which having hardware decoder support being the most
> > interest one.
> > 
> > This message allows the client to send a combination of video codec
> > type with a ranking value of SpiceVideoCodecRank.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  common/messages.h | 10 ++++++++++
> >  spice.proto       | 18 ++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/common/messages.h b/common/messages.h
> > index 516a345..7bc90fd 100644
> > --- a/common/messages.h
> > +++ b/common/messages.h
> > @@ -648,6 +648,16 @@ typedef struct SpiceMsgcDisplayPreferredCompression {
> >      uint8_t image_compression;
> >  } SpiceMsgcDisplayPreferredCompression;
> >  
> > +typedef struct SpiceVideoCodecPreferredRank {
> > +    uint8_t type;
> > +    uint8_t rank;
> > +} SpiceVideoCodecPreferredRank;
> > +
> > +typedef struct SpiceMsgcDisplayPreferredVideoCodecType {
> > +    uint32_t num_of_codecs;
> > +    SpiceVideoCodecPreferredRank codec_ranks[0];
> > +} SpiceMsgcDisplayPreferredVideoCodecType;
> > +
> >  typedef struct SpiceMsgDisplayGlScanoutUnix {
> >      int drm_dma_buf_fd;
> >      uint32_t width;
> > diff --git a/spice.proto b/spice.proto
> > index 0bfc515..c4f1a62 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -706,6 +706,19 @@ flags32 gl_scanout_flags {
> >      Y0TOP
> >  };
> >  
> > +enum8 video_codec_rank {
> > +    DISABLED = 0,
> > +    SOFTWARE_DECODER,
> > +    HARDWARE_DECODER,
> > +    BOTH,
> > +    PREFERRED,
> > +};
> > +
>
> Is it an order?

It is.

> It's not clear what should a server do with these.  The software and
> hardware (and both) tell the server if the client has software and/or
> hardware... but what about preferred?

The idea is that higher rank value has bigger preference. It could be
simply integers from 0 (disabled) -> 4 (preferred) but I named it in a
way that I thought would make sense: HW decoder usually have preference
then SW decoder so I put it in this order.

The idea of preferred is to be a single value that client would prefer.
Current spice-gtk code (in this patches) only try to set the preferred
one.

> The first 4 can be represented with 2 flags (hardware and software)
> but preferred it's something that is not clear in this list.

Yes, I can change to flags.

I hope the preferred makes sense otherwise it is hard to chose a
particularly video codec for streaming.


> The server should have its preference too. Do you have an idea
> which should be these preference and how to match them.

I agree. This patch set introduces SpiceVideoCodecRank but I think it
makes sense to change it to SpiceVideoCodecClientRank to be clear that
this is the client-side rank.

We could do something similar in server-side - if we need to export it,
could be SpiceVideoCodecServerRank or something. Server rank should
always take higher priority then client rank.

It would not be so hard to implement, we already have API that
initializes encoder/video-codec based in the order they are in the API;
This patch set sort it based on client-side rank but we can improve the
rank callback to do it properly, considering the server rank.

> For instance an administrator can decide to disable an hardware
> encoder for licensing issues and because the driver for some
> reason is buggy. Or decide the order based on network performances
> or cpu (which is usually quite different).

Indeed

> And how to choose which one is more important, if client or server
> settings?

IMO, the client-side is always a *preference* settings. If server does
not have any restrictions, it should follow the client preference.

>
> > +struct VideoCodecPreferredRank {
> > +    video_codec_type type;
> > +    video_codec_rank rank;
> > +};
> > +
> >  channel DisplayChannel : BaseChannel {
> >   server:
> >      message {
> > @@ -984,6 +997,11 @@ channel DisplayChannel : BaseChannel {
> >      } preferred_compression;
> >  
> >      message {
> > +        uint32 num_of_codecs;
> > +        VideoCodecPreferredRank codec_ranks[num_of_codecs] @end;
> > +    } preferred_video_codec_type;
> > +
> 
> This message needs to be after gl_draw_done

I'll change it and also rename the enum. Do you think it should be a
flag? Let me know if the naming is okay (I'm terrible with names)

Thanks for the review,
  toso

> 
> > +    message {
> >      } gl_draw_done;
> >  };
> >  
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161024/92c87aa5/attachment.sig>