783368 - Add a low res tile cache to TiledThebesLayer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

With progressive tiled based painting we can extend this to work in two resolution. One is the current resolution an one is a fix resolution that mimics the screenshot. This let's us do screenshot-ing while giving us many benefits of the layer system including proper invalidation.

Note that this feature will likely cause regressions until bug 766991 and general over painting issues remain.

Once we get this we can also work it into our panning heuritics. i.e. We can decide to only paint at low res for high scroll velocity or even abort painting quickly if the screen is out of the painting view.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: early wip

With this patch thebes layer now have two buffers but both still paint at the same resolution. Still working out how best to structure the scalling of the low res buffer.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add InvalidateLayer

The problem I'm running into is that we invalidate the layer using InvalidateRect(layer->GetValidRegion). This was failing with the low res cache because we never marked that region as valid thus we wouldn't get further invalidate events from it.

This patch adds a InvalidateLayer to replace this idiom and works well in practice.

I'm not sure however that I do like this patch because if we go forward with this approach then we need to make sure we invalidate regions in the visible region of the layer and not just from the valid region because they may also contain some low res cache. Matt can we maintain this stronger constraint?

The other alternative is to mark a low res region as 'valid' but this is nasty for the layer system that can assume this region to be fully validated. We would then have to break this into ValidRegion (lowResValidRegion Union hiResValidRegion), lowResValidRegion and hiResValidRegion each with their own getters that we'd have to pick carefully.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add a low res buffer

With this patch we support low res painting. Heuristics are basic: low res first, then high res. This patch wont turn it on yet. Before turning it on I want the cache to be at a fixed res instead of just 1/4 of the current resolution and have some heuristic tuning. In the meantime I'm still ready to land this off.

See https://dl.dropbox.com/u/10523664/output.webm

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 652771 [details] [diff] [review]
Part 1: Add InvalidateLayer

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

Doesn't GetValidRegion() need to return the actual valid region (regardless of what resolution it's drawn at), so that ThebesLayerBuffer can decide what areas need to be redrawn?

I think the resolution of each tile should be an implementation detail that isn't exposed to ThebesLayerBuffer etc. The backend can then decide when to invalidate them and change the resolution.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 652771 [details] [diff] [review]
> Part 1: Add InvalidateLayer
> 
> Review of attachment 652771 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doesn't GetValidRegion() need to return the actual valid region (regardless
> of what resolution it's drawn at), so that ThebesLayerBuffer can decide what
> areas need to be redrawn?

I can certainly write that patch but keep in mind that:
- if we take the first approach of mValidRegion meaning that we're fully done drawing that region then we can't assume that because something is not in the valid region it doesn't need to be invalidated. (We currently make that assumption)
- If we take your approach of mValidRegion meaning that we have some valid content at some resolution then we can't assume that we wont try to paint a valid region because we don't have it at all desired resolutions. (I think we also currently make that assumption)

> 
> I think the resolution of each tile should be an implementation detail that
> isn't exposed to ThebesLayerBuffer etc. The backend can then decide when to
> invalidate them and change the resolution.

I agree, we just have to make sure that we're not using valid region to imply something (see 1st or 2nd approach) that isn't true with these changes.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Girard (:BenWa) from comment #5)
> - if we take the first approach of mValidRegion meaning that we're fully
> done drawing that region then we can't assume that because something is not
> in the valid region it doesn't need to be invalidated. (We currently make
> that assumption)

Where?

> - If we take your approach of mValidRegion meaning that we have some valid
> content at some resolution then we can't assume that we wont try to paint a
> valid region because we don't have it at all desired resolutions. (I think
> we also currently make that assumption)

I don't think we do.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Benoit Girard (:BenWa) from comment #5)
> > - if we take the first approach of mValidRegion meaning that we're fully
> > done drawing that region then we can't assume that because something is not
> > in the valid region it doesn't need to be invalidated. (We currently make
> > that assumption)
> 
> Where?
> 

https://bugzilla.mozilla.org/attachment.cgi?id=652771&action=diff#a/layout/base/FrameLayerBuilder.cpp_sec1

Notice how we only invalidate region from the GetValidRegion()

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I got confused.

The valid region means that the layer has valid content for that region. Matt's comment #4 is exactly right. It's totally OK to ask FrameLayerBuilder to paint an area that's in the valid region.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add a low res buffer

