[Spice-devel] [PATCH spice 6/11] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client.

[Francois Gouget]

On Tue, 19 May 2015, Marc-André Lureau wrote:
[...]
> > +    /* Set the encoder initial bit rate, and ask for a low latency */
> > +    adjust_bit_rate(encoder);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "max-latency", 0, NULL);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "max-keyframe-distance", 0, NULL);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "lag-in-frames", 0, NULL);
> > +
> 
> Those paremeters do not exist with all encoders, notably ffenc_mjpeg.

I'll tweak it so we only set the properties supported by the current 
encoder.


> We could rather use or define encodebin profiles.

See discussion in the previous email.


[...]
> > +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> > +        /* vp8enc gets confused if we try to reconfigure the pipeline */
> >          reset_pipeline(encoder);
> 
> You'll easily reach this condition with CAP_SIZED_STREAM streams.

It's ok. reset_pipeline() frees the current pipeline, causing 
gst_encoder_encode_frame() to call construct_pipeline() as if this was 
the stream's first frame.


[...]
> >      /* The GStreamer buffer timestamps and framerate are irrelevant and would
> >       * be hard to set right because they can arrive a bit irregularly
> >       */
> > -    GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
> > -    GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> > +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> > +    {
> > +        /* FIXME: Maybe try drop-frame = 0 instead? */
> > +        GST_BUFFER_TIMESTAMP(buffer) = frame_time;
> > +        GST_BUFFER_DURATION(buffer) = 1;
> > +    }
> > +    else
> > +    {
> > +        GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
> > +        GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> > +    }
> 
> What is this supposed to change? Did you consider appsrc do-timestamp
> instead?

It's a hack to get both the mjpeg end vp8 encoders working.

Using 'appsrc do-timestamp=1' works for the mjpeg encoder but causes us 
to block indefinitely in gst_app_sink_pull_buffer() for the vp8 encoder 
(independently of is-live), for an unknown reason.

My suspicion is that the frame gets dropped, presumably by vp8enc 
itself, maybe due to pipeline latency issues.

I'm also not sure yet of what the best approach is for timestamping and 
setting the framerate in the appsrc caps and their impact on the 
bitrate.

One option is to advertise the outgoing framerate, the target bitrate 
and use do-timestamping. The outgoing framerate, e.g. 10fps, may be 
lower than the source framerate, e.g. 24 fps, which implies dropping 
frames before passing them off to the pipeline. This means some frames 
will be less than 1/10th second apart and, if I remember correctly, 
this causes the pipeline to drop them which is not what we want (and 
again causes a freeze in gst_app_sink_pull_buffer()).

Another option is to set the bitrate and framerate to get the desired 
compressed buffer size. Then the stream's bitrate can be further reduced 
by controlling the outgoing frame rate. In this case I'm not sure using 
real timestamps makes sense.


> > @@ -364,13 +382,13 @@ static int gst_encoder_encode_frame(GstEncoder
> > *encoder,
> >          encoder->spice_format = bitmap->format;
> >          encoder->width = width;
> >          encoder->height = height;
> > -        if (encoder->pipeline && !reconfigure_pipeline(encoder))
> > -            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> > +        if (encoder->pipeline)
> > +            reconfigure_pipeline(encoder);
> 
> Why this change?

reconfigure_pipeline() used to return false if it failed to reconfigure 
the pipeline. With the old code this would cause us to return 
FRAME_UNSUPPORTED. But in fact in such a case the existing pipeline has 
been freed and all we need to do is to rebuild it from scratch which is 
what we already do a few lines down.

This should maybe have been part of patch 5 though 
reconfigure_pipeline() does not fail with ffenc_mjpeg.


> > +        agent->video_encoder = red_display_create_video_encoder(dcc, initial_bit_rate, &video_cbs, agent);
> >      } else {
> > -        agent->video_encoder = create_video_encoder(0, NULL, NULL);
> > +        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
> >      }
> > +    /* FIXME: We may have failed to create a video encoder which will cause
> > +     *        a crash!
> > +     */
> 
> You may disable video streaming in this case? (worker->streaming_video ==
> STREAM_VIDEO_OFF)

The administrator could certainly set this option.

But as for gracefully handling this failure without configuration, as 
far as I can tell, by the time red_display_create_stream() fails it's 
too late to set streaming_video.


-- 
Francois Gouget <fgouget at codeweavers.com>