streaming: make draw-area visible on MJPEG encoder creation

Submitted by Kevin Pouget on Aug. 29, 2019, 12:16 p.m.

Details

Message ID 20190829121602.16695-1-kpouget@redhat.com
State Superseded
Headers show
Series "streaming: make draw-area visible on MJPEG encoder creation" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Kevin Pouget Aug. 29, 2019, 12:16 p.m.
This patch allows the MJPEG encoder to inform the spice-widget that
its video drawing area (draw-area) should be made visible on screen.

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.

Signed-off-by: Kevin Pouget <kpouget@redhat.com>
---
 src/channel-display-mjpeg.c | 3 +++
 src/spice-widget.c          | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 647d31b..636a98b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -300,5 +300,8 @@  VideoDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream)
 
     /* All the other fields are initialized to zero by g_new0(). */
 
+    /* makes the draw-area visible */
+    hand_pipeline_to_widget(stream, NULL);
+
     return (VideoDecoder*)decoder;
 }
diff --git a/src/spice-widget.c b/src/spice-widget.c
index a9ba1f1..7c257ff 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -2780,13 +2780,20 @@  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.
+ * so that we can the set GST pipeline and overlay related calls from
+ * here.  If the pipeline pointer is NULL, the drawing area of the
+ * native renderer is set visible.
  */
 static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr, SpiceDisplay *display)
 {
 #if defined(GDK_WINDOWING_X11)
     SpiceDisplayPrivate *d = display->priv;
 
+    if (pipeline_ptr == NULL) {
+        gtk_stack_set_visible_child_name(d->stack, "draw-area");
+        return true;
+    }
+
     /* GstVideoOverlay is currently used only under x */
     if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
         GDK_IS_X11_DISPLAY(gdk_display_get_default())) {

Comments

> 
> This patch allows the MJPEG encoder to inform the spice-widget that
> its video drawing area (draw-area) should be made visible on screen.
> 
> 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.
> 
> Signed-off-by: Kevin Pouget <kpouget@redhat.com>

Fine for me. Snir had the most comments on earlier versions.

> ---
>  src/channel-display-mjpeg.c | 3 +++
>  src/spice-widget.c          | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 647d31b..636a98b 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -300,5 +300,8 @@ VideoDecoder* create_mjpeg_decoder(int codec_type,
> display_stream *stream)
>  
>      /* All the other fields are initialized to zero by g_new0(). */
>  
> +    /* makes the draw-area visible */
> +    hand_pipeline_to_widget(stream, NULL);
> +
>      return (VideoDecoder*)decoder;
>  }
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index a9ba1f1..7c257ff 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -2780,13 +2780,20 @@ 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.
> + * so that we can the set GST pipeline and overlay related calls from
> + * here.  If the pipeline pointer is NULL, the drawing area of the
> + * native renderer is set visible.
>   */
>  static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr,
>  SpiceDisplay *display)
>  {
>  #if defined(GDK_WINDOWING_X11)
>      SpiceDisplayPrivate *d = display->priv;
>  
> +    if (pipeline_ptr == NULL) {
> +        gtk_stack_set_visible_child_name(d->stack, "draw-area");
> +        return true;
> +    }
> +
>      /* GstVideoOverlay is currently used only under x */
>      if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
>          GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
Hi,

On 8/29/19 7:17 PM, Frediano Ziglio 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.
>>
>> 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.
>>
>> Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> Fine for me. Snir had the most comments on earlier versions.
>
>> ---
>>   src/channel-display-mjpeg.c | 3 +++
>>   src/spice-widget.c          | 9 ++++++++-
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
>> index 647d31b..636a98b 100644
>> --- a/src/channel-display-mjpeg.c
>> +++ b/src/channel-display-mjpeg.c
>> @@ -300,5 +300,8 @@ VideoDecoder* create_mjpeg_decoder(int codec_type,
>> display_stream *stream)
>>   
>>       /* All the other fields are initialized to zero by g_new0(). */
>>   
>> +    /* makes the draw-area visible */
>> +    hand_pipeline_to_widget(stream, NULL);
>> +
>>       return (VideoDecoder*)decoder;
>>   }
>> diff --git a/src/spice-widget.c b/src/spice-widget.c
>> index a9ba1f1..7c257ff 100644
>> --- a/src/spice-widget.c
>> +++ b/src/spice-widget.c
>> @@ -2780,13 +2780,20 @@ 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.
>> + * so that we can the set GST pipeline and overlay related calls from
>> + * here.  If the pipeline pointer is NULL, the drawing area of the
>> + * native renderer is set visible.
>>    */

I think you should update also the signal comment (gst-video-overlay), that
the pipeline may be NULL

>>   static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr,
>>   SpiceDisplay *display)
>>   {
>>   #if defined(GDK_WINDOWING_X11)
>>       SpiceDisplayPrivate *d = display->priv;
>>   
>> +    if (pipeline_ptr == NULL) {
>> +        gtk_stack_set_visible_child_name(d->stack, "draw-area");
>> +        return true;
>> +    }
>> +

Although this would probably always work fine and returned value is
also not checked but the draw-area is not necessarily related to X11
so it should be outside the define.

Consider these, but other than that fine with me

(Oh, another not really necessary thing would be to make sure egl is not
enabled, but probably should never happen :p)

Snir.


>>       /* GstVideoOverlay is currently used only under x */
>>       if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
>>           GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel