1388842 - Add layerization (display item grouping) to layers-free

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

In the likely case that using one blob image per display item is not workable we'll need to group display items together.

My current plan for this is to have a wrapping display item for svg content. All of the associated svg display items will be grouped into a single blob image.

As a second step, it should be pretty easy to break out 'opacity' and 'transform' items into separate blob images.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

This has the pseudo code of the current plan. I'm going to keep filling it out.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP 2

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP 3

What I currently have

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP 4

This includes all the major pieces needed to get things working. It still needs some stuff filled in to actually work but it does build.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP 4

Some pieces required to build were missing

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

This starts hooking up painting and retaining of the data in userdata

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Things should mostly be there. I've started debugging it. It has bugs.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

The previous version was the wrong patch.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Doesn't crash anymore, but doesn't paint.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Basic painting now works. I haven't test it too much.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Further fixes. I'm going on PTO for a week and will resume work on this when I return.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Rebased

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

This fixes situations where we would not iterate over children

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

This turns on invalidation and fixes some bugs

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Attachment: Group svg content into a single blob image

Comment 26

[Markus Stange [:mstange]]

Jeff and I have started working on this properly. We're going to be committing to the "blob-invalidation" bookmark branch on my mozilla-central user repo: https://hg.mozilla.org/users/mstange_themasta.com/mozilla-central/pushloghtml
hg pull -B blob-invalidation https://hg.mozilla.org/users/mstange_themasta.com/mozilla-central/
hg push -r . -B blob-invalidation ssh://hg.mozilla.org/users/mstange_themasta.com/mozilla-central/

Once things are working correctly, we're going to diff the result with mozilla-central and split that diff up into reviewable pieces, and then attach those to this bug.

Here's a list of work that remains to be done: https://public.etherpad-mozilla.org/p/wr-blob-image-invalidation-missing-pieces

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Attachment: The whole thing

I'm going to break and clean it up a bit before requesting review.

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Attachment: The whole thing

Comment 29

[Jeff Muizelaar [:jrmuizel]]

I'll continue to clean.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Attachment: The whole thing

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 32

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 35

[Markus Stange [:mstange]]

Comment on attachment 8953579 [details] [diff] [review]
Add support for updating blob images

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

Please make all new comments start with an uppercase letter and end with a full stop.

I'd like to review the merging again once the byte stream format has been documented.

::: gfx/2d/2D.h
@@ +1454,5 @@
>    bool GetPermitSubpixelAA() {
>      return mPermitSubpixelAA;
>    }
>  
> +  virtual void FlushItem(IntRect aBounds) {}

const IntRect&

::: gfx/2d/DrawEventRecorder.cpp
@@ +93,5 @@
>  {
>  }
>  
>  void
>  DrawEventRecorderMemory::FlushItem(IntRect aRect)

Might as well fix this one to use a const ref, too.

