Hi, On Tue, Jun 06, 2017 at 05:14:49PM +0200, Christophe Fergeau wrote: > On Tue, May 16, 2017 at 04:48:14PM +0200, Victor Toso wrote: > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > By using this environment variable, we could use decodebin to let > > GStreamer automatically find the best elements to get the streaming > > "the stream"? Yes, fixed > > decoded. It was disable by default, in an attempt to have a easy way > > "disabled". The sentence reads a bit oddly to me, "it's disabled by > default" does not seem related to the followup "in an attempt to have an > easy way to test it" I'll try to formulate it better bellow > > to test it. > > > > Follow up patch will use Playbin to create the pipeline which does the > > "A follow-up patch" "which has a similar behaviour" Thanks, fixed > > > similar behavior but with less work to maintain the pipeline. > > > > Remove this in a separated patch to reduce the code changes. > > "The removal is done in a separate patch to reduce the code changes" Okay. Thanks for all those fixes! Changed the commit log a little bit, hope it got better now! " 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@xxxxxxxxxx> > > Signed-off-by: Victor Toso <me@xxxxxxxxxxxxxx> > > --- > > src/channel-display-gst.c | 22 ++++------------------ > > 1 file changed, 4 insertions(+), 18 deletions(-) > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c > > index d3e83e3..538e75a 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 > > This bug has been resolved 2 years ago, I assume this means we are > confident we're no longer going to trigger this issue? Yeah, for upstream we should be fine. > > > - */ > > - { "decodebin", "" }, > > + /* Spice' video codec type starts on index 1 */ > > I'd word this as /* SpiceVideoCodecType starts at index 1 */ Will fix it! > Apart from these minor comments, looks good to me, > Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx> > > Christophe Thanks for the review, toso > > > + { 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); > > -- > > 2.13.0 > > > > _______________________________________________ > > Spice-devel mailing list > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel