Bug 49629

Summary: [chromium] Compositor needs to manage its VRAM use
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, enne, kbr, nduca, scheib, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 50114    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
kbr: review+, commit-queue: commit-queue-
Added text expectations for mac
none
Patch
none
Patch
none
with suggested renames kbr: review+

Description James Robinson 2010-11-16 15:40:48 PST
[chromium] Compositor needs to manage its VRAM use
Comment 1 James Robinson 2010-11-16 15:54:43 PST
Created attachment 74050 [details]
Patch
Comment 2 James Robinson 2010-11-16 15:57:58 PST
Here's a patch for initial feedback.  I believe it is functionally complete but I haven't constructed tests for it yet and there are some debugging printf()s left in.  I can zoom in/out on maps.google.com quite a bit with a low memory limit and observe that it is evicting textures without crashing, so that's a good sign.

I think the biggest design question is whether to use opaque tokens and raw texture ids or to provide a wrapped texture object to callers.  This way seemed a little simpler to implement at first, but see what you think.
Comment 3 Vangelis Kokkevis 2010-11-17 10:09:10 PST
Comment on attachment 74050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74050&action=review

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:174
> +    return m_contentsDirty || !m_contentsTextureToken || !layerRenderer()->textureManager()->hasTexture(m_contentsTextureToken);

I think we should fold the contentsDirty() call inside updateContents() to make sure that they are called in succession.  Right now there are no explicit guarantees that the texture won't be reclaimed in between.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:316
>      GraphicsContext3D* context = layerRendererContext();

You could, in theory, protect the texture here and unprotect it after the draw to make explicit the fact that the texture should not be reclaimed in between these two steps.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:64
> +        unsigned textureLayerShaderProgram() const { return m_textureLayerShaderProgram; }

Since these are now defined inside RenderSurfaceChromium::SharedValues, you can remove the textureLayerShader* prefix (which is also no longer accurate)

> WebCore/platform/graphics/chromium/TextureManager.cpp:69
> +    ASSERT(!m_textures.get(token).isProtected);

Is there any harm in calling protectTexture twice in a row?
Comment 4 Vangelis Kokkevis 2010-11-17 10:45:07 PST
I meant to add a couple more comments here:

I like the patch and thanks for the RenderSurface cleanup as well. 

I would prefer if possible to wrap the texture token, id and size in a single object so that there aren't that many implicit assumptions.  How about we do something like the following:

class LayerTexture {
public:
  bool isValid() { /* Texture manager checks if the token is still valid */ }

  bool reserve(IntSize) { /* call the texture manager to create a gl texture and allocate it if needed, or use the existing one if it's still valid and matches the size.  Also protect the texture against deletions. */ }

  bool unreserve() { /* call the texture manager to mark the texture as unprotected. */ } ;

private:
  LayerTexture(TextureManager* m) : m_token(0), m_glId(0), m_textureManager(m) {};

  IntSize m_allocatedSize;
  unsigned m_token;
  unsigned m_glId;
}

Now the various layers hold a pointer to a LayerTexture rather that the gl Id and the allocated size, etc.

Now we could create another method on the LayerChromium that combines the current contentsDirty, updateContents, and draw methods and is what the compositor uses.  For example:

ContentLayerChromium::updateAndDraw() {
  if (!m_layerTexture)
     m_layerTexture = layerManager->createLayerTexture();

  if (!m_layerTexture->isValid() || !m_layerTexture->size() != requiredSize) {
     m_contentsDirty = true;
     m_dirtyRect = m_bounds;
  }

  // After calling reserve we're guaranteed that:
  // a. A gl texture of the appropriate size has been allocated
  // b. The texture has been marked as protected.
  m_layerTexture.reserve(requiredSize, GC3D::RGBA);

  // Guard against out of memory conditions.
  if (!m_layerTexture->isValid())
     return;

  if (m_contentsDirty) {
    // update the texture contents
  }

  draw();

  // Mark texture as unprotected
  m_layerTexture.unreserve();
}


I think most of the logic you currently have in the texture manager will remain intact but be wrapped in different APIs.


