[Spice-devel] [RFC spice-gtk 1/2] streaming: make draw-area visible on MJPEG encoder creation

[Victor Toso]

Hi,

Sorry for being late here!

On Thu, Aug 08, 2019 at 06:36:48PM +0300, Snir Sheriber wrote:
> On 8/8/19 1:09 PM, Kevin Pouget wrote:
> > > > +    gtk_stack_set_visible_child_name(d->stack, "draw-area");
> > > > +    return true;
> > > > +}
> > > > +
> > > >    static void invalidate(SpiceChannel *channel,
> > > >                           gint x, gint y, gint w, gint h, gpointer data)
> > > >    {
> > > > @@ -3192,7 +3202,9 @@ static void channel_new(SpiceSession *s, SpiceChannel *channel, SpiceDisplay *di
> > > >                                          G_CALLBACK(spice_display_widget_update_monitor_area),
> > > >                                          display, G_CONNECT_AFTER | G_CONNECT_SWAPPED);
> > > >            spice_g_signal_connect_object(channel, "gst-video-overlay",
> > > > -                                      G_CALLBACK(set_overlay), display, G_CONNECT_AFTER);
> > > > +                                      G_CALLBACK(gst_set_overlay), display, G_CONNECT_AFTER);
> > > > +        spice_g_signal_connect_object(channel, "mjpeg-video-overlay",
> > > > +                                      G_CALLBACK(mjpeg_set_overlay), display, G_CONNECT_AFTER);
> > > >            if (spice_display_channel_get_primary(channel, 0, &primary)) {
> > > >                primary_create(channel, primary.format, primary.width, primary.height,
> > > >                               primary.stride, primary.shmid, primary.data, display);
> > > I'd also suggest another option which is to not allow switching into
> > > built-in
> > > mjpeg in case you started with gstreamer (i.e. one you started with
> > > gstreamer decoder stick to it).
> > > I guess it will be simpler but needs to see what this would require.
> > can we assume that Gstreamer always supports MJPEG decoding?
> 
> Theoretically no, practically i think we can assume this if
> we'll require some libs as mandatory.
> 
> Worth checking performance difference.

Yes, I've tested long time ago and performance was fine, don't
recall if it was avdec_mjpeg or other but there are a few.

I would say we can fix what we need to fix to make this switch
working but it is worth goal to keep decoding as much as possible
in the GStreamer stack, that is, if performance seems fine to
all.

> > if yes, what it the point of using the native decoder?
> 
> Still, this is more safe i guess, gstreamer is building its
> decoding pipeline dynamically , something always may fail.

Indeed but considering that we might have to consider several
video-codecs in the future, the safe native decoder might become
a maintenance burden also because mjpeg is hardly the first
option for streaming solution.

> > at the moment, display_stream_create goes into the native decoder if
> > the stream is MJPEG, and GST otherwise. So GST will never decode MJPEG
> > streams.
> 
> Indeed, i meant, once gstreamer is used don't let native
> mjpeg to be used.
> If it starts with mjpeg, use native, but once you switching to
> gstreamer, use only gstreamer. also if switching back to mjpeg.

I'd consider disabling native mjpeg in favor of gstreamer and
probably removing it in the future from the codebase.

Cheers,
Victor
-------------- 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/20190823/5fa2b809/attachment.sig>