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

[Victor Toso]

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

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

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 at redhat.com>
>
> 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 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: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/9513e0b3/attachment.sig>