Alright so the valid region for the Thebes layer now reflects that we have some data for this layer at any resolution 'mValidRegion.Or(mTiledBuffer.GetValidRegion(), mLowResTiledBuffer.GetValidRegion());'.

I think we should still land part 1 but it's no longer required.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 653949 [details] [diff] [review]
Part 2: Add a low res buffer

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

What's the reasoning for having two BasicTiledLayerBuffers, one at each resolution? It seems like we'd end with two copies of every tile once they've all been drawn unless we manually dump the low res tile buffer at some point.

I also think that adding logic to retain some tiles in the low res buffer permanently and leave them out of the high res one will be really complicated.

Wouldn't it be easier to move the resolution code down to the level of an individual tile? Each BasicTiledLayerTile can store what resolution it was rendered at, and then we can easily apply heuristics (say, based on the tiles position relative to the visible screen) and redraw tiles as needed.

It would also remove the need to make any modifications to the OGL code.

Comment 11

[Matt Woodrow (:mattwoodrow)]

As discussed with BenWa on irc, we redivide the viewport into tiles every time we change the zoom, which forces eviction of all existing tiles.

We really don't want to have to redraw our low res tiles on zoom, so having them in a separate, resolution-fixed tile cache is probably the only solution.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 653949 [details] [diff] [review]
Part 2: Add a low res buffer

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

Few questions, looks good overall.

I'm still fairly worried about how complicated the interactions between the two buffers will be when you try add more complicated heuristics.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +297,5 @@
> +    //      the screen is standing still. As well we can abort
> +    //      a transaction early instead of repeating it to finish
> +    //      the remaining tiles.
> +    // Paint low res first
> +    mLowResTiledBuffer.SetScale(0.25);

