Bug 54315 – [chromium] Update texture for ContentLayerChromiums in draw() call instead of updateContents..() call

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Update texture for ContentLayerChromiums in draw() call instead of updateContents..() call

[James Robinson]

Created attachment 82185 [details]
Patch

[WebKit Review Bot]

Attachment 82185 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7868723

[James Robinson]

Created attachment 82188 [details]
Patch

[Kenneth Russell]

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.

[James Robinson]

Good catch, that is unsafe.  Vangelis pointed out some other issues with this patch offline - I'll try to update this soon.

[James Robinson]

Created attachment 82569 [details]
Patch

[James Robinson]

Created attachment 82572 [details]
Patch

[James Robinson]

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.

[James Robinson]

Created attachment 82573 [details]
add check in unreserveContentsTexture() to match the one in bindContentsTexture()

[WebKit Review Bot]

Attachment 82573 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7920168

[Kenneth Russell]

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.

[James Robinson]

Thanks for looking!  I'll figure out the compile error on mac and update the patch.

[James Robinson]

Created attachment 82730 [details]
fixes mac compile, adds helper for bitmap locking/unlocking

[Vangelis Kokkevis]

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.

[James Robinson]

(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.

[James Robinson]

Created attachment 82866 [details]
Patch

[James Robinson]

Created attachment 82868 [details]
Patch

[James Robinson]

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.

[Kenneth Russell]

Vangelis, can you please do the unofficial review of this patch?

[Vangelis Kokkevis]

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.

[Kenneth Russell]

Comment on attachment 82868 [details]
Patch

Looks good to me.

[James Robinson]

Committed r79033: <http://trac.webkit.org/changeset/79033>

[Antoine Labour]

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.

[James Robinson]

(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.

[Antoine Labour]

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.

[James Robinson]

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.

[Adrienne Walker]

(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.