[Spice-devel,v5,20/20] spice-gtk: Use typefind and decodebin as fallbacks to set up the GStreamer pipeline.

Submitted by Francois Gouget on Aug. 27, 2015, 7:03 p.m.

Details

Message ID alpine.DEB.2.20.1508272045290.22172@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget Aug. 27, 2015, 7:03 p.m.
Potentially this means future video codecs can be supported automatically.
One can also use force usage of typefind and decodebin by setting the SPICE_GST_AUTO environment variable. If set to 'decodebin' then only decodebin will be used.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

Changes since take 2:
 - None.


 src/channel-display-gst.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 59ce3e1..3d9274b 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -110,27 +110,35 @@  static gboolean construct_pipeline(GstDecoder *decoder)
     decoder->pipeline_wait = 1;
     decoder->samples_count = 0;
 
-    const gchar *src_caps, *gstdec_name;
+    const gchar *src_caps = NULL;
+    const gchar *gstdec_name = NULL;
     switch (decoder->base.codec_type) {
     case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-        src_caps = "image/jpeg";
+        src_caps = "caps=image/jpeg";
         gstdec_name = "jpegdec";
         break;
     case SPICE_VIDEO_CODEC_TYPE_VP8:
-        src_caps = "video/x-vp8";
+        src_caps = "caps=video/x-vp8";
         gstdec_name = "vp8dec";
         break;
     case SPICE_VIDEO_CODEC_TYPE_H264:
-        src_caps = "video/x-h264";
+        src_caps = "caps=video/x-h264";
         gstdec_name = "h264parse ! avdec_h264";
         break;
     default:
-        spice_warning("Unknown codec type %d", decoder->base.codec_type);
-        return -1;
+        SPICE_DEBUG("Unknown codec type %d", decoder->base.codec_type);
+        break;
+    }
+    const gchar *gst_auto = getenv("SPICE_GST_AUTO");
+    if (!src_caps || (gst_auto && strcmp(gst_auto, "decodebin"))) {
+        src_caps = "typefind=true"; /* Misidentifies VP8 */
+    }
+    if (!gstdec_name || gst_auto) {
+        gstdec_name = "decodebin";  /* vaapi is assert-happy */
     }
 
     GError *err = NULL;
-    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=1 caps=%s ! %s ! videoconvert ! appsink name=sink caps=video/x-raw,format=BGRx", src_caps, gstdec_name);
+    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=1 %s ! %s ! videoconvert ! appsink name=sink caps=video/x-raw,format=BGRx", src_caps, gstdec_name);
     SPICE_DEBUG("GStreamer pipeline: %s", desc);
     decoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
     g_free(desc);

Comments

