[Spice-commits] 6 commits - src/channel-display-gst.c
Victor Toso de Carvalho
victortoso at kemper.freedesktop.org
Mon Jul 10 15:36:12 UTC 2017
src/channel-display-gst.c | 197 ++++++++++++++++++++++++++++++++++++++--------
1 file changed, 165 insertions(+), 32 deletions(-)
New commits:
commit ed697dd7fa165632bd0cbdb34c971c8001c433fa
Author: Victor Toso <me at victortoso.com>
Date: Thu Apr 27 11:12:38 2017 +0200
display-gst: Use Playbin for GStreamer 1.9.0 onwards
The Playbin can provide the full pipeline which reduces the
overall maintenance in the code as we don't need to track which
decoder can work with our stream type.
We need to maintain the GstCaps per SPICE_VIDEO_CODEC_TYPE in order to
tell Playbin the type of data we expect. This much should be covered
by spice-protocol and very likely we will need to extend it in the
future to allow more settings that might not possible to verify at
runtime.
This patch keeps previous code for compatibility reasons.
Note that we have to wait Playbin to emit "source-setup" in order to
configure GstAppSrc with the capabilities of input stream. If in the
unlikely event of frames arriving while GstAppSrc is not setup, we
will drop those frames.
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Pavel Grunt <pgrunt at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 44aee46..8669562 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -50,6 +50,9 @@ typedef struct SpiceGstDecoder {
guint timer_id;
} SpiceGstDecoder;
+/* FIXME: With gstreamer version 1.9.0 and higher, we are using playbin to
+ * create the pipeline for us and for that reason we don't need to keep track of
+ * decoder's name anymore. */
static struct {
const gchar *dec_name;
const gchar *dec_caps;
@@ -83,6 +86,24 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= SPICE_VIDEO_CODEC_TYPE_ENUM_END);
#define VALID_VIDEO_CODEC_TYPE(codec) \
(codec > 0 && codec < G_N_ELEMENTS(gst_opts))
+/* GstPlayFlags enum is in plugin's header which should not be exported.
+ * https://bugzilla.gnome.org/show_bug.cgi?id=784279
+ */
+typedef enum {
+ GST_PLAY_FLAG_VIDEO = (1 << 0),
+ GST_PLAY_FLAG_AUDIO = (1 << 1),
+ GST_PLAY_FLAG_TEXT = (1 << 2),
+ GST_PLAY_FLAG_VIS = (1 << 3),
+ GST_PLAY_FLAG_SOFT_VOLUME = (1 << 4),
+ GST_PLAY_FLAG_NATIVE_AUDIO = (1 << 5),
+ GST_PLAY_FLAG_NATIVE_VIDEO = (1 << 6),
+ GST_PLAY_FLAG_DOWNLOAD = (1 << 7),
+ GST_PLAY_FLAG_BUFFERING = (1 << 8),
+ GST_PLAY_FLAG_DEINTERLACE = (1 << 9),
+ GST_PLAY_FLAG_SOFT_COLORBALANCE = (1 << 10),
+ GST_PLAY_FLAG_FORCE_FILTERS = (1 << 11),
+} SpiceGstPlayFlags;
+
/* ---------- SpiceGstFrame ---------- */
typedef struct SpiceGstFrame {
@@ -298,13 +319,81 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
return TRUE;
}
+#if GST_CHECK_VERSION(1,9,0)
+static void app_source_setup(GstElement *pipeline G_GNUC_UNUSED,
+ GstElement *source,
+ SpiceGstDecoder *decoder)
+{
+ GstCaps *caps;
+
+ /* - We schedule the frame display ourselves so set sync=false on appsink
+ * so the pipeline decodes them as fast as possible. This will also
+ * minimize the risk of frames getting lost when we rebuild the
+ * pipeline.
+ * - Set max-bytes=0 on appsrc so it does not drop frames that may be
+ * needed by those that follow.
+ */
+ caps = gst_caps_from_string(gst_opts[decoder->base.codec_type].dec_caps);
+ g_object_set(source,
+ "caps", caps,
+ "is-live", TRUE,
+ "format", GST_FORMAT_TIME,
+ "max-bytes", 0,
+ "block", TRUE,
+ NULL);
+ gst_caps_unref(caps);
+ decoder->appsrc = GST_APP_SRC(gst_object_ref(source));
+}
+#endif
+
static gboolean create_pipeline(SpiceGstDecoder *decoder)
{
- gchar *desc;
GstAppSinkCallbacks appsink_cbs = { NULL };
- GError *err = NULL;
GstBus *bus;
+#if GST_CHECK_VERSION(1,9,0)
+ GstElement *playbin, *sink;
+ SpiceGstPlayFlags flags;
+ GstCaps *caps;
+ playbin = gst_element_factory_make("playbin", "playbin");
+ if (playbin == NULL) {
+ spice_warning("error upon creation of 'playbin' element");
+ return FALSE;
+ }
+
+ sink = gst_element_factory_make("appsink", "sink");
+ if (sink == NULL) {
+ spice_warning("error upon creation of 'appsink' element");
+ gst_object_unref(playbin);
+ return FALSE;
+ }
+
+ caps = gst_caps_from_string("video/x-raw,format=BGRx");
+ g_object_set(sink,
+ "caps", caps,
+ "sync", FALSE,
+ "drop", FALSE,
+ NULL);
+ gst_caps_unref(caps);
+
+ g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup), decoder);
+
+ g_object_set(playbin,
+ "uri", "appsrc://",
+ "video-sink", gst_object_ref(sink),
+ NULL);
+
+ /* Disable audio in playbin */
+ g_object_get(playbin, "flags", &flags, NULL);
+ flags &= ~(GST_PLAY_FLAG_AUDIO | GST_PLAY_FLAG_TEXT);
+ g_object_set(playbin, "flags", flags, NULL);
+
+ g_warn_if_fail(decoder->appsrc == NULL);
+ decoder->appsink = GST_APP_SINK(sink);
+ decoder->pipeline = playbin;
+#else
+ gchar *desc;
+ GError *err = NULL;
/* - We schedule the frame display ourselves so set sync=false on appsink
* so the pipeline decodes them as fast as possible. This will also
@@ -330,6 +419,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
decoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "src"));
decoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "sink"));
+#endif
appsink_cbs.new_sample = new_sample;
gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder, NULL);
@@ -457,6 +547,14 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
return FALSE;
}
+#if GST_CHECK_VERSION(1,9,0)
+ if (decoder->appsrc == NULL) {
+ spice_warning("Error: Playbin has not yet initialized the Appsrc element");
+ stream_dropped_frame_on_playback(decoder->base.stream);
+ return TRUE;
+ }
+#endif
+
/* ref() the frame data for the buffer */
frame->ref_data(frame->data_opaque);
GstBuffer *buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
commit c9209aef946b3ad517e7794d2e5648caf5ee2bf9
Author: Victor Toso <me at victortoso.com>
Date: Thu Apr 27 09:53:20 2017 +0200
display-gst: check GstRegistry for decoding elements
With this patch, we can find all the elements in the registry that are
video decoders which can handle the predefined GstCaps.
Main benefits are:
- We don't rely on a predefined list of GstElements. We don't need to
update them;
- debugging: It does help to know what the registry has at runtime;
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 57f0eb7..44aee46 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -521,16 +521,62 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
return (VideoDecoder*)decoder;
}
+static void gstvideo_debug_available_decoders(int codec_type,
+ GList *all_decoders,
+ GList *codec_decoders)
+{
+ GList *l;
+ GString *msg = g_string_new(NULL);
+ /* Print list of available decoders to make debugging easier */
+ g_string_printf(msg, "From %3u video decoder elements, %2u can handle caps %12s: ",
+ g_list_length(all_decoders), g_list_length(codec_decoders),
+ gst_opts[codec_type].dec_caps);
+
+ for (l = codec_decoders; l != NULL; l = l->next) {
+ GstPluginFeature *pfeat = GST_PLUGIN_FEATURE(l->data);
+ g_string_append_printf(msg, "%s, ", gst_plugin_feature_get_name(pfeat));
+ }
+
+ /* Drop trailing ", " */
+ g_string_truncate(msg, msg->len - 2);
+ spice_debug("%s", msg->str);
+ g_string_free(msg, TRUE);
+}
+
G_GNUC_INTERNAL
gboolean gstvideo_has_codec(int codec_type)
{
- gboolean has_codec = FALSE;
+ GList *all_decoders, *codec_decoders;
+ GstCaps *caps;
+ GstElementFactoryListType type;
+
+ g_return_val_if_fail(gstvideo_init(), FALSE);
+ g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), FALSE);
- VideoDecoder *decoder = create_gstreamer_decoder(codec_type, NULL);
- if (decoder) {
- has_codec = TRUE;
- decoder->destroy(decoder);
+ type = GST_ELEMENT_FACTORY_TYPE_DECODER |
+ GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO |
+ GST_ELEMENT_FACTORY_TYPE_MEDIA_IMAGE;
+ all_decoders = gst_element_factory_list_get_elements(type, GST_RANK_NONE);
+ if (all_decoders == NULL) {
+ spice_warning("No video decoders from GStreamer were found");
+ return FALSE;
}
- return has_codec;
+ caps = gst_caps_from_string(gst_opts[codec_type].dec_caps);
+ codec_decoders = gst_element_factory_list_filter(all_decoders, caps, GST_PAD_SINK, TRUE);
+ gst_caps_unref(caps);
+
+ if (codec_decoders == NULL) {
+ spice_debug("From %u decoders, none can handle '%s'",
+ g_list_length(all_decoders), gst_opts[codec_type].dec_caps);
+ gst_plugin_feature_list_free(all_decoders);
+ return FALSE;
+ }
+
+ if (spice_util_get_debug())
+ gstvideo_debug_available_decoders(codec_type, all_decoders, codec_decoders);
+
+ gst_plugin_feature_list_free(codec_decoders);
+ gst_plugin_feature_list_free(all_decoders);
+ return TRUE;
}
commit 44a7cbe72bc2207a58d1356b441efb29638bd57d
Author: Victor Toso <me at victortoso.com>
Date: Thu Apr 27 08:52:14 2017 +0200
display-gst: move "caps=" from struct to pipeline
This way we have a map of the necessary GstCaps to a given
SPICE_VIDEO_CODEC_TYPE.
This patch is also a preparatory patch to:
- Identify which GstElements in GstRegistry can handle this GstCaps
- Use Playbin as wrapper to all elements beside GstAppSrc and
GstAppSink. In this case, we should rely on GstCaps to reduce
typefind errors as we should know what kind of data is expected
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 4ae27fe..57f0eb7 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -58,24 +58,24 @@ static struct {
{ NULL, NULL },
/* SPICE_VIDEO_CODEC_TYPE_MJPEG */
- { "jpegdec", "caps=image/jpeg" },
+ { "jpegdec", "image/jpeg" },
/* SPICE_VIDEO_CODEC_TYPE_VP8
*
* typefind is unable to identify VP8 streams by design.
* See: https://bugzilla.gnome.org/show_bug.cgi?id=756457
*/
- { "vp8dec", "caps=video/x-vp8" },
+ { "vp8dec", "video/x-vp8" },
/* SPICE_VIDEO_CODEC_TYPE_H264
* When setting video/x-h264, h264parse will complain if we don't have the
* stream-format or codec_data information. As stream-format is byte-stream
* (hardcoded in spice-server), let's add it here to avoid the warning.
*/
- { "h264parse ! avdec_h264", "caps=video/x-h264,stream-format=byte-stream" },
+ { "h264parse ! avdec_h264", "video/x-h264,stream-format=byte-stream" },
/* SPICE_VIDEO_CODEC_TYPE_VP9 */
- { "vp9dec", "caps=video/x-vp9" },
+ { "vp9dec", "video/x-vp9" },
};
G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= SPICE_VIDEO_CODEC_TYPE_ENUM_END);
@@ -314,7 +314,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
* needed by those that follow.
*/
desc = g_strdup_printf("appsrc name=src is-live=true format=time max-bytes=0 block=true "
- "%s ! %s ! videoconvert ! appsink name=sink "
+ "caps=%s ! %s ! videoconvert ! appsink name=sink "
"caps=video/x-raw,format=BGRx sync=false drop=false",
gst_opts[decoder->base.codec_type].dec_caps,
gst_opts[decoder->base.codec_type].dec_name);
commit 6fe88871240c53b8b4ed8fe8e28221dd37521c71
Author: Victor Toso <me at victortoso.com>
Date: Thu Apr 27 09:10:52 2017 +0200
display-gst: include capabilities for h264
As the comment states, incomplete GstCaps for h264 could trigger
errors in h264parse element, such as:
gst_h264_parse_set_caps: video/x-h264 caps without
codec_data or stream-format
This would make h264parse to ignore the capabilities that were set.
As spice-server is encoding as byte-stream it should be fine to set
this value here too.
Any other errors to h264 format should either be reported to
GStreamer or fixed by improving the spice-protocol.
The follow up patch will identify elements in GstRegistry based on
GstCaps so this is a necessary change to have.
This is also a preparatory patch to use Playbin element to create the
pipeline. Without this, Playbin or typefind will fail to recognize the
stream as H264.
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 16206f6..4ae27fe 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -68,10 +68,11 @@ static struct {
{ "vp8dec", "caps=video/x-vp8" },
/* SPICE_VIDEO_CODEC_TYPE_H264
- * h264 streams detection works fine and setting an incomplete cap
- * causes errors. So let typefind do all the work.
+ * When setting video/x-h264, h264parse will complain if we don't have the
+ * stream-format or codec_data information. As stream-format is byte-stream
+ * (hardcoded in spice-server), let's add it here to avoid the warning.
*/
- { "h264parse ! avdec_h264", "" },
+ { "h264parse ! avdec_h264", "caps=video/x-h264,stream-format=byte-stream" },
/* SPICE_VIDEO_CODEC_TYPE_VP9 */
{ "vp9dec", "caps=video/x-vp9" },
commit 9807aad51eb2409062d9c94b84006a6de9fc9ec3
Author: Victor Toso <me at victortoso.com>
Date: Thu Apr 27 10:51:21 2017 +0200
display-gst: remove SPICE_GSTVIDEO_AUTO check
The intention behind SPICE_GSTVIDEO_AUTO environment variable was to
easily test the decoding elements given by GStreamer while using the
decodebin element in our pipeline.
The usage of decodebin was disabled by default as it could trigger
different issues such as the usage of unstable vaapi elements [0].
[0] See: https://bugs.freedesktop.org/show_bug.cgi?id=90884
A follow-up patch will use playbin to create the pipeline. Playbin is
very similar to decodebin but it'll provide the whole pipeline which
should bring less maintenance to spice-gtk.
Further notes:
- Vaapi elements are more stable now and it should not be a problem to
use it.
- At this moment, there is no automatic enforcement from spice-gtk to
make usage of any video-codecs besides mjpeg. Application would need
to send preferred-video-codec-type message to trigger video decoding
with GStreamer.
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index d3e83e3..16206f6 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -54,12 +54,8 @@ static struct {
const gchar *dec_name;
const gchar *dec_caps;
} gst_opts[] = {
- /* decodebin will use vaapi if installed, which for a time could
- * intentionally crash the application. So only use decodebin as a
- * fallback or when SPICE_GSTVIDEO_AUTO is set.
- * See: https://bugs.freedesktop.org/show_bug.cgi?id=90884
- */
- { "decodebin", "" },
+ /* SpiceVideoCodecType starts at index 1 */
+ { NULL, NULL },
/* SPICE_VIDEO_CODEC_TYPE_MJPEG */
{ "jpegdec", "caps=image/jpeg" },
@@ -304,21 +300,10 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
static gboolean create_pipeline(SpiceGstDecoder *decoder)
{
gchar *desc;
- gboolean auto_enabled;
- guint opt;
GstAppSinkCallbacks appsink_cbs = { NULL };
GError *err = NULL;
GstBus *bus;
- auto_enabled = (g_getenv("SPICE_GSTVIDEO_AUTO") != NULL);
- if (auto_enabled || !VALID_VIDEO_CODEC_TYPE(decoder->base.codec_type)) {
- SPICE_DEBUG("Trying %s for codec type %d %s",
- gst_opts[0].dec_name, decoder->base.codec_type,
- (auto_enabled) ? "(SPICE_GSTVIDEO_AUTO is set)" : "");
- opt = 0;
- } else {
- opt = decoder->base.codec_type;
- }
/* - We schedule the frame display ourselves so set sync=false on appsink
* so the pipeline decodes them as fast as possible. This will also
@@ -330,7 +315,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
desc = g_strdup_printf("appsrc name=src is-live=true format=time max-bytes=0 block=true "
"%s ! %s ! videoconvert ! appsink name=sink "
"caps=video/x-raw,format=BGRx sync=false drop=false",
- gst_opts[opt].dec_caps, gst_opts[opt].dec_name);
+ gst_opts[decoder->base.codec_type].dec_caps,
+ gst_opts[decoder->base.codec_type].dec_name);
SPICE_DEBUG("GStreamer pipeline: %s", desc);
decoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
commit ee810fc1df7c074e69708930362fa6ba7c746e51
Author: Victor Toso <me at victortoso.com>
Date: Mon May 15 15:21:23 2017 +0200
display-gst: check codec type before creating decoder
Inserting this check in channel-display-gst.c as the GStreamer decoder
is the only one handling all the different video formats supported by
spice-protocol.
If a unsupported/bad codec type value was sent, spice-gtk will fail to
create the decoder and any messages related to this stream-id will be
ignored.
Signed-off-by: Victor Toso <victortoso at redhat.com>
Acked-by: Pavel Grunt <pgrunt at redhat.com>
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 9b79403..d3e83e3 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -512,6 +512,8 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
{
SpiceGstDecoder *decoder = NULL;
+ g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), NULL);
+
if (gstvideo_init()) {
decoder = spice_new0(SpiceGstDecoder, 1);
decoder->base.destroy = spice_gst_decoder_destroy;
More information about the Spice-commits
mailing list