Hello,
On Thu, Aug 8, 2019 at 11:23 AM Snir Sheriber <ssheribe@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 8/7/19 6:49 PM, Kevin Pouget wrote:
> > This patch allows the MJPEG encoder to inform the spice-widget that
> > its video drawing area (draw-area) should be made visible on screen.
>
>
> So the first one does not work?
it was working, but the patch was more a workaround than a proper fix,
this call:
> > + gtk_stack_set_visible_child_name(d->stack, "draw-area");
was triggered too often, ie also when switching between GST streams
(but the gst-area would be put back on top right after)
> BTW i think the consensus is to mention the patch version in all
> of the subjects (not only in the cover letter)
yes, sorry, I wanted to post this as a reply to the previous email,
but git forgot to ask me the in-reply-to value, I don't know why :#
....
> > This is required to switch from GST video decoding to native MJPEG
> > decoding, otherwise the gst-area remained on top and the MJPEG video
> > stream was never shown.
>
>
> Actually IIRC the first version of the gst-overlay signal included
> a boolean value which could have been useful here :P
you mean an earlier version, or the one modified by this patch?
I wondered if it would be an acceptable solution to hack the existing
set_overlay function,
for instance if pipeline_ptr == NULL, then set the native drawing area
> >
> > Signed-off-by: Kevin Pouget <kpouget@xxxxxxxxxx>
> >
> > ---
> > src/channel-display-mjpeg.c | 2 ++
> > src/channel-display-priv.h | 2 +-
> > src/channel-display.c | 38 ++++++++++++++++++++++++++++++++++---
> > src/spice-marshal.txt | 1 +
> > src/spice-widget.c | 16 ++++++++++++++--
> > 5 files changed, 53 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index 647d31b..6f1a4d7 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -300,5 +300,7 @@ VideoDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream)
> >
> > /* All the other fields are initialized to zero by g_new0(). */
> >
> > + show_mjpeg_widget(stream);
> > +
> > return (VideoDecoder*)decoder;
> > }
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 16c12c6..3424a8e 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -199,7 +199,7 @@ void stream_dropped_frame_on_playback(display_stream *st);
> > void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t width, uint32_t height, int stride, uint8_t* data);
> > guintptr get_window_handle(display_stream *st);
> > gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline);
> > -
> > +gboolean show_mjpeg_widget(display_stream *st);
> > void spice_frame_free(SpiceFrame *frame);
> >
> > G_END_DECLS
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 44555e3..18e95b9 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -90,7 +90,8 @@ enum {
> > SPICE_DISPLAY_MARK,
> > SPICE_DISPLAY_GL_DRAW,
> > SPICE_DISPLAY_STREAMING_MODE,
> > - SPICE_DISPLAY_OVERLAY,
> > + SPICE_DISPLAY_GST_OVERLAY,
> > + SPICE_DISPLAY_MJPEG_OVERLAY,
>
> The term overlay is derived from GstVideoOverlay which is an gstreamer
> interface that overlays gstreamer output on the gtk widget, in the case
> of builtin mjpeg (also with gst is not always true) we pass pixels and
> render into the surface I don't think the name should invlude "overlay".
actually, I also don't really like to have "mjpeg" in the name,
because GST also supports it,
maybe SPICE_DISPLAY_NATIVE_DRAW_AREA
> >
> > SPICE_DISPLAY_LAST_SIGNAL,
> > };
> > @@ -491,7 +492,7 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
> > *
> > * Since: 0.36
> > **/
> > - signals[SPICE_DISPLAY_OVERLAY] =
> > + signals[SPICE_DISPLAY_GST_OVERLAY] =
> > g_signal_new("gst-video-overlay",
> > G_OBJECT_CLASS_TYPE(gobject_class),
> > 0, 0,
> > @@ -501,6 +502,25 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
> > 1,
> > GST_TYPE_PIPELINE);
> >
> > + /**
> > + * SpiceDisplayChannel::mjpeg-video-overlay:
> > + * @display: the #SpiceDisplayChannel that emitted the signal
> > + *
> > + * The #SpiceDisplayChannel::mjpeg-video-overlay signal is emitted
> > + * when the MJPEG encoder is ready and its drawing area should be
> > + * made visible on screen
> > + *
> > + * Since: 0.37
> > + **/
> > + signals[SPICE_DISPLAY_MJPEG_OVERLAY] =
> > + g_signal_new("mjpeg-video-overlay",
> > + G_OBJECT_CLASS_TYPE(gobject_class),
> > + 0, 0,
> > + NULL, NULL,
> > + g_cclosure_user_marshal_BOOLEAN__VOID,
> > + G_TYPE_BOOLEAN,
> > + 0);
> > +
> > channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
> > }
> >
> > @@ -1472,12 +1492,24 @@ gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
> > gboolean res = false;
> >
> > if (st->surface->streaming_mode) {
> > - g_signal_emit(st->channel, signals[SPICE_DISPLAY_OVERLAY], 0,
> > + g_signal_emit(st->channel, signals[SPICE_DISPLAY_GST_OVERLAY], 0,
> > pipeline, &res);
> > }
> > return res;
> > }
> >
> > +G_GNUC_INTERNAL
> > +gboolean show_mjpeg_widget(display_stream *st)
> > +{
> > + gboolean res = false;
> > + g_warning("SHOW!");
>
> I guess this was for testing purposes.
indeed, I'll update the checkpatch script to warn about new g_warning!
> > + if (st->surface->streaming_mode) {
> > + g_signal_emit(st->channel, signals[SPICE_DISPLAY_MJPEG_OVERLAY], 0, &res);
> > + }
> > +
> > + return res;
> > +}
> > +
> > /* after a sequence of 3 drops, push a report to the server, even
> > * if the report window is bigger */
> > #define STREAM_REPORT_DROP_SEQ_LEN_LIMIT 3
> > diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
> > index 46be405..e60d725 100644
> > --- a/src/spice-marshal.txt
> > +++ b/src/spice-marshal.txt
> > @@ -8,6 +8,7 @@ VOID:POINTER,INT
> > VOID:POINTER,BOOLEAN
> > BOOLEAN:POINTER,UINT
> > BOOLEAN:UINT
> > +BOOLEAN:VOID
> > VOID:UINT,POINTER,UINT
> > VOID:UINT,UINT,POINTER,UINT
> > BOOLEAN:UINT,POINTER,UINT
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index a9ba1f1..34e3c5e 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -2210,6 +2210,8 @@ static void update_image(SpiceDisplay *display)
> > SpiceDisplayPrivate *d = display->priv;
> >
> > spice_cairo_image_create(display);
> > + gtk_stack_set_visible_child_name(d->stack, "draw-area");
> > +
> > if (d->canvas.convert)
> > do_color_convert(display, &d->area);
> > }
> > @@ -2782,7 +2784,7 @@ static void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
> > /* This callback should pass to the widget a pointer of the pipeline
> > * so that we can set pipeline and overlay related calls from here.
> > */
> > -static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr, SpiceDisplay *display)
> > +static gboolean gst_set_overlay(SpiceChannel *channel, void* pipeline_ptr, SpiceDisplay *display)
> > {
> > #if defined(GDK_WINDOWING_X11)
> > SpiceDisplayPrivate *d = display->priv;
> > @@ -2808,6 +2810,14 @@ static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr, SpiceDisp
> > return false;
> > }
> >
> > +static gboolean mjpeg_set_overlay(SpiceChannel *channel, SpiceDisplay *display)
> > +{
> > + SpiceDisplayPrivate *d = display->priv;
> > + g_warning("SET!");
>
> Same
>
> > + 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?
if yes, what it the point of using the native decoder?
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.
> > --
> > 2.21.0
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel