[Spice-devel] [PATCH v6 08/26] server: Add VP8 support to the GStreamer encoder

[Christophe Fergeau]

On Mon, Oct 26, 2015 at 09:06:03PM +0100, Francois Gouget wrote:
> On Thu, 22 Oct 2015, Christophe Fergeau wrote:
> 
> > On Wed, Oct 14, 2015 at 05:32:03PM +0200, Francois Gouget wrote:
> > > The Spice server administrator can specify the preferred encoder and
> > > codec preferences to optimize for CPU or bandwidth usage. Preferences
> > > are described in a semi-colon separated list of encoder:codec pairs.
> > > For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
> > > VP8 video encoder and the original MJPEG video encoder as a fallback.
> > > The server has a default preference list which can also be selected by
> > > specifying 'auto' as the preferences list.
> > 
> > Note: All this paragraph describes a very different thing than what the
> > shortlog says "server: Add VP8 support to the GStreamer encoder". Imo
> > they are 2 distinct things, the addition of
> > spice_server_set_video_codecs() and the addition of vp8 support to the
> > gstreamer encoder, ie they should be (at least) 2 different commits. It
> > might even make sense to move the client capability checks to a 3rd
> > commit.
> 
> These are technically independent but conceptually they all depend on 
> each other:
>  * Adding VP8 support without a way to request using VP8 instead of 
>    MJPEG would be equivalent to adding dead code. Furthermore if 
>    compiling in GStreamer support forced use of VP8 then that server 
>    would be incompatible with most clients so checking the client caps 
>    is really necessary.
> 
>  * Committing a way to pick the encoder and codec when the only 
>    codec is MJPEG does not make sense either. This could degenerate to 
>    code that only lets the administrator pick the encoder (i.e. builting 
>    or gstreamer), but the next patch would have to add support for the 
>    codec and would essentially be just as big.
> 

I would add the whole parsing code from red_dispatcher.c in separate
commit(s). They are quite pointless without VP8 support, but this is
still some preparatory work which is logically independant from the VP8
support. It's better to have it separate imo, better when reviewing,
better when bisecting, ...

>  * Committing the CAPS checking code first could work, though it would 
>    be somewhat pointless and would not even shave a couple of lines off 
>    this one.
> 
> 
> > When adding use of SPICE_DISPLAY_CAP_MULTI_CODEC , I would raise the
> > spice-protocol requirement in configure.ac to 0.12.11 (and do a
> > post-release bump in spice-protocol git).
> 
> I have bumped the requirement to 0.12.11 in configure.ac but I believe 
> the post-release bump in spice-protocol git has to be left to you.

It was actually done a while ago in spice-protocol git
http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=017ddbe7a7c73816f068527531

> 
> > > @@ -1349,10 +1349,12 @@ static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder,
> > >      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> > >  }
> > >  
> > > -VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> > > +VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> > > +                                uint64_t starting_bit_rate,
> > >                                  VideoEncoderRateControlCbs *cbs,
> > >                                  void *cbs_opaque)
> > >  {
> > > +    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
> > 
> > I'd make that g_return_val_if_fail(codec_type ==
> > SPICE_VIDEO_CODEC_TYPE_MJPEG);
> 
> mjpeg_encoder.c does not depend on glib.h currently. The g_return_xxx() 
> functions are also very rarely used in the Spice server: only twice in 
> reds_stream.c. Furthermore calling mjpeg_encoder_new() for anything but 
> the MJPEG codec can only result from a coding error. So I think 
> spice_assert() is appropriate here.


spice_return_if_fail() can be used then. xx_return_if_fail() are used to
indicate programming errors, ie if they are triggered, then the code is
buggy. spice-server being a library, the less assert() it contains, the
better. So if the rest of the code can handle it just fine, better to
tag programming errors with xx_return_if_fail() rather than
assert'ing().

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151030/a05e6141/attachment-0001.sig>