Closed Bug 794200 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: Layers, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attachment #664611 - Flags: review?(bgirard)
This changes the iteration order/tile rect calculations so that we draw in the direction of scrolling.
Attachment #664613 - Flags: review?(bgirard)
Whoops, fix up CompositorParent.h
Attachment #664613 - Attachment is obsolete: true
Attachment #664613 - Flags: review?(bgirard)
Attachment #664615 - Flags: review?(bgirard)
I had the logic for this the wrong way round. Fixed and named the region a bit better.
Attachment #664611 - Attachment is obsolete: true
Attachment #664611 - Flags: review?(bgirard)
Attachment #664945 - Flags: review?(bgirard)
Depends on: 771219
Attachment #664945 - Flags: review?(bgirard) → review+
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.
Attachment #664615 - Flags: review?(bgirard) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d4fc54501277
https://hg.mozilla.org/mozilla-central/rev/49a7067b35fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 795259
You need to log in before you can comment on or make changes to this bug.