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@xxxxxxxxxxxxxxx>_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel