Closed Bug 740598 Opened 8 years ago Closed 6 years ago

[Azure] Add DrawTarget::ScrollRect

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: gw280, Assigned: mattwoodrow)

References

Details

Attachments

(2 files, 2 obsolete files)

Add a method to scroll a sub-rect of a DrawTarget by an offset.
Attachment #610683 - Flags: review?(bas.schouten)
Blocks: 740580
I'll need some more explanation as to why we'd want this? This isn't really that consistently supported amongst backends. What is the advantage over taking a snapshot and using CopySurface on that? This also isn't too fact but it's gives the user a better indication of the work that needs to be done by several of the backends to support this functionality.
Because on the backends that *do* support doing this, it's considerable amounts of extra work. Remember with BasicLayers we often move pixels around when the rotated buffer logic gets overly complex. This patch was a considerable perf improvement for skia content on android when I tested it, I don't think we can afford to skip this.

We could make ScrollRect return a bool, and bail out if the operation isn't supported efficiently, but that moves the complexity up into the caller.

I'd vote for taking this, but making sure that it's clearly documented as being potentially slow.
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Because on the backends that *do* support doing this, it's considerable
> amounts of extra work. Remember with BasicLayers we often move pixels around
> when the rotated buffer logic gets overly complex. This patch was a
> considerable perf improvement for skia content on android when I tested it,
> I don't think we can afford to skip this.
> 
> We could make ScrollRect return a bool, and bail out if the operation isn't
> supported efficiently, but that moves the complexity up into the caller.
> 
> I'd vote for taking this, but making sure that it's clearly documented as
> being potentially slow.

I don't understand why for any backend this is a lot faster than a snapshot and then a CopySurface? Unless you're lucky about how exactly the move takes place most backends would need to do some kind of pingpong/intermediate surface. As far as I can tell this would be implemented just as efficiently (if we optimize for it a little), using snapshot/copysurface. Since a snapshot should be copy-on-write anyway. (exactly like a self-copy used in ScrollRect, really)
(In reply to Bas Schouten (:bas) from comment #4)
> I don't understand why for any backend this is a lot faster than a snapshot
> and then a CopySurface? Unless you're lucky about how exactly the move takes
> place most backends would need to do some kind of pingpong/intermediate
> surface. As far as I can tell this would be implemented just as efficiently
> (if we optimize for it a little), using snapshot/copysurface. Since a
> snapshot should be copy-on-write anyway. (exactly like a self-copy used in
> ScrollRect, really)

Looking at the code for Skia, at least, it seems that to do the snapshot/copysurface, we:

- Create an SkBitmap wrapping the pixel data
- Save the current matrix, clip and draw filter and pushes it onto a stack
- Reset the matrix
- Clip the update rect
- Create an SkPaint object
- Set transfer mode on SkPaint
- Create SkBlitter_Sprite object wrapping bitmap data
- Use blitter to blit data in
- Pop matrix/clip/filter off stack

For scrollRect:

- Lock pixels
- memmove each row to the correct place in memory
- Unlock pixels

I don't think any one of these additional operations is particularly costly, but I think they add up.
How would we optimize the Snapshot/CopySurface path to avoid actually making a copy of the surface? (which is what needs to happen to avoid a performance regression here).

The snapshot needs to trigger the copy-on-write path, since someone might attempt to use the snapshot again for a different operation, and CopySurface() will modify the drawtarget.
How about optimising CopySurface in Skia's case to use SkBitmap::scrollRect if our SourceSurfaceSkia
is a current snapshot of our DrawTarget?
Attachment #614161 - Flags: review?(matt.woodrow)
Attachment #614161 - Flags: review?(bas.schouten)
Attachment #610683 - Attachment is obsolete: true
Attachment #610683 - Flags: review?(bas.schouten)
Comment on attachment 614161 [details] [diff] [review]
Bug 740598 - Add an optimised codepath in DrawTargetSkia::CopySurface if we're copying to ourselves.

Oops, just been pointed out to me that we're doing a MarkChanged() here which is resulting in a deep copy of the bitmap to keep the snapshot correct. There doesn't appear to be any way around this with the CopySurface/Snapshot APIs, so yes, we're going to need ScrollRect.
Attachment #614161 - Flags: review?(matt.woodrow)
Attachment #614161 - Flags: review?(bas.schouten)
(In reply to George Wright (:gw280) from comment #8)
> Comment on attachment 614161 [details] [diff] [review]
> Bug 740598 - Add an optimised codepath in DrawTargetSkia::CopySurface if
> we're copying to ourselves.
> 
> Oops, just been pointed out to me that we're doing a MarkChanged() here
> which is resulting in a deep copy of the bitmap to keep the snapshot
> correct. There doesn't appear to be any way around this with the
> CopySurface/Snapshot APIs, so yes, we're going to need ScrollRect.

Hrm, so what maybe we need is a 'live' SourceSurface API that doesn't guarantee it's a snapshot of the surface in the state that it was taken. And then that can be optimized in the way George suggested.
Bas,

I see where you're coming from in comment 2, and agree that having ScrollRect might give the impression that's it's going to be fast across all backends. But, the fact is, we currently need to do a ScrollRect type operation, and I think calling it out explicitly and doing it efficiently on backends that support it makes the most sense. It also makes it easier to keep track of users of ScrollRect and move away from it towards solutions that are more efficient with all backends.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)

Bas just reminded me that implementing ScrollRect on a backend that can't self-copy will need to double-copy which we want to avoid. We need to come up with an interface that is efficient in both cases.
Starting this up again, since we're going to be hitting it with azure on BasicLayers.

Currently working on a quick patch to implement the 'live source surface' idea of Bas' for DrawTargetCairo to see how much of an impact it has on talos.

The current problem I see with this idea is that every function that takes a SourceSurface (or SurfacePattern) now needs to be aware that the source might be itself, and the copy-on-write flush won't fix that. Most backends may need to make temporaries copies of the changed pixels to avoid incorrect rendering.

Going down the ScrollRect route means that we can implement a generic safe version (see the obsolete patch), which has identical performance to the current code, and then backends can opt-in to custom implementations if they think they can do better.

Jeff, what did you mean by double-copy? The current code is doing a full copy of the entire surface into a snapshot, and then copying back the relevant pixels, I don't see how a ScrollRect method would do *worse* than that. At the very least we can only make a copy of the relevant pixels, and at best make no copy at all.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
Blocks: 916034
Attachment #614161 - Attachment is obsolete: true
Attachment #815233 - Flags: review?(bas)
Attachment #815234 - Flags: review?(bas)
Assignee: gwright → matt.woodrow
Attachment #815233 - Flags: review?(bas) → review+
Comment on attachment 815234 [details] [diff] [review]
Optimize CopyRect for cairo

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +140,5 @@
> +#ifdef CAIRO_HAS_WIN32_SURFACE
> +    case CAIRO_SURFACE_TYPE_WIN32:
> +    case CAIRO_SURFACE_TYPE_WIN32_PRINTING:
> +      return true;
> +#endif

Direct2D supports self-copy in a more optimized way than below for the sort of operations we should be dealing with here (axis-aligned, no clip, etc). We might want to add it onto the list, although ideally we shouldn't ever be using a cairo backend with Direct2D so I'm not sure we need to care.
Attachment #815234 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/b3a790613001
https://hg.mozilla.org/mozilla-central/rev/0cb34784d9da
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Flags: needinfo?(bas)
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.