Make this a #define or const instead.

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +134,4 @@
>  {
> +  // TODO use the resolution parameter instead?
> +  if (aTiledBuffer->IsLowResBuffer()) {
> +    printf_stderr("Painted lowres\n");

Debugging code left in.

@@ +141,5 @@
> +    delete aTiledBuffer;
> +    mLowResRegionToUpload.Or(mLowResRegionToUpload, mLowResMainMemoryTiledBuffer.GetLastPaintRegion());
> +    mValidRegion.SetEmpty();
> +  } else {
> +    printf_stderr("Painted hires\n");

Same again.

@@ +146,5 @@
> +    mMainMemoryTiledBuffer.ReadUnlock();
> +    mMainMemoryTiledBuffer = *aTiledBuffer;
> +    // TODO: Remove me once Bug 747811 lands.
> +    delete aTiledBuffer;
> +    mRegionToUpload.Or(mRegionToUpload, mMainMemoryTiledBuffer.GetLastPaintRegion());

Do we want to clear mLowResValidRegion here? If not, can you add a comment to here, or above as to why it's different.

@@ +165,5 @@
> +    mLowResMainMemoryTiledBuffer.ReadUnlock();
> +    mLowResMainMemoryTiledBuffer = BasicTiledLayerBuffer();
> +    mLowResRegionToUpload = nsIntRegion();
> +    printf_stderr("Uploaded lowres\n");
> +    return;

What if we have high res areas to upload too?

We should either allow both, or assert that we don't need to I think.

@@ +310,5 @@
>  
>    Layer* maskLayer = GetMaskLayer();
>  
>    // Render old tiles to fill in gaps we haven't had the time to render yet.
> +  if (mReusableTileStore && false) {

If think this shouldn't change while low res painting is turned off.

@@ +323,5 @@
>    const nsIntRect visibleRect = visibleRegion.GetBounds();
>  
> +  printf_stderr("render lowres\n");
> +  RenderTileLayer(visibleRect, mLowResVideoMemoryTiledBuffer, mLowResValidRegion, maskLayer, aOffset);
> +  RenderTileLayer(visibleRect, mVideoMemoryTiledBuffer, mValidRegion, maskLayer, aOffset);

I assume you're going to add a way to make sure we don't overdraw here. Guess this is fine for now since it's pref'd off.

Comment 13

[Chris Lord [:cwiiis]]

I've been having a look at this and the patches here - I think it'd be better to treat the low-res tile cache as more of a low-accuracy tile cache and derive it from the standard resolution tile cache.

Adding a second drawing resolution parameter that flies at odds with the existing one seems like something that'd be a bit delicate, and I'd have thought scaling an existing rendering rather than re-rendering would be faster anyway.

Down-side of course is that you'd no longer be able to just do a low-res render and no high-res if you wanted that, but I wonder if it'd just be a better idea to add interruptable drawing and have a low-accuracy cache that's longer-lived than the main cache, filled in as the main layer is validated? This would nullify the ReusableTileStore, but wouldn't entirely supplant the screenshot layer, but I'd have thought with interruptable drawing, we ought to be good enough that we wouldn't need it anyway.

A long-term solution that I think would be nice would be to remove drawing resolution from layout and make it purely a function of the rendering in gfx. We already have to set the clamping scroll-port when zooming separately from the resolution, so I'm not sure what we're getting by doing it the current way. It'd simplify the code and invalidation in layout, certainly, and this patch proves that it'd not be too involved to add the same mechanism in gfx.

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 652771 [details] [diff] [review]
Part 1: Add InvalidateLayer

I'm going to finish up this work after progressive drawing is ironed out.

Comment 15

[Chris Lord [:cwiiis]]

Stealing this (we've discussed already) - patches incoming, mostly for feedback rather than review. I'm attaching them before adding flags so I can go over them first, apologies in advance for the noise.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add 'critical' display-port to nsIDOMWindowUtils

This adds a new element property to represent a sub-section of the display-port that should be considered 'critical'.

Conceptually, this is so we can specify a larger display port and designate an area of it to be rendered at the full-fat resolution/coherency. The rest of the display-port can be rendered at leisure, at whatever resolution is convenient/fast, and with less focus on maintaining coherency.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Use the critical display port to increase the display-port size

This sets the critical display-port to the old display-port size and sets the display-port to the page size.

This is mainly just to test the code works (seemingly, it does).

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Add resolution to TiledLayerBuffer

This adds the concept of resolution to the TiledLayerBuffer, and the code to draw it correctly when it's set. This is partially derived from BenWa's attached patch.

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Disable reusable tile store

I've not made these patches play nicely with the reusable tile store yet - this adds a pref for it and turns it off.

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Add low precision buffer

This adds a low precision buffer to BasicTiledThebesLayer / TiledThebesLayerOGL that renders the entire display-port contents at a lower resolution.

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Add display port generations

This adds a 'generation' property for display ports to help identify them after they've been set.

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Use display port generations for cancelling

This passes the display port generation to the progressive update callback and uses it for cancelling instead of checking the display-port values.

This lets us send the real display port values for low-resolution tile drawing, meaning it won't get cancelled prematurely when the high-resolution display-port no longer contains the viewport.

Comment 23

[Chris Lord [:cwiiis]]

Things still to do/consider:

- The low precision buffer shouldn't bother validating the valid region of the high-precision buffer - unless we're scrolling, at least.
- Perhaps use the reusable tile store to draw the low-precision tiles so that they don't disappear on resolution changes(?) - it may just be nicer to add per-tile resolution support to the tiled buffer though.
- General tidying/polishing.

I'm not sure the generation patches are 100% either, I did see an area in low res that wasn't updated to high, but I don't know if that was due to these patches or an existing bug (say bug 807299).

Comment 24

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add 'critical' display-port to nsIDOMWindowUtils

Comment 25

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Prefer the critical display port in tiled layers

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Add resolution to TiledLayerBuffer

Comment 27

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Disable reusable tile store

Still debatable whether this should replace reusable tile store or work in addition to it... In my testing, this is a lot more effective and has fewer hard problems though. Maybe disable for now and think about re-enabling in a later bug.

Comment 28

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Add low precision buffer

Comment 29

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Add display port generations

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Use display port generations for cancelling

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Part 8 - Send the correct display port when doing low precision updates

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 681721 [details] [diff] [review]
Part 7 - Use display port generations for cancelling

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

::: mobile/android/base/gfx/DisplayPortMetrics.java
@@ +31,5 @@
>  
>      public DisplayPortMetrics(float left, float top, float right, float bottom, float resolution) {
>          this.resolution = resolution;
>          mPosition = new RectF(left, top, right, bottom);
> +        generation = currentGeneration;

I'm not entirely happy with how currentGeneration is exposed. The usual idiom for generation code is to keep it private static, and have the constructor here do something like "generation = currentGeneration++". I would like to do something like that here but it looks like we create more DisplayPortMetrics objects than times we increment the generation, so it's not really doable easily. One possibility might be to add a boolean to the DPM constructor indicating if the generation should be incremented or not and then propagate that backwards to the points where the decision is made. However that doesn't seem much better as an overall solution, so I guess this is fine. If you'd like to switch to doing that that's fine too.

::: mobile/android/chrome/content/browser.js
@@ +2768,5 @@
>        cwu.setCriticalDisplayPortForElement(displayPort.x, displayPort.y,
>                                             displayPort.width, displayPort.height,
>                                             element);
> +    } else {
> +      cwu.setDisplayPortGenerationForElement(aDisplayPort.generation, element);

I feel like this hunk belongs in a different patch, maybe. Not really sure what this is doing without properly reading all the previous patches.

::: widget/android/nsIAndroidBridge.idl
@@ +34,2 @@
>  
>  [scriptable, uuid(e1bfbc07-dbae-409d-a5b5-ef57522c1f15)]

Need to modify this uuid.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 681720 [details] [diff] [review]
Part 6 - Add display port generations

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +358,5 @@
>    }
>  
>    content->SetProperty(nsGkAtoms::DisplayPort, new nsRect(displayport),
>                         DestroyNsRect);
> +  content->SetProperty(nsGkAtoms::DisplayPortGeneration, (void*)(intptr_t)aGeneration);

I suggest using a single property which is a heap-allocated struct containing an nsRect and the generation.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +153,5 @@
> +                                in nsIDOMElement aElement,
> +                                [optional] in int32_t aGeneration);
> +
> +  /**
> +   * Set the display port generation.

Why do we need to set the display port generation separately from setting the display port?

Comment 34

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 681720 [details] [diff] [review]
> Part 6 - Add display port generations
> 
> Review of attachment 681720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDOMWindowUtils.cpp
> @@ +358,5 @@
> >    }
> >  
> >    content->SetProperty(nsGkAtoms::DisplayPort, new nsRect(displayport),
> >                         DestroyNsRect);
> > +  content->SetProperty(nsGkAtoms::DisplayPortGeneration, (void*)(intptr_t)aGeneration);
> 
> I suggest using a single property which is a heap-allocated struct
> containing an nsRect and the generation.
> 
> ::: dom/interfaces/base/nsIDOMWindowUtils.idl
> @@ +153,5 @@
> > +                                in nsIDOMElement aElement,
> > +                                [optional] in int32_t aGeneration);
> > +
> > +  /**
> > +   * Set the display port generation.
> 
> Why do we need to set the display port generation separately from setting
> the display port?

It's possible for the UI to send a display-port that we consider identical to the one already set - in this case, we don't update the display-port (as this would cause an unnecessary draw), but we do update the generation (so that any subsequent draw isn't incorrectly skipped).

With this in mind, do you think it's still better to store this in a single property?

Comment 35

[Chris Lord [:cwiiis]]

Attachment: Part 6 - Don't validate high-precision valid region in low-precision buffer

Don't validate high-precision valid region in the low-precision buffer. This should stop us from drawing twice most of the time.

Comment 36

[Chris Lord [:cwiiis]]

The generations code doesn't work quite right unfortunately, I'm working on trying to get it correct...

Comment 37

[Chris Lord [:cwiiis]]

Ok, don't think generations can work easily, just going to give up on that for now and make things work without.

Comment 38

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #37)
> Ok, don't think generations can work easily, just going to give up on that
> for now and make things work without.

Got what I think is a reasonable and simpler replacement, just waiting on building to test...

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Part 7 - Send the correct display port when doing low precision updates

Don't bother with generations, just store whether we think the display-port is current when we do the high precision update and use that stored boolean when doing low precision updates.

Comment 40

[Benoit Girard (:BenWa)]

Comment on attachment 681713 [details] [diff] [review]
Part 2 - Prefer the critical display port in tiled layers

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

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +437,5 @@
>      oldValidRegion.And(oldValidRegion, mVisibleRegion);
>      mTiledBuffer.ClearPaintedRegion();
>  
> +    if (!layerDisplayPort.IsEmpty()) {
> +      oldValidRegion.And(oldValidRegion, layerDisplayPort);

nit: move this to line 438 to cluster the changes to oldValidRegion

Comment 41

[Benoit Girard (:BenWa)]

Comment on attachment 681716 [details] [diff] [review]
Part 3 - Add resolution to TiledLayerBuffer

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

::: gfx/layers/TiledLayerBuffer.h
@@ +111,5 @@
>    bool RemoveTile(int x, int y, Tile& aRemovedTile);
>  
>    uint16_t GetTileLength() const { return TILEDLAYERBUFFER_TILE_SIZE; }
> +  uint32_t GetScaledTileLengthX() const { return TILEDLAYERBUFFER_TILE_SIZE / mResolution.width; }
> +  uint32_t GetScaledTileLengthY() const { return TILEDLAYERBUFFER_TILE_SIZE / mResolution.height; }

I decided to not support rectangular tiles, do we really want to support an uneven resolution? Not supporting that will lower the complexity for something we're likely to not use.

@@ +140,5 @@
> +  // contents of the buffer are drawn. mResolution has no effect on the
> +  // coordinate space of the valid region, but does affect the size of an
> +  // individual tile's rect in relation to the valid region.
> +  const gfxSize& GetResolution() const { return mResolution; }
> +  void SetResolution(const gfxSize& aResolution) { mResolution = aResolution; }

I feel like this method should invalidate the buffer but maybe this will become clearer with the next patch.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 681719 [details] [diff] [review]
Part 5 - Add low precision buffer

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

::: mobile/android/chrome/content/browser.js
@@ +2755,5 @@
> +      // critical display-port. Spare area is *not* redistributed to the other
> +      // axis, as display-list building and invalidation cost scales with the
> +      // size of the display-port.
> +      let pageXMost = -geckoScrollX + pageWidth;
> +      let pageYMost = -geckoScrollY + pageHeight;

I believe this chunk of code won't work properly with RTL pages. It's possible for geckoScrollX to be negative and in that case pageXMost will be larger than the pageWidth which will screw up the calculations below.

@@ +2762,5 @@
> +      // resolution to stop resolution oscillations due to rounding errors.
> +      let dpW = Math.min(pageWidth, displayPort.width * 4);
> +      let dpH = Math.min(pageHeight, displayPort.height * 4);
> +      let dpX = Math.min(Math.max(displayPort.x - displayPort.width * 1.5, -geckoScrollX), pageXMost - dpW);
> +      let dpY = Math.min(Math.max(displayPort.y - displayPort.height * 1.5, -geckoScrollY), pageYMost - dpH);

Same here, I think using -geckoScrollX will also cause badness in these calculations on RTL pages.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 682170 [details] [diff] [review]
Part 7 - Send the correct display port when doing low precision updates

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

This looks fine to me, but I think BenWa should also review to verify the logic in GeckoLayerClient is correct, since I've only skimmed the previous patches and haven't been following along with this bug that much.

Comment 44

[Benoit Girard (:BenWa)]

Comment on attachment 681719 [details] [diff] [review]
Part 5 - Add low precision buffer

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

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +192,5 @@
>      drawRect.Scale(mResolution.width, mResolution.height);
>      ctxt->Rectangle(drawRect, true);
>      ctxt->Fill();
>    } else {
> +    printf_stderr("Taking non-single-buffer route\n");

printf

@@ +324,5 @@
>    nsIntRect paintBounds = aRegionToPaint.GetBounds();
>  
>    int startX, incX, startY, incY;
>    if (aScrollOffset.x >= mLastScrollOffset.x) {
> +    startX = aTiledBuffer.RoundDownToTileEdgeX(paintBounds.x);

If we don't support uneven resolution let's go back to using the X/Y

@@ +568,5 @@
> +  if (!criticalDisplayPort.IsEmpty() &&
> +      !nsIntRegion(layerDisplayPort).Contains(mVisibleRegion)) {
> +    nsIntRegion oldValidRegion = mLowPrecisionTiledBuffer.GetValidRegion();
> +    oldValidRegion.And(oldValidRegion, mVisibleRegion);
> +    mLowPrecisionTiledBuffer.ClearPaintedRegion();

We should clear this right after sending it off to the compositor. I think it's less error prone for when that code is changed around.

@@ +571,5 @@
> +    oldValidRegion.And(oldValidRegion, mVisibleRegion);
> +    mLowPrecisionTiledBuffer.ClearPaintedRegion();
> +
> +    // If the frame resolution has changed, invalidate the buffer
> +    clearedLowPrecision = false;

this line isn't needed, you initialize it above.

@@ +581,5 @@
> +      mLowPrecisionValidRegion.SetEmpty();
> +      mLowPrecisionTiledBuffer.SetFrameResolution(resolution);
> +    }
> +
> +    if (!BasicManager()->IsRepeatTransaction()) {

Add a comment along the lines of:
// Drop the valid region that fell off the visibleRegion this frame

Actually I don't think this is correct. We use !Repeat to do work only once on the first part however if we painted at a high resolution before this the low res painting will never call this.

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +161,5 @@
> +
> +void
> +TiledThebesLayerOGL::ProcessLowPrecisionUploadQueue()
> +{
> +  if (!mLowPrecisionMainMemoryTiledBuffer.IsLocked() &&

I don't understand this condition. This needs a comment.

@@ +167,5 @@
> +    return;
> +
> +  mLowPrecisionRegionToUpload.And(mLowPrecisionRegionToUpload,
> +                                  mLowPrecisionMainMemoryTiledBuffer.GetValidRegion());
> +  // XXX Frame resolution is unused here

This warning isn't clear. Can you elaborate?

Comment 45

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Add resolution to TiledLayerBuffer

This changes resolution to be a single dimension and clears the valid region when setting resolution, addressing review comments. Carrying r=bgirard. This also adjusts the code in the non-single-paint-buffer render path so that when it's fixed, low-precision tiles will continue to render correctly.

Comment 46

[Chris Lord [:cwiiis]]

Attachment: Part 5 - Add low precision buffer

This addresses both BenWa and kats' review comments - Hopefully RTL pages should work ok (though I still need to test this), and all of BenWa's comments have been addressed.

Using IsRepeat() is ok as it's only true on the second iteration (this differs from GetRepeat, which will change immediately after SetRepeat).

What isn't ok is the setting of the last scroll offset on !GetRepeat, as this will cause layers to not update the last scroll offset when a previous layer has called SetRepeat.

I'll address that in a follow-up patch if that's ok. The consequence of this is that the tiles may draw in the wrong order under some (reasonably uncommon) circumstances.

Comment 47

[Chris Lord [:cwiiis]]

Attachment: Part 8 - Fix ProgressiveUpdateData

Silly mistake, will cause bad cancelling.

Comment 48

[Chris Lord [:cwiiis]]

Attachment: Part 9 - Fix coherency when doing progressive updates (again)

This changes tile draw ordering/rules so that tiles that were on screen at the time of paint command issuing and remain on screen are drawn in a single transaction.

It also removes the cancelling case where an updated displayport was set and no fresh content is going to be drawn - this was rarely hit, except when it shouldn't have been.

These together stop visible glitching when elements switch layers (and various other similar situations with growing/shrinking, etc.)

Comment 49

[Chris Lord [:cwiiis]]

Attachment: Part 10 - Uncouple low-res updates from high-res

Decouple drawing of tiles on the low precision buffer with those on the unscaled buffer. This allows low-res updates to continue after high-res updates have finished (which allows it to be filled more quickly, preventing the user from seeing blank areas).

Comment 50

[Chris Lord [:cwiiis]]

Hopefully this is the last major revision to these patches - I've pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=7e4007bef4bc

I expect there may be issues, but hopefully easily solved.

Comment 51

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #46)
> Created attachment 683859 [details] [diff] [review]
> Part 5 - Add low precision buffer
> Using IsRepeat() is ok as it's only true on the second iteration (this
> differs from GetRepeat, which will change immediately after SetRepeat).
> 

I'm not sure I follow the logic. The first time IsRepeat is false which we may paint hi res tiles and repeat the transaction which then in a repeated transaction we may start painting the low res. Inside the low res code you have code that does work on !IsRepeat() which wont be true since we've painted hi res tiles.

Comment 52

[Benoit Girard (:BenWa)]

Comment on attachment 683862 [details] [diff] [review]
Part 9 - Fix coherency when doing progressive updates (again)

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

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +281,5 @@
>    // is lower priority than speed.
>    bool drawingLowPrecision = aTiledBuffer.IsLowPrecision();
>  
>    // Find out if we have any non-stale content to update.
> +  nsIntRegion freshRegion = aInvalidRegion;

Lets only assign this in the else block below.

@@ +313,5 @@
> +  if (!drawingLowPrecision && aInvalidRegion.Intersects(criticalViewportRect)) {
> +    paintInSingleTransaction = true;
> +    aRegionToPaint.And(aInvalidRegion, criticalViewportRect);
> +  } else {
> +    // Paint tiles that have no content before tiles that only have stale content.

I don't understand why we want to do this. Suppose that we have the top half of a gif painted at frame 0, then it changes frame 1 and we show the bottom half. If we paint the freshRegion and display that update then we will show top half frame 0, bottom half frame 1. I don't understand why we wouldn't update stale region before draw fresh region?

Comment 53

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #51)
> (In reply to Chris Lord [:cwiiis] from comment #46)
> > Created attachment 683859 [details] [diff] [review]
> > Part 5 - Add low precision buffer
> > Using IsRepeat() is ok as it's only true on the second iteration (this
> > differs from GetRepeat, which will change immediately after SetRepeat).
> > 
> 
> I'm not sure I follow the logic. The first time IsRepeat is false which we
> may paint hi res tiles and repeat the transaction which then in a repeated
> transaction we may start painting the low res. Inside the low res code you
> have code that does work on !IsRepeat() which wont be true since we've
> painted hi res tiles.

The invalid regions don't change in-between transactions, so this scenario where high-res paints in one transaction, then low-res in the next can't happen - Low res tiles are never painted in response to the valid region growing and the display-port can't change between transactions.

i.e. if there is low res area to paint, it will be painted in the first transaction - before part 10, there is the possibility low-res would lag behind, but it always starts at the same time as high-res, so there's no possibility of the !IsRepeatTransaction() block being hit.

Comment 54

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #52)
> Comment on attachment 683862 [details] [diff] [review]
> Part 9 - Fix coherency when doing progressive updates (again)
> 
> Review of attachment 683862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicTiledThebesLayer.cpp
> @@ +281,5 @@
> >    // is lower priority than speed.
> >    bool drawingLowPrecision = aTiledBuffer.IsLowPrecision();
> >  
> >    // Find out if we have any non-stale content to update.
> > +  nsIntRegion freshRegion = aInvalidRegion;
> 
> Lets only assign this in the else block below.

Sounds good to me.

> @@ +313,5 @@
> > +  if (!drawingLowPrecision && aInvalidRegion.Intersects(criticalViewportRect)) {
> > +    paintInSingleTransaction = true;
> > +    aRegionToPaint.And(aInvalidRegion, criticalViewportRect);
> > +  } else {
> > +    // Paint tiles that have no content before tiles that only have stale content.
> 
> I don't understand why we want to do this. Suppose that we have the top half
> of a gif painted at frame 0, then it changes frame 1 and we show the bottom
> half. If we paint the freshRegion and display that update then we will show
> top half frame 0, bottom half frame 1. I don't understand why we wouldn't
> update stale region before draw fresh region?

This part hasn't changed - we always drew fresh first. The scenario you suggest will always be broken though - We can't maintain coherency if something is partially fresh and stale at the same time unless we just give up on multiple transactions.

What I realise I have broken with this change is something that's only partially on the screen when drawing, but is entirely in the stale region - this will cause an unnecessary visible split.

To fix this, we would have to draw stale first, and we'd have to union it with the previous screen region - I'll make this change.

Comment 55

[Chris Lord [:cwiiis]]

Attachment: Part 9 - Fix coherency when doing progressive updates (again) v2

Addresses review comments, hopefully.

Comment 56

[Chris Lord [:cwiiis]]

Earlier try push had patches in the queue that broke talos tests (but otherwise was green, which is encouraging :))

Here's a push with the latest patches, rebased and without the breaking patches: https://tbpl.mozilla.org/?tree=Try&rev=badff86f29ca

Comment 57

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 683859 [details] [diff] [review]
Part 5 - Add low precision buffer

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

browser.js changes look good to me; I only skimmed the rest of the code.

::: mobile/android/chrome/content/browser.js
@@ +2762,5 @@
> +
> +      let dpX = Math.min(Math.max(displayPort.x - displayPort.width * 1.5,
> +                                  pageRect.left - geckoScrollX), pageXMost - dpW);
> +      let dpY = Math.min(Math.max(displayPort.y - displayPort.height * 1.5,
> +                                  pageRect.top - geckoScrollY), pageYMost - dpH);

Looks good. Maybe use ViewportHandler.clamp() here if you think it's more readable? I don't care much either way.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 683860 [details] [diff] [review]
Part 8 - Fix ProgressiveUpdateData

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

Good catch

Comment 59

[Benoit Girard (:BenWa)]

Comment on attachment 683859 [details] [diff] [review]
Part 5 - Add low precision buffer

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

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +165,5 @@
> +
> +void
> +TiledThebesLayerOGL::ProcessLowPrecisionUploadQueue()
> +{
> +  if (!mPendingLowPrecisionUpload)

Why not mLowResPrecissionRegionToUpload.IsEmpty()?

@@ +190,5 @@
>  
>  void
>  TiledThebesLayerOGL::ProcessUploadQueue()
>  {
> +  if (!mPendingUpload)

Why not use mRegionToUpload.IsEmpty() || mLowResPrecissionRegionToUpload.IsEmpty()?

@@ -193,5 @@
>  
>    mVideoMemoryTiledBuffer.Upload(&mMainMemoryTiledBuffer,
>                                   mMainMemoryTiledBuffer.GetValidRegion(),
>                                   mRegionToUpload, resolution);
> -  mVideoMemoryTiledBuffer.SetResolution(mMainMemoryTiledBuffer.GetResolution());

We should still set the FrameResolution or remove the tile store (or put a fat warning).

Comment 60

[Chris Lord [:cwiiis]]

Attachment: Part 11 - Put low precision rendering behind a pref

This pref doesn't stop *all* the work - ideally, it'd also be checked in browser.js and a critical displayport wouldn't get set, but I think this is probably good enough. This would at least stop us using the memory and doing the rendering, but the display-list would be larger than is necessary.

Comment 61

[(no longer active)]

Backed out because this does not compile on Windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f61af760950

Comment 62

[Chris Lord [:cwiiis]]

Sorry about that - I've removed the offending roundf and seeing as the tree is closed, pushed a build to try to make extra sure:

https://tbpl.mozilla.org/?tree=Try&rev=320a48a844de

Will push back on inbound when this is looking green.

Comment 63

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0171b51aa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a79d36c9174b
https://hg.mozilla.org/integration/mozilla-inbound/rev/729f9f0d437f
https://hg.mozilla.org/integration/mozilla-inbound/rev/13917d4b99b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74b22565be0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d09617883c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1b3e8b8ac7
https://hg.mozilla.org/integration/mozilla-inbound/rev/808c7d0aedec
https://hg.mozilla.org/integration/mozilla-inbound/rev/d014526b795e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e93e08cd1d4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d013240eac

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b0171b51aa3
https://hg.mozilla.org/mozilla-central/rev/a79d36c9174b
https://hg.mozilla.org/mozilla-central/rev/729f9f0d437f
https://hg.mozilla.org/mozilla-central/rev/13917d4b99b8
https://hg.mozilla.org/mozilla-central/rev/f74b22565be0
https://hg.mozilla.org/mozilla-central/rev/7d09617883c7
https://hg.mozilla.org/mozilla-central/rev/3f1b3e8b8ac7
https://hg.mozilla.org/mozilla-central/rev/808c7d0aedec
https://hg.mozilla.org/mozilla-central/rev/d014526b795e
https://hg.mozilla.org/mozilla-central/rev/e93e08cd1d4c
https://hg.mozilla.org/mozilla-central/rev/c4d013240eac

Comment 65

[Aaron Train [:aaronmt]]

Nomination for Aurora? Aurora is still affected by a few obvious dependencies (re: invalidation issues).

Comment 66

[Chris Lord [:cwiiis]]

(In reply to Aaron Train [:aaronmt] from comment #65)
> Nomination for Aurora? Aurora is still affected by a few obvious
> dependencies (re: invalidation issues).

I'll wrap all these patches and those in follow up bugs into one patch rebased on aurora to make this a bit easier to merge - I'll do this on Monday morning (or earlier if I have some time this weekend).

There's still (at least) one invalidation bug after all of these patches, I'm hoping to track down and fix this next week.

Comment 67

[Kartikaya Gupta (email:[e-mail])]

Note: this work was uplifted to aurora as part of bug 817575.