On Thu, Aug 27, 2015 at 09:03:21PM +0200, Francois Gouget wrote:
> Potentially this means future video codecs can be supported automatically.
> One can also use force usage of typefind and decodebin by setting the
> SPICE_GST_AUTO environment variable. If set to 'decodebin' then only
> decodebin will be used.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> Changes since take 2:
>  - None.
> 
> 
>  src/channel-display-gst.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 59ce3e1..3d9274b 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -110,27 +110,35 @@ static gboolean construct_pipeline(GstDecoder *decoder)
>      decoder->pipeline_wait = 1;
>      decoder->samples_count = 0;
>  
> -    const gchar *src_caps, *gstdec_name;
> +    const gchar *src_caps = NULL;
> +    const gchar *gstdec_name = NULL;
>      switch (decoder->base.codec_type) {
>      case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> -        src_caps = "image/jpeg";
> +        src_caps = "caps=image/jpeg";
>          gstdec_name = "jpegdec";
>          break;
>      case SPICE_VIDEO_CODEC_TYPE_VP8:
> -        src_caps = "video/x-vp8";
> +        src_caps = "caps=video/x-vp8";
>          gstdec_name = "vp8dec";
>          break;
>      case SPICE_VIDEO_CODEC_TYPE_H264:
> -        src_caps = "video/x-h264";
> +        src_caps = "caps=video/x-h264";
>          gstdec_name = "h264parse ! avdec_h264";
>          break;
>      default:
> -        spice_warning("Unknown codec type %d", decoder->base.codec_type);
> -        return -1;
> +        SPICE_DEBUG("Unknown codec type %d", decoder->base.codec_type);
> +        break;
> +    }
> +    const gchar *gst_auto = getenv("SPICE_GST_AUTO");
> +    if (!src_caps || (gst_auto && strcmp(gst_auto, "decodebin"))) {

can be g_strcmp0(gst_auto, "decodebin") here rather than (gst_auto &&
strcmp(..)) and I'd add an explicit != 0

> +        src_caps = "typefind=true"; /* Misidentifies VP8 */

What does VP8 gets identified as when using this? Has this been reported
to gstreamer developers?

> +    }
> +    if (!gstdec_name || gst_auto) {
> +        gstdec_name = "decodebin";  /* vaapi is assert-happy */

Does this comment mean that gstreamer is going to pick vaapi if
available when decodebin is present? Have you reported the crashes you
got upstream? Are these bugs the only reason why you are using fixed
pipelines for VP8/H264 rather than always using decodebin?

Looks good otherwise.

Christophe

>      }
>  
>      GError *err = NULL;
> -    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=1 caps=%s ! %s ! videoconvert ! appsink name=sink caps=video/x-raw,format=BGRx", src_caps, gstdec_name);
> +    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=1 %s ! %s ! videoconvert ! appsink name=sink caps=video/x-raw,format=BGRx", src_caps, gstdec_name);
>      SPICE_DEBUG("GStreamer pipeline: %s", desc);
>      decoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
>      g_free(desc);
> -- 
> 2.5.0
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, 21 Sep 2015, Christophe Fergeau wrote:
[...]
> > +        src_caps = "typefind=true"; /* Misidentifies VP8 */
> 
> What does VP8 gets identified as when using this? Has this been reported
> to gstreamer developers?

With GStreamer 1.6.0 typefind does not identify it at all (with 
GStreamer 0.10 I believe it mistook it for something else). I reported 
the GStreamer 1.0 issue this but it does not seem like this is a use 
case the GStreamer developers care about: 
https://bugzilla.gnome.org/show_bug.cgi?id=756457


> > +    }
> > +    if (!gstdec_name || gst_auto) {
> > +        gstdec_name = "decodebin";  /* vaapi is assert-happy */
> 
> Does this comment mean that gstreamer is going to pick vaapi if
> available when decodebin is present?

Yes.

> Have you reported the crashes you got upstream?

I did and it got fixed recently. It will be some time before the fix is 
widely disseminated though.
https://bugs.freedesktop.org/show_bug.cgi?id=90884


> Are these bugs the only reason why you are using fixed pipelines for 
> VP8/H264 rather than always using decodebin?

Yes. At this point relying on typefind and/or decodebin does not seem 
reliable enough.
Hey,

Thanks for all the details!

On Tue, Oct 13, 2015 at 01:12:40AM +0200, Francois Gouget wrote:
> On Mon, 21 Sep 2015, Christophe Fergeau wrote:
> [...]
> > > +        src_caps = "typefind=true"; /* Misidentifies VP8 */
> > 
> > What does VP8 gets identified as when using this? Has this been reported
> > to gstreamer developers?
> 
> With GStreamer 1.6.0 typefind does not identify it at all (with 
> GStreamer 0.10 I believe it mistook it for something else). I reported 
> the GStreamer 1.0 issue this but it does not seem like this is a use 
> case the GStreamer developers care about: 
> https://bugzilla.gnome.org/show_bug.cgi?id=756457
> 
> 
> > > +    }
> > > +    if (!gstdec_name || gst_auto) {
> > > +        gstdec_name = "decodebin";  /* vaapi is assert-happy */
> > 
> > Does this comment mean that gstreamer is going to pick vaapi if
> > available when decodebin is present?
> 
> Yes.
> 
> > Have you reported the crashes you got upstream?
> 
> I did and it got fixed recently. It will be some time before the fix is 
> widely disseminated though.
> https://bugs.freedesktop.org/show_bug.cgi?id=90884
> 
> 
> > Are these bugs the only reason why you are using fixed pipelines for 
> > VP8/H264 rather than always using decodebin?
> 
> Yes. At this point relying on typefind and/or decodebin does not seem 
> reliable enough.

Having all/most of this as code comments or in the commit log would be
useful imo.

Christophe