(In reply to comment #3)
> (From update of attachment 74050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74050&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:174
> > +    return m_contentsDirty || !m_contentsTextureToken || !layerRenderer()->textureManager()->hasTexture(m_contentsTextureToken);
> 
> I think we should fold the contentsDirty() call inside updateContents() to make sure that they are called in succession.  Right now there are no explicit guarantees that the texture won't be reclaimed in between.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:316
> >      GraphicsContext3D* context = layerRendererContext();
> 
> You could, in theory, protect the texture here and unprotect it after the draw to make explicit the fact that the texture should not be reclaimed in between these two steps.
> 
> > WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:64
> > +        unsigned textureLayerShaderProgram() const { return m_textureLayerShaderProgram; }
> 
> Since these are now defined inside RenderSurfaceChromium::SharedValues, you can remove the textureLayerShader* prefix (which is also no longer accurate)
> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:69
> > +    ASSERT(!m_textures.get(token).isProtected);
> 
> Is there any harm in calling protectTexture twice in a row?
Comment 5 Vincent Scheib 2010-11-17 11:02:19 PST
Like it + Vangelis's suggestions in context of compositor. 

I wonder about managing all texture use for a renderer / browser. Canvas will be creating textures as well, it would be nice if a single texture memory tracker could be used to free textures from any source. Perhaps we can iterate and refactor that later, but perhaps holding off doing anything we'll just need to undo then. e.g. naming "LayerTexture" vs something like "MemManagedGPUTexture".
Comment 6 Nat Duca 2010-11-17 13:20:44 PST
(In reply to comment #5)
> I wonder about managing all texture use for a renderer / browser. Canvas will be creating textures as well, ...

The compositor will likely be moving to a secondary thread whereas [I assume] Canvas will live on the main thread. Fusion between the two may not be feasible in such a world.
Comment 7 James Robinson 2010-11-17 19:13:42 PST
Created attachment 74194 [details]
Patch
Comment 8 James Robinson 2010-11-17 19:16:47 PST
Here it is with LayerTexture and the other fixes suggested.  I didn't combine update with draw as it sounds like for masks we need to keep them separate.  I'm also not handling allocation failures properly in RenderSurface - I think we need to add a skipDrawing flag to it as well and propagate it out somehow.

Still needs tests.
Comment 9 James Robinson 2010-11-18 19:18:48 PST
Created attachment 74342 [details]
Patch
Comment 10 Vangelis Kokkevis 2010-11-18 20:10:12 PST
Comment on attachment 74342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74342&action=review

Looks good overall.  A couple of comments inline.

> WebCore/ChangeLog:13
> +        The TextureManager works by providing tokens to callers that want to use a managed texture.

This part of the description should be modified to reflect the new LayerTexture wrapper object.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:313
>          m_allocatedTextureSize = requiredTextureSize;

I mention this further down as well but I think it would be better for the LayerTexture to also store the reserved size instead of keeping track of it with m_allocatedTextureSize here. You could potentially pass the requiredTextureSize to the isValid() call to test it against the already allocated size.

> WebCore/platform/graphics/chromium/LayerTexture.cpp:70
> +

I think it would be convenient for a LayerTexture to also store its size. This would simplify the layer update logic as the layer won't have to store the allocated texture size itself.

> WebCore/platform/graphics/chromium/LayerTexture.h:52
> +    void bindTexture(unsigned target);

I don't think it's necessary to pass the target as a param.  It's always going to be TEXTURE_2D.

> WebCore/platform/graphics/chromium/LayerTexture.h:53
> +    void framebufferTexture2D(unsigned long target, unsigned long attachment, unsigned long textarget, long level);

Similarly here, none of the params are really useful.

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:72
> +        LOG_ERROR("RenderSurfaceChromium: Failed to create scroll shader program");

Remove the word scroll

> WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> +        int renderSurfaceShaderSamplerLocation() const { return m_renderSurfaceShaderSamplerLocation; }

To stay consistent with other Shared value classes, please remove the renderSurface prefix from all members and methods.
Comment 11 James Robinson 2010-11-19 16:16:56 PST
Created attachment 74439 [details]
Patch
Comment 12 James Robinson 2010-11-19 16:17:58 PST
Addressed feedback.  I think the size part is a lot better this way - the LayerChromiums don't have to care what they have allocated in the past, they just have to ask the LayerTexture if it currently has a texture of the right size/format for the current frame in updateContentsIfDirty().
Comment 13 Vangelis Kokkevis 2010-11-19 17:43:30 PST
Comment on attachment 74439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review

Other than a bit of cleanup in the content layer, this looks good! You'll have to find a WK reviewer though to bless it.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:292
> +void ContentLayerChromium::updateTextureRect(void* pixels, const IntSize& bitmapSize, const IntSize& requiredTextureSize, const IntRect& requestedUpdateRect)

I think we don't need to pass in bitmap size as it will always be equal to the size of the requestedUpdateRect.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:303
> +    if (!m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA)) {

I don't believe we need this test either. Since we get the pixels from our caller, we have to assume that we get enough to update the entire texture if it's not valid.  This check is already done in line 219 above.
Comment 14 Vangelis Kokkevis 2010-11-19 18:08:38 PST
Comment on attachment 74439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review

One more thing that just occurred to me.

> WebCore/platform/graphics/chromium/TextureManager.cpp:142
> +    size_t memoryRequiredBytes = memoryUseBytes(size, format);

You also need to call: layerRenderer()->checkTextureSize(size) to make sure that the texture is within the range supported by the h/w.
Comment 15 James Robinson 2010-11-19 18:13:45 PST
(In reply to comment #14)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> One more thing that just occurred to me.
> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:142
> > +    size_t memoryRequiredBytes = memoryUseBytes(size, format);
> 
> You also need to call: layerRenderer()->checkTextureSize(size) to make sure that the texture is within the range supported by the h/w.

Shouldn't the caller (the *LayerChromium) do this check?  Right now the TextureManager doesn't know anything about the LayerRenderer.
Comment 16 James Robinson 2010-11-19 18:29:29 PST
(In reply to comment #13)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> Other than a bit of cleanup in the content layer, this looks good! You'll have to find a WK reviewer though to bless it.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:292
> > +void ContentLayerChromium::updateTextureRect(void* pixels, const IntSize& bitmapSize, const IntSize& requiredTextureSize, const IntRect& requestedUpdateRect)
> 
> I think we don't need to pass in bitmap size as it will always be equal to the size of the requestedUpdateRect.

True!  Removed.
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:303
> > +    if (!m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA)) {
> 
> I don't believe we need this test either. Since we get the pixels from our caller, we have to assume that we get enough to update the entire texture if it's not valid.  This check is already done in line 219 above.

This isn't true for ImageLayerChromium - the updateRect is just the dirty rect.  I could have ImageLayerChromium::updateContentsIfDirty grow the dirty rectangle if the layer is dirty because of its LayerTexture being evicted, but that seems a little odd.
Comment 17 Vangelis Kokkevis 2010-11-22 10:43:54 PST
Comment on attachment 74439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:304
>          ASSERT(bitmapSize == requiredTextureSize);

The problem is that if we ever end up growing the updateRect here then the new size won't match the size of the bitmap that "pixels" points to. As a result, texSubImage2D will either produce wrong results or read into uninitialized memory. I think that we need to check for the right texture size in ImageLayerChromium much like ContentLayerChromium does.

> WebCore/platform/graphics/chromium/TextureManager.cpp:143
> +    if (memoryRequiredBytes > m_memoryLimitBytes || !reduceMemoryToLimit(m_memoryLimitBytes - memoryRequiredBytes))

It seems more natural if texture manager does it as it's the texture manager that tries to allocate the memory for the texture.  What you could do is record the maximum supported texture size at creation time by calling:

GLC(m_context, m_context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &m_maxTextureSize));

much like the LayerRenderer does.  As a matter of fact that call could probably be completely removed from the layer renderer once all the layer textures are provided by the texture manager.
Comment 18 James Robinson 2010-11-22 11:50:55 PST
(In reply to comment #17)
> (From update of attachment 74439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74439&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:304
> >          ASSERT(bitmapSize == requiredTextureSize);
> 
> The problem is that if we ever end up growing the updateRect here then the new size won't match the size of the bitmap that "pixels" points to. As a result, texSubImage2D will either produce wrong results or read into uninitialized memory. I think that we need to check for the right texture size in ImageLayerChromium much like ContentLayerChromium does.

Yeah, I'll unify this logic.

> 
> > WebCore/platform/graphics/chromium/TextureManager.cpp:143
> > +    if (memoryRequiredBytes > m_memoryLimitBytes || !reduceMemoryToLimit(m_memoryLimitBytes - memoryRequiredBytes))
> 
> It seems more natural if texture manager does it as it's the texture manager that tries to allocate the memory for the texture.  What you could do is record the maximum supported texture size at creation time by calling:
> 
> GLC(m_context, m_context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &m_maxTextureSize));
> 
> much like the LayerRenderer does.  As a matter of fact that call could probably be completely removed from the layer renderer once all the layer textures are provided by the texture manager.

Good point.
Comment 19 James Robinson 2010-11-22 14:10:55 PST
Created attachment 74596 [details]
Patch
Comment 20 James Robinson 2010-11-22 14:13:33 PST
This makes sure that the updateRect is the right size before calling updateTextureRect and adds a max texture size limit to TextureManager.
Comment 21 Kenneth Russell 2010-11-22 16:48:40 PST
Comment on attachment 74596 [details]
Patch

r+'ing based on Vangelis's review. Submitting to commit queue.
Comment 22 WebKit Commit Bot 2010-11-22 23:28:15 PST
Comment on attachment 74596 [details]
Patch

Rejecting patch 74596 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ts/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...........................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
618.69s total testing time

22016 test cases (99%) succeeded
2 test cases (<1%) were new
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6332009
Comment 23 Kenneth Russell 2010-11-23 10:48:11 PST
The commit queue failure above looks odd -- like DumpRenderTree silently aborted. Should we cq+ this again?
Comment 24 Vangelis Kokkevis 2010-11-23 11:01:41 PST
(In reply to comment #23)
> The commit queue failure above looks odd -- like DumpRenderTree silently aborted. Should we cq+ this again?

Yes, please.
Comment 25 WebKit Commit Bot 2010-11-23 12:13:01 PST
The commit-queue encountered the following flaky tests while processing attachment 74596 [details]:

http/tests/misc/uncacheable-script-repeated.html

Please file bugs against the tests.  These tests were authored by koivisto@iki.fi.  The commit-queue is continuing to process your patch.
Comment 26 WebKit Commit Bot 2010-11-23 12:30:12 PST
The commit-queue encountered the following flaky tests while processing attachment 74596 [details]:

compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 27 WebKit Commit Bot 2010-11-23 13:58:12 PST
Comment on attachment 74596 [details]
Patch

Rejecting patch 74596 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
ts/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...........................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
537.57s total testing time

22019 test cases (99%) succeeded
2 test cases (<1%) were new
13 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6378023
Comment 28 Vangelis Kokkevis 2010-11-24 14:34:16 PST
Created attachment 74793 [details]
Added text expectations for mac
Comment 29 Kenneth Russell 2010-11-24 14:41:43 PST
Comment on attachment 74793 [details]
Added text expectations for mac

Let's try again.
Comment 30 Kenneth Russell 2010-11-24 14:55:50 PST
Removed cq+ hoping that the commit queue hasn't picked this up already. Due to time constraints we are going to land this manually and fix up any remaining test failures.
Comment 31 Vangelis Kokkevis 2010-11-24 15:00:44 PST
Committed r72701: <http://trac.webkit.org/changeset/72701>
Comment 32 James Robinson 2010-12-07 22:10:43 PST
This patch was reverted in r72770 due to rendering issues.  It looks like the problem is that I didn't understand the clipped update path at all for content layers.  I'll try to construct a test case and update the patch ASAP.
Comment 33 James Robinson 2010-12-07 23:21:14 PST
Here's a diff to the previous patch to make it actually work:


--- a/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
+++ b/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
@@ -212,17 +212,17 @@ void ContentLayerChromium::updateContentsIfDirty()
             return;
         }
 
+        // If the texture needs to be reallocated then we must redraw the entire
+        // visible contents of the layer.
+        if (!m_contentsTexture || !m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA))
+            m_dirtyRect = boundsRect;
+
         calculateClippedUpdateRect(dirtyRect, m_largeLayerDrawRect);
         if (!layerRenderer()->checkTextureSize(m_largeLayerDrawRect.size())) {
             m_skipsDraw = true;
             return;
         }
 
-        // If the texture needs to be reallocated then we must redraw the entire
-        // contents of the layer.
-        if (!m_contentsTexture || !m_contentsTexture->isValid(requiredTextureSize, GraphicsContext3D::RGBA))
-            dirtyRect = boundsRect;
-
         // If the portion of the large layer that's visible hasn't changed
         // then we don't need to update it, _unless_ its contents have changed
         // in which case we only update the dirty bits.
@@ -230,7 +230,7 @@ void ContentLayerChromium::updateContentsIfDirty()
             if (!m_dirtyRect.intersects(dirtyRect))
                 return;
             dirtyRect.intersect(IntRect(m_dirtyRect));
-            updateRect = dirtyRect;
+            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
             requiredTextureSize = m_largeLayerDirtyRect.size();
         } else {
             m_largeLayerDirtyRect = dirtyRect;

I've manually tested this pretty carefully in the large layer case to ensure that we still render correctly if the large layer's backing texture is evicted between frames (although we render brutally slowly if we have to upload 65MB+ of texture data for each frame).  New patch incoming..
Comment 34 James Robinson 2010-12-07 23:35:52 PST
Created attachment 75878 [details]
Patch
Comment 35 James Robinson 2010-12-07 23:37:36 PST
Relative to the last patch the only changes here are (other than merging up):

- the changes in comment #33
- updating several callers of the GLC() macro so they compile if DEBUG_GL_CALLS is set to 1
- moves the new tests from LayoutTests/compositing to LayoutTests/platform/chromium/compositing.  These tests are very particular to the chromium compositor implementation and aren't generally useful for other ports.  Having the tests in LT/p/c/ also means we don't have to worry about expectations for other ports
Comment 36 Kenneth Russell 2010-12-08 12:03:35 PST
Comment on attachment 75878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75878&action=review

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());

I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?
Comment 37 James Robinson 2010-12-08 14:04:00 PST
(In reply to comment #36)
> (From update of attachment 75878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75878&action=review
> 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> > +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
> 
> I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?

This is definitely subtle.  In this function, dirtyRect is the dirty rectangle in the coordinate space of the layer and updateRect is in the coordinate space of the texture.  For large layers we always paint the dirty region of the layer (bounded by dirtyRect) into a software bitmap and then upload the entire software bitmap into a texture (bound by updateRect).  For "normal" layers the updateRect is not necessarily the entire bounds of the texture - it might be a subregion - but it's still in the coordinate space of the texture, not the layer.

To be honest I'm kind of amazed that this piece of code worked without this change - I suspect it had bugs that we just didn't catch.
Comment 38 James Robinson 2010-12-08 14:05:39 PST
(In reply to comment #37)
> (In reply to comment #36)
> > (From update of attachment 75878 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75878&action=review
> > 
> > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:233
> > > +            updateRect = IntRect(IntPoint(0, 0), dirtyRect.size());
> > 
> > I'm sure that your testing has verified the correctness of this change, but it looks like the effect is to move the updateRect to the upper-left corner. Is that the correct coordinate system transformation? Or is this supposed to create a rectangle starting at (0,0) and extending to the lower-right point of dirtyRect?
> 
> This is definitely subtle.  In this function, dirtyRect is the dirty rectangle in the coordinate space of the layer and updateRect is in the coordinate space of the texture.  For large layers we always paint the dirty region of the layer (bounded by dirtyRect) into a software bitmap and then upload the entire software bitmap into a texture (bound by updateRect).  For "normal" layers the updateRect is not necessarily the entire bounds of the texture - it might be a subregion - but it's still in the coordinate space of the texture, not the layer.

Actually now that I've written it out I'm not entirely sure that it's correct. Hmm...
> 
> To be honest I'm kind of amazed that this piece of code worked without this change - I suspect it had bugs that we just didn't catch.
Comment 39 James Robinson 2010-12-08 20:40:00 PST
Comment on attachment 75878 [details]
Patch

The large layer update logic isn't quite 100%.  Will fix and update.
Comment 40 James Robinson 2010-12-08 21:16:31 PST
Created attachment 76012 [details]
Patch
Comment 41 James Robinson 2010-12-08 21:18:03 PST
(In reply to comment #40)
> Created an attachment (id=76012) [details]
> Patch

The only difference between this and the previous patch is in ContentLayerChromium.cpp/h.  I rewrote the large layer handling following the design that Ken, Vangelis and I worked out on the whiteboard.  Hopefully it's a bit easier to follow now.
Comment 42 Vangelis Kokkevis 2010-12-09 14:29:06 PST
Comment on attachment 76012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76012&action=review

I believe the new large layer math is right.  Just a few comments on the variable names to make sure they reflect the actual space the various entities are in.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:206
> +        IntRect visibleRectInOriginCoords = layerOriginTransform.mapRect(visibleRectInLayerCoords);

Instead of "InOriginCoords" I think you should use in "SurfaceCoords" .  This is an IntRect expressed in the target RenderSurface coordinate system.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:208
> +

m_layerCenterInSurfaceCoords

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:231
> +        IntSize layerOffsetFromRenderSurfaceCoords(visibleRectInLayerCoords.x(), visibleRectInLayerCoords.y());

I think layerOffsetFromRenderSurfaceCoords should be called visibleRectOffsetInLayerCoords as you are still in layer coords.
Comment 43 James Robinson 2010-12-09 15:41:36 PST
Created attachment 76127 [details]
with suggested renames
Comment 44 Vangelis Kokkevis 2010-12-09 16:21:06 PST
Comment on attachment 76127 [details]
with suggested renames

Looks good!  Thanks.
Comment 45 Kenneth Russell 2010-12-09 16:32:05 PST
Comment on attachment 76127 [details]
with suggested renames

Looks good to me too.
Comment 46 James Robinson 2010-12-09 17:45:31 PST
Committed r73663: <http://trac.webkit.org/changeset/73663>