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.
* 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.
> > /* Start playing */
> > spice_debug("setting state to PLAYING");
> > @@ -225,6 +274,12 @@ static gboolean construct_pipeline(SpiceGstEncoder *encoder,
> > /* A helper for gst_encoder_encode_frame(). */
> > static void reconfigure_pipeline(SpiceGstEncoder *encoder)
> > {
> > + if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> > + /* vp8enc gets confused if we try to reconfigure the pipeline */
> > + reset_pipeline(encoder);
> > + return;
> > + }
>
> Any bug reference for this one? Just add it to the comment if there is
> such a bug, if not, that's fine.
vp8enc ignores our attempt to change the appsrc caps so that when it
gets a buffer of the wrong size we get stuck. Given that VP8 performs
inter-frame compression I cannot totally blame it for not liking the
video format to change mid-stream. So while I'd prefer it if it could
deal with this case, I'm not sure this warrants a vp8enc bug report. In
any case this requires a workaround for now. I tried to make the comment
more explicit.
Here are the corresponding log traces:
(/usr/bin/Xorg:26752): Spice-Debug **: gstreamer_encoder.c:415:gst_encoder_encode_frame: video format change: width 0 -> 640, height 0 -> 360, format 0 -> 9
[...]
(/usr/bin/Xorg:26752): Spice-Debug **: gstreamer_encoder.c:415:gst_encoder_encode_frame: video format change: width 640 -> 764, height 360 -> 448, format 9 -> 9
[...]
0:00:00.343781738 26752 0x7f9768544050 DEBUG basetransform gstbasetransform.c:624:gst_base_transform_transform_size:<ffmpegcsp0> asked to transform size 1369088 for caps video/x-raw-rgb, bpp=(int)32, depth=(int)24, width=(int)640, height=(int)360, endianness=(int)4321, red_mask=(int)65280, green_mask=(int)16711680, blue_mask=(int)-16777216, framerate=(fraction)30/1 to size for caps video/x-raw-yuv, format=(fourcc)I420, width=(int)640, height=(int)360, framerate=(fraction)30/1 in direction SINK
** (Xorg:26752): WARNING **: ffmpegcsp0: size 1369088 is not a multiple of unit size 921600
See also:
http://lists.freedesktop.org/archives/spice-devel/2015-August/021274.html
> > @@ -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.
See also:
http://lists.freedesktop.org/archives/spice-devel/2015-July/021161.html
> > +int red_dispatcher_set_video_codecs(const char *codecs)
> > +{
> > + uint32_t encoder;
> > + SpiceVideoCodecType type;
> > + uint32_t cap;
> > + char *encoder_name, *codec_name;
> > + static RedVideoCodec new_codecs[RED_MAX_VIDEO_CODECS];
> > + int count;
> > + const char* c;
> > +
> > + if (strcmp(codecs, "auto") == 0) {
> > + codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
> > + }
> > +
> > + c = codecs;
> > + count = 0;
> > + while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
>
> Might be simpler to parse with g_strsplit() and strstr or such, but your
> way is ok (though took me a while to find what sscanf(%m) is doing!)
I was initially using a more manual way of parsing the string but Victor
Toso recommended using sscanf which did lead to shorter code:
http://lists.freedesktop.org/archives/spice-devel/2015-July/021161.html
> > @@ -3108,10 +3135,15 @@ static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
> > video_cbs.update_client_playback_delay = red_stream_update_client_playback_latency;
> >
> > initial_bit_rate = red_stream_get_initial_bit_rate(dcc, stream);
> > - agent->video_encoder = red_display_create_video_encoder(initial_bit_rate, &video_cbs, agent);
> > + agent->video_encoder = red_display_create_video_encoder(dcc, initial_bit_rate, &video_cbs, agent);
> > } else {
> > - agent->video_encoder = red_display_create_video_encoder(0, NULL, NULL);
> > + agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
> > }
> > + if (agent->video_encoder == NULL) {
> > + stream->refs--;
> > + return FALSE;
> > + }
> > +
>
> Have you tested this error path? What happens when we destroy the stream
> this way? bad rendering, or just fall back to no streaming?
If I remember correctly (I tested this months ago), it goes back to no
streaming. I don't remember if I got it to stop trying to initiate the
stream over and over again though.
--
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel