Hi,
On 8/8/19 1:09 PM, Kevin Pouget wrote:
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?
Earlier version
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
I'm not sure how bad is it, at least it shouldn't damage.
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
Sounds better.
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?
Theoretically no, practically i think we can assume this if we'll
require some libs as mandatory.
Worth checking performance difference.
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.
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.
Snir
--
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