794200 - When drawing progressively with tiles, draw them in an 'intelligent' order

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

When progressive tile drawing is enabled, tiles are always just drawn from the top-left. As the comment in the file says, we should make some effort to draw these in an order that helps us reduce 'checkerboarding' as much as possible.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Draw tiles with no content before stale tiles

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Draw tiles in order of scroll direction

This changes the iteration order/tile rect calculations so that we draw in the direction of scrolling.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Draw tiles in order of scroll direction

Whoops, fix up CompositorParent.h

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Draw tiles with no content before stale tiles

I had the logic for this the wrong way round. Fixed and named the region a bit better.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 664615 [details] [diff] [review]
Part 2 - Draw tiles in order of scroll direction

Review of attachment 664615 [details] [diff] [review]:
-----------------------------------------------------------------

After convincing myself this seems correct. Two optional nits, take them or leave them.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +256,5 @@
>      if (!emptyRegion.IsEmpty()) {
>        regionToPaint = emptyRegion;
>      }
>  
> +    // Decide what direction to start drawing rects from by checking the

I wonder if this code is now a good candidate for a helper method. As we plan to extend this logic this method may grow. What do you think?

@@ +285,2 @@
>  
> +    int paintTileStartX, paintTileStartY;

Quick comment to make it easier to infer the intent?

// Decide the progressive draw direction for the X and Y axis.
// The direction will follow the scrolling direction.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> Comment on attachment 664615 [details] [diff] [review]
> Part 2 - Draw tiles in order of scroll direction
> 
> Review of attachment 664615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After convincing myself this seems correct. Two optional nits, take them or
> leave them.
> 
> ::: gfx/layers/basic/BasicTiledThebesLayer.cpp
> @@ +256,5 @@
> >      if (!emptyRegion.IsEmpty()) {
> >        regionToPaint = emptyRegion;
> >      }
> >  
> > +    // Decide what direction to start drawing rects from by checking the
> 
> I wonder if this code is now a good candidate for a helper method. As we
> plan to extend this logic this method may grow. What do you think?

I agree, but seeing as bug 794465 moves some of the variables this uses outside of this block, I think it might be worth saving this for when we decide to switch to progressive drawing permanently (or until the logic becomes more complex, whichever comes first).

I'll add a comment about this though.

> @@ +285,2 @@
> >  
> > +    int paintTileStartX, paintTileStartY;
> 
> Quick comment to make it easier to infer the intent?
> 
> // Decide the progressive draw direction for the X and Y axis.
> // The direction will follow the scrolling direction.

Will add.

Comment 7

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fc54501277
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a7067b35fe

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4fc54501277
https://hg.mozilla.org/mozilla-central/rev/49a7067b35fe