Bug 54315 - [chromium] Update texture for ContentLayerChromiums in draw() call instead of updateContents..() call
Summary: [chromium] Update texture for ContentLayerChromiums in draw() call instead of...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks: 54047
  Show dependency treegraph
 
Reported: 2011-02-11 14:57 PST by James Robinson
Modified: 2011-03-03 10:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (17.24 KB, patch)
2011-02-11 14:59 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (17.42 KB, patch)
2011-02-11 15:06 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (22.42 KB, patch)
2011-02-15 18:37 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (22.46 KB, patch)
2011-02-15 19:17 PST, James Robinson
no flags Details | Formatted Diff | Diff
add check in unreserveContentsTexture() to match the one in bindContentsTexture() (22.62 KB, patch)
2011-02-15 19:25 PST, James Robinson
no flags Details | Formatted Diff | Diff
fixes mac compile, adds helper for bitmap locking/unlocking (23.22 KB, patch)
2011-02-16 17:56 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2011-02-17 15:04 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (23.74 KB, patch)
2011-02-17 15:12 PST, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-02-11 14:57:18 PST
[chromium] Update texture for ContentLayerChromiums in draw() call instead of updateContents..() call
Comment 1 James Robinson 2011-02-11 14:59:04 PST
Created attachment 82185 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-11 15:02:11 PST
Attachment 82185 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7868723
Comment 3 James Robinson 2011-02-11 15:06:13 PST
Created attachment 82188 [details]
Patch
Comment 4 Kenneth Russell 2011-02-11 18:04:03 PST
Comment on attachment 82188 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:299
> +        SkAutoLockPixels lock(bitmap);
> +        // FIXME: do we need to support more image configurations?
> +        if (bitmap.config() == SkBitmap::kARGB_8888_Config)
> +            pixels = bitmap.getPixels();