::: gfx/2d/DrawTargetRecording.cpp
@@ +651,5 @@
> +void
> +DrawTargetRecording::FlushItem(IntRect aBounds)
> +{
> +  mRecorder->FlushItem(aBounds);
> +  // reinitialize the recorder with us a the drawtarget (flush clears this)

a -> as

I don't really understand the lifecycle of the recorded DrawTargets.

::: gfx/webrender_bindings/Moz2DImageRenderer.cpp
@@ +246,5 @@
>    size_t offset = 0;
>    while (reader.pos < reader.len) {
>      size_t end = reader.ReadSize();
>      size_t extra_end = reader.ReadSize();
> +    reader.SkipBounds();

Is there a description of the shape / grammar of the byte stream? This line needs a comment that points to that description.

::: gfx/webrender_bindings/src/moz2d_renderer.rs
@@ +73,1 @@
>  struct BufReader<'a>

Might want to rename this to BlobImageByteStreamReader or something

@@ +96,5 @@
>      }
> +
> +    fn read_entry(&mut self) -> (usize, usize, Box2d) {
> +        let end = self.read();
> +        let extra_end = self.read();

This needs a comment. What is the meaning of end and extra_end?

@@ +104,5 @@
> +}
> +
> +// This is used for writing new blob images.
> +// In our case this is the result of merging an old one and a new one
> +struct BufWriter

This could use a better name. BlobImageByteStreamWriter?

@@ +105,5 @@
> +
> +// This is used for writing new blob images.
> +// In our case this is the result of merging an old one and a new one
> +struct BufWriter
> +{

rust style puts the opening brace on the previous line, I think

@@ +107,5 @@
> +// In our case this is the result of merging an old one and a new one
> +struct BufWriter
> +{
> +    buf: Vec<u8>,
> +    index: Vec<u8>

These fields need documentation.

@@ +116,5 @@
> +    fn new() -> BufWriter {
> +        BufWriter{ buf: Vec::new(), index: Vec::new() }
> +    }
> +
> +    fn new_entry(&mut self, extra_size: usize, bounds: Box2d, data: &[u8]) {

Please document the parameters.

@@ +120,5 @@
> +    fn new_entry(&mut self, extra_size: usize, bounds: Box2d, data: &[u8]) {
> +        self.buf.extend_from_slice(data);
> +        self.index.extend_from_slice(convert_to_bytes(&(self.buf.len() - extra_size)));
> +        self.index.extend_from_slice(convert_to_bytes(&self.buf.len()));
> +        // XXX: we can agregate these writes

aggregate

@@ +148,5 @@
> +    y2: u32
> +}
> +
> +impl Box2d {
> +    fn contained_by(&self, other: &Box2d) -> bool {

Interestingly passive twist. I would have expected a "contains" method with inverted parameters.

@@ +160,5 @@
> +        self.x2 > other.x1 &&
> +        self.y1 < other.y2 &&
> +        self.y2 > other.y1
> +    }
> +    fn overlaps(&self, other: &Box2d) -> bool {

call this partially_overlaps

@@ +165,5 @@
> +        self.intersects(other) && !self.contained_by(other)
> +    }
> +}
> +
> +fn create_index_reader(buf: &[u8]) -> BufReader {

Should this be a creation method on BufReader, maybe?

@@ +189,5 @@
> +        );
> +    }
> +}
> +
> +/* The invarients that we need for this to work properly are that

invariants

Should probably use // or ///-style comments here

@@ +194,5 @@
> + * - all new content is contained in the dirty_rect
> + * - all content that overlaps with the dirty_rect is included in the new index
> + *   this is needed so that we can properly synchronize our buffers
> + */
> +fn merge_blob_images(old: &[u8], new: &[u8], dirty_rect: DeviceUintRect, ) -> Vec<u8> {

don't need the trailing comma

@@ +205,5 @@
> +    dump_blob_index(old, dirty_rect);
> +    dlog!("new:");
> +    dump_blob_index(new, dirty_rect);
> +
> +    let mut index = create_index_reader(old);

index_reader_old

@@ +206,5 @@
> +    dlog!("new:");
> +    dump_blob_index(new, dirty_rect);
> +
> +    let mut index = create_index_reader(old);
> +    let mut new_index = create_index_reader(new);

index_reader_new

@@ +261,5 @@
> +    }
> +
> +    let result = result.finish();
> +    {
> +        let mut index = create_index_reader(&result);

index_reader

@@ +266,5 @@
> +        assert!(index.pos < index.buf.len(), "Unexpectedly empty result. This blob should just have been deleted");
> +        while index.pos < index.buf.len() {
> +            let (extra, end, bounds) = index.read_entry();
> +            dlog!("result bounds: {} {} {:?}", extra, end, bounds);
> +        }

Is this section just to print debug information?

Comment 36

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

I missed a couple of comments, but I need to catch a train and this should be enough to get to the next stage of review.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

There were some issues with the previous patch.

Comment 39

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 40

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

More cleaner.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

Comment 42

[Markus Stange [:mstange]]

Comment on attachment 8954488 [details] [diff] [review]
Add support for updating blob images

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

::: gfx/2d/DrawEventRecorder.cpp
@@ +105,3 @@
>    WriteElement(mIndex, mOutputStream.mLength);
> +
> +  // write out the fonts into the extra data section

this is a sentence

::: gfx/2d/DrawTargetRecording.cpp
@@ +648,5 @@
>    return pathRecording.forget();
>  }
>  
> +// This should only be called on the 'root' DrawTargetRecording.
> +// Calling it on a child DrawTargetRecordings will cause confusion.

s/DrawTargetRecordings/DrawTargetRecording/

@@ +662,5 @@
> +                                                    mSize,
> +                                                    mFinalDT->GetFormat(),
> +                                                    false, nullptr));
> +  // Add the current transform to the new recording
> +  mRecorder->RecordEvent(RecordedSetTransform(this, DrawTarget::GetTransform()));

Maybe assert that the clip stack is empty? If this DrawTarget keeps track of clips, not sure if it does.

::: gfx/webrender_bindings/src/moz2d_renderer.rs
@@ +118,5 @@
> + *  - the index is smaller so we append it to the end of the data array
> + *  during construction. This makes it more likely that we'll fit inside
> + *  the data Vec
> + *  - we use indices/offsets instead of sizes to avoid having to deal with any
> + *  arithmetic that might overflow.

Thank you for this explanation, I hadn't thought about the overflow aspect.

@@ +122,5 @@
> + *  arithmetic that might overflow.
> + */
> +
> +
> +struct BlobReader<'a> {

This seems to only read the index. Rename to BlobIndexReader?

