[Spice-devel,v16,02/23] streaming: Remove the Drawable.sized_stream field

Submitted by Francois Gouget on June 7, 2016, 1:58 p.m.

Details

Message ID 62e357107b426e9b298ab771599cc593a40a34dc.1465306176.git.fgouget@free.fr
State New
Headers show
Series "Add GStreamer support for video streaming" ( rev: 18 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget June 7, 2016, 1:58 p.m.
Only red_marshall_stream_data() needs to know whether to send the frame
using a SpiceMsgDisplayStreamDataSized or a regular StreamData message.
So check whether we have a sized frame there and simplify the rest of
the code.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 server/dcc-send.c        | 39 +++++++++++-----------------
 server/display-channel.c |  2 +-
 server/display-channel.h |  1 -
 server/stream.c          | 67 +++++++++++++++---------------------------------
 server/stream.h          |  8 +-----
 5 files changed, 38 insertions(+), 79 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 5171f9a..798a9b3 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -1657,36 +1657,27 @@  static int red_marshall_stream_data(RedChannelClient *rcc,
     DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
     DisplayChannel *display = DCC_TO_DC(dcc);
     Stream *stream = drawable->stream;
-    SpiceImage *image;
+    SpiceCopy *copy;
     uint32_t frame_mm_time;
     uint32_t n;
-    int width, height;
+    int is_sized, width, height;
     int ret;
 
-    if (!stream) {
-        spice_assert(drawable->sized_stream);
-        stream = drawable->sized_stream;
-    }
     spice_assert(drawable->red_drawable->type == QXL_DRAW_COPY);
 
-    image = drawable->red_drawable->u.copy.src_bitmap;
-
-    if (image->descriptor.type != SPICE_IMAGE_TYPE_BITMAP) {
+    copy = &drawable->red_drawable->u.copy;
+    if (copy->src_bitmap->descriptor.type != SPICE_IMAGE_TYPE_BITMAP) {
         return FALSE;
     }
 
-    if (drawable->sized_stream) {
-        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
-            SpiceRect *src_rect = &drawable->red_drawable->u.copy.src_area;
+    width = copy->src_area.right - copy->src_area.left;
+    height = copy->src_area.bottom - copy->src_area.top;
+    is_sized = (width != stream->width) || (height != stream->height) ||
+               !rect_is_equal(&drawable->red_drawable->bbox, &stream->dest_area);
 
-            width = src_rect->right - src_rect->left;
-            height = src_rect->bottom - src_rect->top;
-        } else {
-            return FALSE;
-        }
-    } else {
-        width = stream->width;
-        height = stream->height;
+    if (is_sized &&
+        !red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
+        return FALSE;
     }
 
     StreamAgent *agent = &dcc->stream_agents[get_stream_id(display, stream)];
@@ -1710,8 +1701,8 @@  static int red_marshall_stream_data(RedChannelClient *rcc,
     outbuf_size = dcc->send_data.stream_outbuf_size;
     ret = agent->video_encoder->encode_frame(agent->video_encoder,
                                              frame_mm_time,
-                                             &image->u.bitmap, width, height,
-                                             &drawable->red_drawable->u.copy.src_area,
+                                             &copy->src_bitmap->u.bitmap,
+                                             width, height, &copy->src_area,
                                              stream->top_down,
                                              &dcc->send_data.stream_outbuf,
                                              &outbuf_size, &n);
@@ -1732,7 +1723,7 @@  static int red_marshall_stream_data(RedChannelClient *rcc,
     }
     dcc->send_data.stream_outbuf_size = outbuf_size;
 
-    if (!drawable->sized_stream) {
+    if (!is_sized) {
         SpiceMsgDisplayStreamData stream_data;
 
         red_channel_client_init_send_data(rcc, SPICE_MSG_DISPLAY_STREAM_DATA, NULL);
@@ -2132,7 +2123,7 @@  static void marshall_qxl_drawable(RedChannelClient *rcc,
     spice_return_if_fail(display);
     /* allow sized frames to be streamed, even if they where replaced by another frame, since
      * newer frames might not cover sized frames completely if they are bigger */
-    if ((item->stream || item->sized_stream) && red_marshall_stream_data(rcc, m, item)) {
+    if (item->stream && red_marshall_stream_data(rcc, m, item)) {
         return;
     }
     if (display->enable_jpeg)
diff --git a/server/display-channel.c b/server/display-channel.c
index 2888cad..0ba4064 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1396,7 +1396,7 @@  void drawable_unref(Drawable *drawable)
     spice_warn_if_fail(ring_is_empty(&drawable->pipes));
 
     if (drawable->stream) {
-        detach_stream(display, drawable->stream, TRUE);
+        detach_stream(display, drawable->stream);
     }
     region_destroy(&drawable->tree_item.base.rgn);
 
diff --git a/server/display-channel.h b/server/display-channel.h
index 16ea709..5891e94 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -70,7 +70,6 @@  struct Drawable {
     int gradual_frames_count;
     int last_gradual_frame;
     Stream *stream;
-    Stream *sized_stream;
     int streamable;
     BitmapGradualType copy_bitmap_graduality;
     DependItem depend_items[3];
diff --git a/server/stream.c b/server/stream.c
index d8be148..e100a3c 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -226,15 +226,14 @@  static int is_next_stream_frame(DisplayChannel *display,
                                 int container_candidate_allowed)
 {
     RedDrawable *red_drawable;
-    int is_frame_container = FALSE;
 
     if (!candidate->streamable) {
-        return STREAM_FRAME_NONE;
+        return FALSE;
     }
 
     if (candidate->creation_time - other_time >
             (stream ? RED_STREAM_CONTINUS_MAX_DELTA : RED_STREAM_DETACTION_MAX_DELTA)) {
-        return STREAM_FRAME_NONE;
+        return FALSE;
     }
 
     red_drawable = candidate->red_drawable;
@@ -242,17 +241,16 @@  static int is_next_stream_frame(DisplayChannel *display,
         SpiceRect* candidate_src;
 
         if (!rect_is_equal(&red_drawable->bbox, other_dest)) {
-            return STREAM_FRAME_NONE;
+            return FALSE;
         }
 
         candidate_src = &red_drawable->u.copy.src_area;
         if (candidate_src->right - candidate_src->left != other_src_width ||
             candidate_src->bottom - candidate_src->top != other_src_height) {
-            return STREAM_FRAME_NONE;
+            return FALSE;
         }
     } else {
         if (rect_contains(&red_drawable->bbox, other_dest)) {
-            SpiceRect* candidate_src;
             int candidate_area = rect_get_area(&red_drawable->bbox);
             int other_area = rect_get_area(other_dest);
             /* do not stream drawables that are significantly
@@ -263,31 +261,20 @@  static int is_next_stream_frame(DisplayChannel *display,
                 rect_debug(other_dest);
                 spice_debug("new box ==>");
                 rect_debug(&red_drawable->bbox);
-                return STREAM_FRAME_NONE;
-            }
-
-            candidate_src = &red_drawable->u.copy.src_area;
-            if (candidate_area > other_area ||
-                candidate_src->right - candidate_src->left != other_src_width ||
-                candidate_src->bottom - candidate_src->top != other_src_height) {
-                is_frame_container = TRUE;
+                return FALSE;
             }
         } else {
-            return STREAM_FRAME_NONE;
+            return FALSE;
         }
     }
 
     if (stream) {
         SpiceBitmap *bitmap = &red_drawable->u.copy.src_bitmap->u.bitmap;
         if (stream->top_down != !!(bitmap->flags & SPICE_BITMAP_FLAGS_TOP_DOWN)) {
-            return STREAM_FRAME_NONE;
+            return FALSE;
         }
     }
-    if (is_frame_container) {
-        return STREAM_FRAME_CONTAINER;
-    } else {
-        return STREAM_FRAME_NATIVE;
-    }
+    return TRUE;
 }
 
 static void attach_stream(DisplayChannel *display, Drawable *drawable, Stream *stream)
@@ -334,15 +321,11 @@  static void attach_stream(DisplayChannel *display, Drawable *drawable, Stream *s
     }
 }
 
-void detach_stream(DisplayChannel *display, Stream *stream,
-                   int detach_sized)
+void detach_stream(DisplayChannel *display, Stream *stream)
 {
     spice_assert(stream->current && stream->current->stream);
     spice_assert(stream->current->stream == stream);
     stream->current->stream = NULL;
-    if (detach_sized) {
-        stream->current->sized_stream = NULL;
-    }
     stream->current = NULL;
 }
 
@@ -538,16 +521,13 @@  void stream_trace_update(DisplayChannel *display, Drawable *drawable)
                                                  stream->last_time,
                                                  stream,
                                                  TRUE);
-        if (is_next_frame != STREAM_FRAME_NONE) {
+        if (is_next_frame) {
             if (stream->current) {
                 stream->current->streamable = FALSE; //prevent item trace
                 before_reattach_stream(display, stream, drawable);
-                detach_stream(display, stream, FALSE);
+                detach_stream(display, stream);
             }
             attach_stream(display, drawable, stream);
-            if (is_next_frame == STREAM_FRAME_CONTAINER) {
-                drawable->sized_stream = stream;
-            }
             return;
         }
     }
@@ -556,8 +536,7 @@  void stream_trace_update(DisplayChannel *display, Drawable *drawable)
     trace_end = trace + NUM_TRACE_ITEMS;
     for (; trace < trace_end; trace++) {
         if (is_next_stream_frame(display, drawable, trace->width, trace->height,
-                                       &trace->dest_area, trace->time, NULL, FALSE) !=
-                                       STREAM_FRAME_NONE) {
+                                       &trace->dest_area, trace->time, NULL, FALSE)) {
             if (stream_add_frame(display, drawable,
                                  trace->first_frame_time,
                                  trace->frames_count,
@@ -585,14 +564,11 @@  void stream_maintenance(DisplayChannel *display,
                                              stream->width, stream->height,
                                              &stream->dest_area, stream->last_time,
                                              stream, TRUE);
-        if (is_next_frame != STREAM_FRAME_NONE) {
+        if (is_next_frame) {
             before_reattach_stream(display, stream, candidate);
-            detach_stream(display, stream, FALSE);
+            detach_stream(display, stream);
             prev->streamable = FALSE; //prevent item trace
             attach_stream(display, candidate, stream);
-            if (is_next_frame == STREAM_FRAME_CONTAINER) {
-                candidate->sized_stream = stream;
-            }
         }
     } else if (candidate->streamable) {
         SpiceRect* prev_src = &prev->red_drawable->u.copy.src_area;
@@ -603,7 +579,7 @@  void stream_maintenance(DisplayChannel *display,
                                  &prev->red_drawable->bbox, prev->creation_time,
                                  prev->stream,
                                  FALSE);
-        if (is_next_frame != STREAM_FRAME_NONE) {
+        if (is_next_frame) {
             stream_add_frame(display, candidate,
                              prev->first_frame_time,
                              prev->frames_count,
@@ -833,13 +809,12 @@  static void dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
         /* (1) The caller should detach the drawable from the stream. This will
          * lead to sending the drawable losslessly, as an ordinary drawable. */
         if (dcc_drawable_is_in_pipe(dcc, stream->current)) {
-            spice_debug("stream %d: upgrade by linked drawable. sized %d, box ==>",
-                        stream_id, stream->current->sized_stream != NULL);
+            spice_debug("stream %d: upgrade by linked drawable. box ==>",
+                        stream_id);
             rect_debug(&stream->current->red_drawable->bbox);
             goto clear_vis_region;
         }
-        spice_debug("stream %d: upgrade by drawable. sized %d, box ==>",
-                    stream_id, stream->current->sized_stream != NULL);
+        spice_debug("stream %d: upgrade by drawable. box ==>", stream_id);
         rect_debug(&stream->current->red_drawable->bbox);
         rcc = RED_CHANNEL_CLIENT(dcc);
         upgrade_item = spice_new(RedUpgradeItem, 1);
@@ -882,7 +857,7 @@  static void detach_stream_gracefully(DisplayChannel *display, Stream *stream,
         dcc_detach_stream_gracefully(dcc, stream, update_area_limit);
     }
     if (stream->current) {
-        detach_stream(display, stream, TRUE);
+        detach_stream(display, stream);
     }
 }
 
@@ -919,11 +894,11 @@  void stream_detach_behind(DisplayChannel *display, QRegion *region, Drawable *dr
             }
         }
         if (detach && stream->current) {
-            detach_stream(display, stream, TRUE);
+            detach_stream(display, stream);
         } else if (!is_connected) {
             if (stream->current &&
                 region_intersects(&stream->current->tree_item.base.rgn, region)) {
-                detach_stream(display, stream, TRUE);
+                detach_stream(display, stream);
             }
         }
     }
diff --git a/server/stream.h b/server/stream.h
index 715f920..9dcb8f7 100644
--- a/server/stream.h
+++ b/server/stream.h
@@ -52,12 +52,6 @@  typedef struct RedStreamActivateReportItem {
     uint32_t stream_id;
 } RedStreamActivateReportItem;
 
-enum {
-    STREAM_FRAME_NONE,
-    STREAM_FRAME_NATIVE,
-    STREAM_FRAME_CONTAINER,
-};
-
 #define STREAM_STATS
 #ifdef STREAM_STATS
 typedef struct StreamStats {
@@ -161,6 +155,6 @@  void                  stream_agent_unref                            (DisplayChan
                                                                      StreamAgent *agent);
 void                  stream_agent_stop                             (StreamAgent *agent);
 
-void detach_stream(DisplayChannel *display, Stream *stream, int detach_sized);
+void detach_stream(DisplayChannel *display, Stream *stream);
 
 #endif /* STREAM_H */

Comments

Hey,

Thanks for working on this, makes the code easier to follow!
Imo it's easier to review if (artificially) split, I'll send what I did
as a followup to this patch.

Just one minor comment below:

On Tue, Jun 07, 2016 at 03:58:00PM +0200, Francois Gouget wrote:
> Only red_marshall_stream_data() needs to know whether to send the frame
> using a SpiceMsgDisplayStreamDataSized or a regular StreamData message.
> So check whether we have a sized frame there and simplify the rest of
> the code.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/dcc-send.c        | 39 +++++++++++-----------------
>  server/display-channel.c |  2 +-
>  server/display-channel.h |  1 -
>  server/stream.c          | 67 +++++++++++++++---------------------------------
>  server/stream.h          |  8 +-----
>  5 files changed, 38 insertions(+), 79 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 5171f9a..798a9b3 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -1657,36 +1657,27 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>      DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
>      DisplayChannel *display = DCC_TO_DC(dcc);
>      Stream *stream = drawable->stream;
> -    SpiceImage *image;
> +    SpiceCopy *copy;
>      uint32_t frame_mm_time;
>      uint32_t n;
> -    int width, height;
> +    int is_sized, width, height;
>      int ret;
>  
> -    if (!stream) {
> -        spice_assert(drawable->sized_stream);
> -        stream = drawable->sized_stream;
> -    }

We could keep some check that 'drawable->stream' is not NULL, maybe not
an assert, but a g_return_val_if_fail(stream != NULL, FALSE); ?

Acked-by: Christophe Fergeau <cfergeau@redhat.com>

Christophe
On Fri, Jun 10, 2016 at 03:45:50PM +0200, Christophe Fergeau wrote:
> Imo it's easier to review if (artificially) split, I'll send what I did
> as a followup to this patch.

I kept yourself as the author of the followup patches as I just moved
things around, let me know
1) if you think the split patches are fine, or if you prefer the current
version
2) if you prefer that authorship is handled differently, let me know too
;)

Christophe