This looks unsafe. The SkAutoLockPixels goes out of scope here but pixels is used later.
Comment 5 James Robinson 2011-02-14 18:41:04 PST
Good catch, that is unsafe.  Vangelis pointed out some other issues with this patch offline - I'll try to update this soon.
Comment 6 James Robinson 2011-02-15 18:37:31 PST
Created attachment 82569 [details]
Patch
Comment 7 James Robinson 2011-02-15 19:17:18 PST
Created attachment 82572 [details]
Patch
Comment 8 James Robinson 2011-02-15 19:24:07 PST
The general idea is that ContentLayerChromium::updateContentsIfNeeded() updates the software backing bitmap (which is a skia::PlatformCanvas for content layers on windows+linux and a Vector<uint8_t> for content layers on mac + image layers on all platforms) and sets the m_updateUpdateRect to the portion of the texture that needs an update.  The actual texture upload happens in updateTextureRectIfNeeded() which is called by draw() (for normal layers) and in bindContentsTexture() (needed for mask layers which don't draw themselves).  The upload function checks if the backing texture is available and uploads either the dirty rect or the entire texture as needed.  I've also added a check for m_skipsDraw in bindContentsTexture() so we don't attempt to bind textures as masks for layers that we can't render.
Comment 9 James Robinson 2011-02-15 19:25:18 PST
Created attachment 82573 [details]
add check in unreserveContentsTexture() to match the one in bindContentsTexture()
Comment 10 WebKit Review Bot 2011-02-15 23:05:26 PST
Attachment 82573 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7920168
Comment 11 Kenneth Russell 2011-02-16 13:55:13 PST
Comment on attachment 82573 [details]
add check in unreserveContentsTexture() to match the one in bindContentsTexture()

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

This looks good to me. One further comment about safety of locking and unlocking the bitmap. You might want to add a simple file-local class which only conditionally unlocks the pixels in its destructor. Let me know if you'd like me to r+ this version or want to fix the Mac compilation failure first.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:335
>          return;

I think we'd need to potentially unlock the bitmap's pixels here.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:347
> +        ptrdiff_t srcOffset = (m_uploadUpdateRect.y() + row) * srcStride + m_uploadUpdateRect.x() * 4;
> +        ptrdiff_t destOffset = row * destStride;

Might be useful to have some assertions here to make sure the copies don't go out of bounds.
Comment 12 James Robinson 2011-02-16 14:17:24 PST
Thanks for looking!  I'll figure out the compile error on mac and update the patch.
Comment 13 James Robinson 2011-02-16 17:56:01 PST
Created attachment 82730 [details]
fixes mac compile, adds helper for bitmap locking/unlocking
Comment 14 Vangelis Kokkevis 2011-02-17 00:13:24 PST
Comment on attachment 82730 [details]
fixes mac compile, adds helper for bitmap locking/unlocking

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:311
> +    SkBitmapConditionalAutoLockerPixels()

Can't you use Skia's SkAutoLockPixels ?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:333
> +void ContentLayerChromium::updateTextureRectIfNeeded()

nit:  rename to updateTextureIfNeeded() since you don't pass a rect to the function anymore?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:370
> +    Vector<uint8_t> uploadBuffer(m_uploadUpdateRect.height() * destStride);

In the presumably common case where your uploadUpdateRect has the same dimensions as your texture (full layer invalidate), you could avoid copying to the uploadBuffer and do the texSubImage2D call directly from the pixel data.
Comment 15 James Robinson 2011-02-17 14:49:22 PST
(In reply to comment #14)
> (From update of attachment 82730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82730&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:311
> > +    SkBitmapConditionalAutoLockerPixels()
> 
> Can't you use Skia's SkAutoLockPixels ?

The problem is I have to have the SkBitmap when constructing the SkAutoLockPixels and I couldn't get the blocks to line up nicely with that constraint. This helper lets me lock the pixels whenever and ensures that there's an unlock made before the function returns no matter when the lock happens.

> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:333
> > +void ContentLayerChromium::updateTextureRectIfNeeded()
> 
> nit:  rename to updateTextureIfNeeded() since you don't pass a rect to the function anymore?

good idea, will do

> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:370
> > +    Vector<uint8_t> uploadBuffer(m_uploadUpdateRect.height() * destStride);
> 
> In the presumably common case where your uploadUpdateRect has the same dimensions as your texture (full layer invalidate), you could avoid copying to the uploadBuffer and do the texSubImage2D call directly from the pixel data.


also a good idea - should be easy to do.
Comment 16 James Robinson 2011-02-17 15:04:26 PST
Created attachment 82866 [details]
Patch
Comment 17 James Robinson 2011-02-17 15:12:05 PST
Created attachment 82868 [details]
Patch
Comment 18 James Robinson 2011-02-17 15:25:39 PST
This avoids allocating an extra upload bitmap if the upload region's stride matches the texture and adds some assertions to try to keep the offsets from going out of bounds.
Comment 19 Kenneth Russell 2011-02-17 16:52:22 PST
Vangelis, can you please do the unofficial review of this patch?
Comment 20 Vangelis Kokkevis 2011-02-17 22:47:41 PST
Comment on attachment 82868 [details]
Patch

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

unofficial r+ from me

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:102
> +    if (m_uploadBufferSize != bitmapSize) {

nit: per webkit style, you shouldn't be using braces here.
Comment 21 Kenneth Russell 2011-02-18 12:00:45 PST
Comment on attachment 82868 [details]
Patch

Looks good to me.
Comment 22 James Robinson 2011-02-18 14:18:19 PST
Committed r79033: <http://trac.webkit.org/changeset/79033>
Comment 23 Antoine Labour 2011-03-02 16:54:22 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=82868&action=review

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:375
> +    uint8_t* uploadPixels = pixels + srcStride * m_uploadUpdateRect.x();

Shouldn't this be something like pixels + srcStride * m_uploadUpdateRect.y() + 4*m_uploadUpdateRect.x() instead ?

I'm getting a crash in glTexSubImage2D where the pixels argument points out of memory.
Comment 24 James Robinson 2011-03-02 16:57:33 PST
(In reply to comment #23)
> View in context: https://bugs.webkit.org/attachment.cgi?id=82868&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:375
> > +    uint8_t* uploadPixels = pixels + srcStride * m_uploadUpdateRect.x();
> 
> Shouldn't this be something like pixels + srcStride * m_uploadUpdateRect.y() + 4*m_uploadUpdateRect.x() instead ?
> 
> I'm getting a crash in glTexSubImage2D where the pixels argument points out of memory.

Good point - this math is totally wrong for non-zero x().  Yikes.

What's the case that fails?  Our testing is obviously insufficient.
Comment 25 Antoine Labour 2011-03-02 17:10:05 PST
I don't have an exact repro case... I was tracking something else and selection on the page was causing garbage to be painted, and then it crashed into the debugger which let me debug it. I'll update the bug if I have clear repro steps.
Comment 26 James Robinson 2011-03-02 17:19:20 PST
Actually, if there's an x offset to the upload rect then the strides won't match and should fall into the row-by-row copy path which seems OK.

This code would be wrong if there was a non-zero y offset, but I think in that case it'd be more likely that it would upload pixels at the wrong offset rather than pure garbage.  I need to take a closer look.
Comment 27 Adrienne Walker 2011-03-03 10:52:44 PST
(In reply to comment #26)
> Actually, if there's an x offset to the upload rect then the strides won't match and should fall into the row-by-row copy path which seems OK.
> 
> This code would be wrong if there was a non-zero y offset, but I think in that case it'd be more likely that it would upload pixels at the wrong offset rather than pure garbage.  I need to take a closer look.

For what it's worth, I'm in the middle of refactoring/removing that code to use the row-by-row copy logic in LayerTilerChromium.cpp instead.  (Oh, duplicated logic.) I wouldn't spend too much time investigating.

Also, selecting something on the page and causing garbage could be due to https://bugs.webkit.org/show_bug.cgi?id=55679 if you're running a webkit revision after r80081.