@@ +157,5 @@
> +    }
> +
> +    fn new_entry(&mut self, extra_size: usize, bounds: Box2d, data: &[u8]) {
> +        self.data.extend_from_slice(data);
> +        self.index.extend_from_slice(convert_to_bytes(&(self.data.len() - extra_size)));

Add comment: // Write 'end' to the index: the offset where the regular data ends and the extra data starts.

@@ +158,5 @@
> +
> +    fn new_entry(&mut self, extra_size: usize, bounds: Box2d, data: &[u8]) {
> +        self.data.extend_from_slice(data);
> +        self.index.extend_from_slice(convert_to_bytes(&(self.data.len() - extra_size)));
> +        self.index.extend_from_slice(convert_to_bytes(&self.data.len()));

Add comment: // Write 'extra_end' to the index: the offset where the extra data ends.

@@ +160,5 @@
> +        self.data.extend_from_slice(data);
> +        self.index.extend_from_slice(convert_to_bytes(&(self.data.len() - extra_size)));
> +        self.index.extend_from_slice(convert_to_bytes(&self.data.len()));
> +        // XXX: we can aggregate these writes
> +        self.index.extend_from_slice(convert_to_bytes(&bounds.x1));

Add comment: // Write the bounds to the index.

@@ +179,5 @@
> +
> +
> +// XXX: Do we want to allow negative values here or clamp to the image bounds?
> +#[derive(Debug, Eq, PartialEq, Clone, Copy)]
> +struct Box2d {

You could implement From<DeviceUintRect> for Box2d

@@ +199,5 @@
> +        self.x2 > other.x1 &&
> +        self.y1 < other.y2 &&
> +        self.y2 > other.y1
> +    }
> +    fn partially_overlaps(&self, other: &Box2d) -> bool {

It looks like all callers of this already check self.contained_by(other) before they call partially_overlaps. So I think you could just remove this method and call intersects everywhere.

@@ +234,5 @@
> +/* The invariants that we need for this to work properly are that
> + * - all new content is contained in the dirty_rect
> + * - all content that overlaps with the dirty_rect is included in the new index
> + *   this is needed so that we can properly synchronize our buffers
> + */

This comment needs to be expanded.

// Merge a new partial blob image into an existing complete blob image.
// The result a buffer with the data for the merged blob image.
// All data from the new blob image will be present in the merged blob image.
// Every item from the existing blob image which intersects dirty_rect will be discarded,
// but these items will be used as "sync points" to arrive at a correct order.

@@ +255,5 @@
> +    // overlap but are not contained by the dirty rect
> +    let mut begin = 0;
> +    let mut new_begin = 0;
> +    dlog!("dirty rect: {:?}", dirty_rect);
> +    while old_reader.reader.pos < old_reader.reader.buf.len() {

I think this calls for a has_more() method on BlobIndexReader.

@@ +256,5 @@
> +    let mut begin = 0;
> +    let mut new_begin = 0;
> +    dlog!("dirty rect: {:?}", dirty_rect);
> +    while old_reader.reader.pos < old_reader.reader.buf.len() {
> +        let (extra, end, bounds) = old_reader.read_entry();

Please rename extra to end and end to extra_end to stay consistent with the naming in the blob stream description. Or alternatively, rename these things everywhere else.

@@ +261,5 @@
> +        dlog!("bounds: {} {} {:?}", extra, end, bounds);
> +        if bounds.contained_by(&dirty_rect) {
> +            dlog!("skip");
> +            // Skip these items as they will be replaced with items from new.
> +        } else if bounds.partially_overlaps(&dirty_rect) {

I would do the !intersects part first, and put the most complicated case into the last if branch.

@@ +263,5 @@
> +            dlog!("skip");
> +            // Skip these items as they will be replaced with items from new.
> +        } else if bounds.partially_overlaps(&dirty_rect) {
> +            // This is a sync point between the old and new lists
> +            // find matching rect in new list.

these are two separate sentences

"matching rect" doesn't really correspond to what you actually end up checking for.

Also add: "This item will be present in the new list unchanged. It's unchanged because it's not fully contained in the dirty rect, and it's present because it intersects the dirty rect."

@@ +265,5 @@
> +        } else if bounds.partially_overlaps(&dirty_rect) {
> +            // This is a sync point between the old and new lists
> +            // find matching rect in new list.
> +            while new_reader.reader.pos < new_reader.reader.buf.len() {
> +                let (new_extra, new_end, new_bounds) = new_reader.read_entry();

same rename request as above

@@ +271,5 @@
> +
> +                if new_bounds.contained_by(&dirty_rect) {
> +                    dlog!("new item");
> +                    result.new_entry(new_end - new_extra, new_bounds, &new[new_begin..new_end]);
> +                } else if new_bounds.partially_overlaps(&dirty_rect) {

Add comment: "This item is an unchanged item which is also present in the old list. If it had changed, it would be fully contained in the dirty rect."

@@ +272,5 @@
> +                if new_bounds.contained_by(&dirty_rect) {
> +                    dlog!("new item");
> +                    result.new_entry(new_end - new_extra, new_bounds, &new[new_begin..new_end]);
> +                } else if new_bounds.partially_overlaps(&dirty_rect) {
> +                    // You might think that bounds == new_bounds, but sequence points might not be in the same

missing the word "order"

@@ +274,5 @@
> +                    result.new_entry(new_end - new_extra, new_bounds, &new[new_begin..new_end]);
> +                } else if new_bounds.partially_overlaps(&dirty_rect) {
> +                    // You might think that bounds == new_bounds, but sequence points might not be in the same
> +                    // because of an earlier update. We don't need them to be in the same order
> +                    // we just need the same number of them.

It looks like this doesn't actually work. Example:
old:
0, 0, 100, 100
250, 50, 350, 150
200, 0, 300, 100
80, 20, 220, 120

dirty_rect: 80, 20, 220, 120
new:
200, 0, 300, 100
0, 0, 100, 100

merged should be:
0, 0, 100, 100
250, 50, 350, 150
200, 0, 300, 100

or merged should be:
250, 50, 350, 150
0, 0, 100, 100
200, 0, 300, 100

or merged should be:
250, 50, 350, 150
200, 0, 300, 100
0, 0, 100, 100

but this algorithm computes:
200, 0, 300, 100
250, 50, 350, 150
0, 0, 100, 100

@@ +276,5 @@
> +                    // You might think that bounds == new_bounds, but sequence points might not be in the same
> +                    // because of an earlier update. We don't need them to be in the same order
> +                    // we just need the same number of them.
> +                    result.new_entry(new_end - new_extra, new_bounds, &new[new_begin..new_end]);
> +                    new_begin = new_end;

It would be nice if the new_begin state lived in the BlobIndexReader and the BlobIndexReader returned { begin, end, extra_end, bounds } from read_entry().

Comment 43

[Markus Stange [:mstange]]

Here's a better example of a broken case, where the "true" ordering doesn't change.

You start out with:

state I
-------
C 0, 0, 100, 100
D 80, 20, 220, 120

Then you add two overlapping rectangles A, B at the bottom; B overlaps with D.

state II
--------
A 250, 50, 350, 150
B 200, 0, 300, 100
C 0, 0, 100, 100
D 80, 20, 220, 120

Then you remove D.

state III
---------
A 250, 50, 350, 150
B 200, 0, 300, 100
C 0, 0, 100, 100


Here are the updates and what your algorithm produces as the merged image:

state I -> state II
-------------------

dirty_rect: 200, 0, 350, 150
A 250, 50, 350, 150
B 200, 0, 300, 100
D 80, 20, 220, 120

merged:
C 0, 0, 100, 100
A 250, 50, 350, 150
B 200, 0, 300, 100
D 80, 20, 220, 120


state II -> state III
---------------------

dirty_rect: 80, 20, 220, 120 
B 200, 0, 300, 100
C 0, 0, 100, 100

merged:
B 200, 0, 300, 100
A 250, 50, 350, 150
C 0, 0, 100, 100

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

This rewrites the blob merging algorithm. It does not address any of the other review comments yet.

It is based off of the reference merge algorithm from bug 1439809:

"For-each item i in the new list:
    If the item has a matching item in the old list:
        Remove items from the start of the old list up until we reach an item that also exists in the new list (leaving the matched item in place):
            Add valid items to the merged list, destroy invalid items.
    Add i into the merged list.
    If the start of the old list matches i, remove and destroy it, otherwise mark the old version of i as used.
Add all remaining valid items from the old list into the merged list, skipping over (and destroying) any that are marked as used."

It modifies it slightly: The 'used' marking is unneeded because it is only used in the last step and we know that any remaining partially overlapping items will already have been dealt with from the new list and therefore can all be discarded.

We use bounds equality to stand in for item equality. Hopefully that's ok

Comment 45

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

This rewrites the algorithm again Matt's new algorithm from bug 1443027. It seems passes all of test cases, but I can't say I'm completely confident that it always works.

Comment 46

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

This should have all the review comments addressed. I'm pretty happy with how it turned out.

Comment 47

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

A couple more things that I forgot to address along with a bonus simplification.

Comment 48

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for updating blob images

This rewrites the blob merging to be as simple as possible. We assume all of the old items are always sent and we only replace items that are in the dirty rect.

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 8958476 [details] [diff] [review]
Add support for updating blob images

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

Looks good!

::: gfx/webrender_bindings/src/moz2d_renderer.rs
@@ +237,5 @@
> +
> +/* Merge a new partial blob image into an existing complete blob image.
> +   All of the items not fully contained in the dirty_rect should match
> +   in both new and old lists.
> +   We use continue to use the old content for these items.

double "use"

@@ +255,5 @@
> +    let mut new_reader = BlobReader::new(new);
> +
> +    // Loop over both new and old entries merging them.
> +    // Both new and old must have the same number of entries that
> +    // overlap but are not contained by the dirty rect

, and they must be in the same order.

@@ +267,5 @@
> +                assert!(old_reader.reader.has_more());
> +                let (old_begin, old_end, old_extra, old_bounds) = old_reader.read_entry();
> +                dlog!("new bounds: {} {} {:?}", old_end, old_extra, old_bounds);
> +                if old_bounds.contained_by(&dirty_rect) {
> +                    // fully contained items will be replaced

maybe use "discarded" instead of "replaced" because there might not be a replacement for this item

Comment 50

[Markus Stange [:mstange]]

Comment on attachment 8954462 [details] [diff] [review]
Add blob invalidation

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

I haven't dug into the meat of this patch yet, but here are the comments that I had on my first quick read through.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +59,5 @@
> +NS_DECLARE_FRAME_PROPERTY_WITH_DTOR(BlobGroupDataProperty,
> +                                    nsTArray<BlobItemData*>,
> +                                    RemoveFrameFromBlobGroup);
> +
> +struct BlobItemData {

opening brace goes on the next line

@@ +137,5 @@
> +    }
> +  }
> +};
> +
> +BlobItemData*

static

@@ +154,5 @@
> +  return nullptr;
> +}
> +
> +// We keep around the BlobItemData so that when we invalidate it get properly included in the rect
> +void RemoveFrameFromBlobGroup(nsTArray<BlobItemData*>* aArray) {

static

@@ +194,5 @@
> +    GP("Group count: %d\n", mGroupCount);
> +  }
> +};
> +
> +bool LayerItem(nsDisplayItem* aItem) {

// Returns whether this is an item for which complete invalidation was
// reliant on LayerTreeInvalidation in the pre-webrender world.
static bool
IsContainerLayerItem()
{

@@ +251,5 @@
> +  return changed;
> +}
> +
> +struct DIGroup {
> +  // should we just store these things in the hashtable?

Did you find an answer to this question?

@@ +281,5 @@
> +  gfx::Size mScale;
> +  IntPoint mGroupOffset;
> +  Maybe<wr::ImageKey> mKey;
> +
> +  void InvalidateRect(IntRect aRect) {

const IntRect& aRect

@@ +286,5 @@
> +    // Empty rects get dropped
> +    mInvalidRect = mInvalidRect.Union(aRect);
> +  }
> +
> +  IntRect ItemBounds(nsDisplayItem *item)

aItem

@@ +302,5 @@
> +      delete data;
> +    }
> +  }
> +
> +  void ComputeGeometryChange(nsDisplayItem *item, BlobItemData *aData, Matrix &aMatrix, nsDisplayListBuilder *builder) {

aItem and aBuilder

@@ +317,5 @@
> +      GP("shift %d %d\n", shift.x, shift.y);
> +    int32_t appUnitsPerDevPixel = item->Frame()->PresContext()->AppUnitsPerDevPixel();
> +    MOZ_RELEASE_ASSERT(mAppUnitsPerDevPixel == appUnitsPerDevPixel);
> +    // XXX: we need to compute this properly. This basically just matches the
> +    // computation for regular fallback. We should be more disciplined.

Compute what properly? How is it wrong?

@@ +437,5 @@
> +        // We haven't detected any changes so far. Unfortunately we don't
> +        // currently have a good way of checking if the transform has changed so we just
> +        // recompute our rect and see if it has changed.
> +        // If we want this to go faster, we can probably put a flag on the frame
> +        // using the style sytem UpdateTransformLayer hint and check for that.

wrong indent for this comment

@@ +457,5 @@
> +          GP("TransformChange: %s %d %d %d %d\n", item->Name(),
> +                 aData->mRect.x, aData->mRect.y, aData->mRect.XMost(), aData->mRect.YMost());
> +        } else if (LayerItem(item)) {
> +          UniquePtr<nsDisplayItemGeometry> geometry(item->AllocateGeometry(builder));
> +          // we need to catch bounds changes of containers so that continue to have the correct bounds rects in the recording

missing the word "we" in there somewhere, I think

@@ +510,5 @@
> +    for (auto iter = mDisplayItems.Iter(); !iter.Done(); iter.Next()) {
> +      BlobItemData* data = iter.Get()->GetKey();
> +      // XXX: If we had two hash tables we could actually move items from one into the other
> +      // and then only iterate over the old items. However, this is probably more expensive
> +      // because doing an insert is more expensive than doing a lookup because of resizing?

update the comment

@@ +568,5 @@
> +
> +    PaintItemRange(aGrouper, aStartItem, aEndItem, context, recorder);
> +
> +    if (!mKey) {
> +#if 0

We could respect the general paint flashing pref here

@@ +574,5 @@
> +      dt->FillRect(gfx::Rect(0, 0, size.width, size.height), gfx::ColorPattern(gfx::Color(0., 1., 0., 0.5)));
> +      dt->FlushItem(IntRect(IntPoint(0, 0), size));
> +#endif
> +    }
> +    bool isOpaque = false; // XXX: set this correctly

How is it wrong?

@@ +783,5 @@
> +  virtual ~WebRenderGroupData();
> +
> +  virtual WebRenderGroupData* AsGroupData() override { return this; }
> +  virtual UserDataType GetType() override { return UserDataType::eGroupSplit; }
> +  static UserDataType Type() { return UserDataType::eGroupSplit; }

This enum value should probably be renamed as well.

@@ +813,5 @@
> +    Matrix4x4Flagged t = transformItem->GetTransform();
> +    Matrix t2d;
> +    bool is2D = t.Is2D(&t2d);
> +    GP("active: %d\n", transformItem->MayBeAnimated(aDisplayListBuilder));
> +    return transformItem->MayBeAnimated(aDisplayListBuilder) || !is2D || HasActiveChildren(*transformItem->GetChildren(), aDisplayListBuilder);

may want to break this line somewhere

@@ +815,5 @@
> +    bool is2D = t.Is2D(&t2d);
> +    GP("active: %d\n", transformItem->MayBeAnimated(aDisplayListBuilder));
> +    return transformItem->MayBeAnimated(aDisplayListBuilder) || !is2D || HasActiveChildren(*transformItem->GetChildren(), aDisplayListBuilder);
> +  }
> +  // TODO: handle opacity etc.

This needs to handle things like active transforms inside inactive opacity

@@ +822,5 @@
> +
> +// If we have an item we need to make sure it matches the current group
> +// otherwise it means the item switched groups and we need to invalidate
> +// it and recreate the data.
> +BlobItemData *GetBlobItemDataForGroup(nsDisplayItem* aItem, DIGroup* aGroup)

static BlobItemData*
GetBlobItemDataForGroup(nsDisplayItem* aItem, DIGroup* aGroup)

@@ +1805,5 @@
> +}
> +
> +WebRenderGroupData::~WebRenderGroupData()
> +{
> +  GP("Group data destruct\n");

Why not make this NS_LOG_CTOR/DTOR?

::: gfx/layers/wr/WebRenderUserData.h
@@ +52,5 @@
>    virtual WebRenderImageData* AsImageData() { return nullptr; }
>    virtual WebRenderFallbackData* AsFallbackData() { return nullptr; }
>    virtual WebRenderCanvasData* AsCanvasData() { return nullptr; }
> +  virtual WebRenderGroupData* AsGroupData() { return nullptr; }
> +  virtual WebRenderGroupSplitData* AsGroupSplitData() { return nullptr; }

WebRenderGroupSplitData no longer exists

@@ +213,5 @@
>  
>    UniquePtr<WebRenderCanvasRendererAsync> mCanvasRenderer;
>  };
>  
> +

unnecessary

::: layout/painting/RetainedDisplayListBuilder.cpp
@@ +47,5 @@
>        // If we have existing cached geometry for this item, then check that for
>        // whether we need to invalidate for a sync decode. If we don't, then
>        // use the item's flags.
>        DisplayItemData* data = FrameLayerBuilder::GetOldDataFor(i);
> +      //XXX: handle webrender case

Is this an existing bug?

::: layout/painting/nsDisplayList.cpp
@@ +6885,5 @@
>    return container.forget();
>  }
>  
> +mozilla::gfx::CompositionOp
> +nsDisplayBlendMode::BlendMode() {

brace goes on the next line

@@ +10143,5 @@
> +                                             aSc,
> +                                             aManager,
> +                                             aDisplayListBuilder);
> +  } else {
> +    return nullptr;

return false

::: layout/painting/nsDisplayList.h
@@ +6479,5 @@
>      return mFrame->Extend3DContext() ||
>        mFrame->Combines3DTransformWithAncestors();
>    }
>  
> +  StoreList mStoredList;

Does this need to be public?

Comment 51

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2adda34a5051
Add support for updating blob images. r=mstange

Comment 53

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

I think I've addressed all of the review comments. Can you take a closer look now?

Comment 54

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

With the logging and some debugging code stripped out.

Comment 55

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2adda34a5051

Comment 56

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 57

[Markus Stange [:mstange]]

Comment on attachment 8961012 [details] [diff] [review]
Add blob invalidation

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

::: gfx/2d/2D.h
@@ +1517,5 @@
>  class DrawEventRecorder : public RefCounted<DrawEventRecorder>
>  {
>  public:
>    MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DrawEventRecorder)
> +  virtual bool Finish() = 0;

Please document what the return value means.

::: gfx/2d/DrawEventRecorder.cpp
@@ +105,1 @@
>    WriteElement(mIndex, mOutputStream.mLength);

I think there should be a comment here that references your other documentation about the index format ([end, extra_end, bounds]).

@@ +115,2 @@
>    ClearResources();
> +  WriteHeader(mOutputStream);

Please add a comment about what this does.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +60,5 @@
> +                                    nsTArray<BlobItemData*>,
> +                                    RemoveFrameFromBlobGroup);
> +
> +
> +struct BlobItemData

Please add a comment about the ownership + lifecycle for this class. Who owns it / where is it stored?

@@ +75,5 @@
> +  // It would be nice to not need this. We need to be able to call ComputeInvalidationRegion.
> +  // ComputeInvalidationRegion will sometimes reach into parent style structs to get information
> +  // that can change the invalidation region
> +  UniquePtr<nsDisplayItemGeometry> mGeometry;
> +  DisplayItemClip mClip; // this can change

Is there any field of this class that cannot change? Not sure this comment is useful.

@@ +102,5 @@
> +    AddFrame(aItem->Frame());
> +  }
> +
> +  void
> +  AddFrame(nsIFrame* aFrame)

This method only has one caller, the constructor. Let's make it private or inline it into the constructor.

@@ +157,5 @@
> +}
> +
> +// We keep around the BlobItemData so that when we invalidate it get properly included in the rect
> +static void
> +RemoveFrameFromBlobGroup(nsTArray<BlobItemData*>* aArray)

This is a bit of a hostile function, isn't it? You give it a pointer and it deletes it.

@@ +180,5 @@
> +  std::vector<nsDisplayItem*> mItemStack;
> +  nsDisplayListBuilder* mDisplayListBuilder;
> +  ScrollingLayersHelper& mScrollingHelper;
> +  Matrix mTransform;
> +  int mGroupCount;

size_t

@@ +190,5 @@
> +                       wr::DisplayListBuilder& aBuilder,
> +                       wr::IpcResourceUpdateQueue& aResources,
> +                       DIGroup* aGroup, nsDisplayList* aList,
> +                       const StackingContextHelper& aSc);
> +  void ConstructGroupsInsideInactive(WebRenderCommandBuilder* aCommandBuilder,

Please add comments for these three methods.

@@ +219,5 @@
> +    case DisplayItemType::TYPE_FILTER: {
> +      return true;
> +    }
> +    case DisplayItemType::TYPE_MASK: {
> +      return true;

These case branches can be grouped more concisely.

@@ +229,5 @@
> +}
> +
> +#include <sstream>
> +
> +bool LayerPropertyChanged(nsDisplayItem* aItem, BlobItemData* aData) {

Maybe UpdateContainerLayerPropertiesAndDetectChange would be a better name?

@@ +262,5 @@
> +}
> +
> +struct DIGroup {
> +  // XXX: Storing owning pointers to the BlobItemData in a hash table is not
> +  // a good choice. There are two better options:

nsPtrHashKey doesn't look like an owning pointer to me.

@@ +307,5 @@
> +      delete data;
> +    }
> +  }
> +
> +  void ComputeGeometryChange(nsDisplayItem *aItem, BlobItemData *aData, Matrix &aMatrix, nsDisplayListBuilder *aBuilder) {

Let's move this whole method out-of-line.

@@ +346,5 @@
> +      auto transBounds = nsLayoutUtils::MatrixTransformRect(bounds,
> +                                                            Matrix4x4::From2D(aMatrix),
> +                                                            float(appUnitsPerDevPixel));
> +
> +      IntRect transformedRect = RoundedOut(aMatrix.TransformBounds(ToRect(nsLayoutUtils::RectToGfxRect(combined.GetBounds(), appUnitsPerDevPixel)))) - mGroupOffset;

This line is way too long and repeated a bunch of times in this method. Can you put it in one place and break it appropriately?

Comment 58

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

The one with logging stripped

Comment 59

[Markus Stange [:mstange]]

Comment on attachment 8961073 [details] [diff] [review]
Add blob invalidation

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

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +75,5 @@
> +  bool mEmpty;
> +
> +  // properties that are used to emulate layer tree invalidation
> +  Matrix mMatrix;
> +  Matrix4x4Flagged mTransform;

Please document the difference between mMatrix and mTransform.

@@ +385,5 @@
> +
> +        } else if (!aMatrix.ExactlyEquals(aData->mMatrix)) {
> +          // We haven't detected any changes so far. Unfortunately we don't
> +          // currently have a good way of checking if the transform has changed so we just
> +          // recompute our rect and see if it has changed.

I think this comment needs to be updated. You no longer check a rectangle, you do actually check the entire matrix here.

@@ +395,5 @@
> +            // the bounds of layer items can change on us
> +            // other items shouldn't
> +            MOZ_RELEASE_ASSERT(geometry->mBounds.IsEqualEdges(aData->mGeometry->mBounds));
> +          } else {
> +            aData->mGeometry = Move(geometry);

Why are we only persisting the bounds if we have a non-ContainerLayer item? Is it because nsDisplayItemGeometry never has useful things to say about container item changes anyway?

@@ +449,5 @@
> +    IntSize size = mGroupBounds.Size().ScaleToNearestPixels(mScale.width, mScale.height, aGrouper->mAppUnitsPerDevPixel);
> +
> +    if (mInvalidRect.IsEmpty()) {
> +      if (mKey)
> +        PushImage(aBuilder, bounds);

missing braces

Comment 60

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Less stuff

Comment 61

[Markus Stange [:mstange]]

Comment on attachment 8961105 [details] [diff] [review]
Add blob invalidation

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

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +471,5 @@
> +
> +    RefPtr<gfx::DrawTarget> dt = gfx::Factory::CreateRecordingDrawTarget(recorder, dummyDt, size);
> +    // Setup the gfxContext
> +    RefPtr<gfxContext> context = gfxContext::CreateOrNull(dt);
> +    context->SetMatrix(Matrix::Scaling(mScale.width, mScale.height).PreMultiply(Matrix::Translation(-bounds.x, -bounds.y)));

could use PreTranslate here

@@ +604,5 @@
> +      gfx::Matrix matrix;
> +      int commonClipCount = 0; // Don't share any clips, always apply all clips.
> +      if (currentClip.HasClip()) {
> +        aContext->Save();
> +        currentClip.ApplyTo(aContext, this->mAppUnitsPerDevPixel, commonClipCount);

We can call ApplyTo without the commonClipCount argument, 0 is its default value.

@@ +610,5 @@
> +        matrix = aContext->CurrentMatrix();
> +      }
> +
> +      auto transformItem = static_cast<nsDisplayTransform*>(aItem);
> +      auto trans = transformItem->GetTransform();

I would prefer a type here instead of auto.

@@ +611,5 @@
> +      }
> +
> +      auto transformItem = static_cast<nsDisplayTransform*>(aItem);
> +      auto trans = transformItem->GetTransform();
> +      Matrix m;

rename to trans2d

@@ +710,5 @@
> +  }
> +  return false;
> +}
> +
> +// We can't easily use GetLayerState because it wants a bunch of layers related information.

Add a comment:
This function decides whether we want to treat this item as "active", which means
that it's a container item which we will turn into a WebRender StackingContext, or
whether we treat it as "inactive" and include it inside the parent blob image.

@@ +735,5 @@
> +// it and recreate the data.
> +static BlobItemData*
> +GetBlobItemDataForGroup(nsDisplayItem* aItem, DIGroup* aGroup)
> +{
> +    BlobItemData* data = GetBlobItemData(aItem);

wrong indent

@@ +772,5 @@
> +    if (IsItemProbablyActive(item, mDisplayListBuilder)) {
> +      mGroupCount++;
> +      currentGroup->EndGroup(aCommandBuilder->mManager, aBuilder, aResources, this, startOfCurrentGroup, item);
> +      // Note: this call to CreateWebRenderCommands can recurse back into
> +      // this function.

move comment down by 2 lines

@@ +826,5 @@
> +        // Save the current transform.
> +        Matrix m = mTransform;
> +
> +        mTransform.PreMultiply(t2d);
> +        ConstructGroupsInsideInactive(aCommandBuilder, aBuilder, aResources, currentGroup, transformItem->GetChildren(), aSc);

can just use "children" instead of "transformItem->GetChildren()" here, I think

@@ +845,5 @@
> +  currentGroup->EndGroup(aCommandBuilder->mManager, aBuilder, aResources, this, startOfCurrentGroup, nullptr);
> +}
> +
> +// This does a pass over the display lists and will join the display items
> +// into groups as well as paint them

Where does the painting happen? The only thing this function does, other than recursing, is to call GetBlobItemDataForGroup and ComputeGeometryChange.
It looks like painting only happens in EndGroup.

@@ +869,5 @@
> +
> +      Matrix m = mTransform;
> +
> +      mTransform.PreMultiply(t2d);
> +      ConstructGroupsInsideInactive(aCommandBuilder, aBuilder, aResources, currentGroup, transformItem->GetChildren(), aSc);

can just use "children" instead of "transformItem->GetChildren()" here, I think

Comment 62

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #57)
> > +  void ComputeGeometryChange(nsDisplayItem *aItem, BlobItemData *aData, Matrix &aMatrix, nsDisplayListBuilder *aBuilder) {
> 
> Let's move this whole method out-of-line.

Why? None of the other methods are out-of-line.

Comment 63

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add blob invalidation

Comment 64

[Markus Stange [:mstange]]

Comment on attachment 8961183 [details] [diff] [review]
Add blob invalidation

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

Let's land it! Would be good to do a clang-format pass on it at some point.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b73ff00586c
Add blob invalidation. r=mstange

Comment 66

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8b73ff00586c