[Spice-devel] [spice v16 01/23] streaming: Better check for sized frames

[Francois Gouget]

On Tue, 7 Jun 2016, Frediano Ziglio wrote:
[...]
> >          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
> > @@ -265,7 +266,10 @@ static int is_next_stream_frame(DisplayChannel *display,
> >                  return STREAM_FRAME_NONE;
> >              }
> >  
> > -            if (candidate_area > other_area) {
> > +            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;
> >              }
> >          } else {
> 
> Not a problem of this patch but this code is confusing.
> It's not clear that other_area is not the area or "other_src",
> perhaps would be better renamed to other_dest_area (I was
> going to suggest to remove the first test on the if as
> a result).

other_area is an area, as in pixel^2. So the '_area' suffix makes sense. 
And the 'other_' part makes it clear it corresponds to 'other_dest'. 
You can rename it to 'other_dest_area' but I fail to see how it's 
clearer. Aren't you going to be confused because it matches the 
dest_area field of struct Stream?


-- 
Francois Gouget <fgouget at codeweavers.com>