[Spice-devel] [PATCH spice-gtk v3 2/6] display-gst: remove SPICE_GSTVIDEO_AUTO check

[Christophe Fergeau]

On Tue, May 16, 2017 at 04:48:14PM +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> By using this environment variable, we could use decodebin to let
> GStreamer automatically find the best elements to get the streaming

"the stream"?

> 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"

> 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"

> 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"

> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Signed-off-by: Victor Toso <me at victortoso.com>
> ---
>  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?

> -     */
> -    { "decodebin", "" },
> +    /* Spice' video codec type starts on index 1 */

I'd word this as /* SpiceVideoCodecType starts at index 1 */


Apart from these minor comments, looks good to me, 
Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe

> +    { 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170606/47403ab7/attachment-0001.sig>