Bug 44127 – [chromium] Thumbnails not generated for GPU Rendered Pages

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[W. James MacLean]

When running chrome with --enable-accelerated-compositing, page thumbnails are not generated for pages rendered on the GPU.

[W. James MacLean]

Created attachment 64625 [details]
Patch to allow pixel readback from GPU for thumbnail generation.

Patch to allow pixel readback from GPU for thumbnail generation.

[WebKit Review Bot]

Attachment 64625 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:512:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:528:  Missing spaces around /  [whitespace/operators] [3]
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:529:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[W. James MacLean]

Created attachment 64628 [details]
 Patch to allow pixel readback from GPU for thumbnail generation. 

Fixes style errors from previous patch.

[WebKit Review Bot]

Attachment 64628 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:528:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vangelis Kokkevis]

Comment on attachment 64628 [details]
 Patch to allow pixel readback from GPU for thumbnail generation. 

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index ad3ea90..6854b00 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Added functions to allow readback of GPU buffer for generating Chromium thumbnails.
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::readPixels):
> +        * platform/graphics/chromium/LayerRendererChromium.h:
> +
>  2010-08-17  Darin Fisher  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 2f70efa..b73ae86 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -506,6 +506,36 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>      m_needsDisplay = false;
>  }
>  
> +void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
> +{
> +    if (!canvasPtr)
> +        return;
> +
> +    makeContextCurrent();
> +
> +    checkGLError();
> +
> +    const SkBitmap bitmap = canvasPtr->getDevice()->accessBitmap(false);
> +    void* pixels = bitmap.getPixels();
> +
> +    glReadPixels(0, 0, bitmap.width(), bitmap.height(),
> +                 GL_RGBA, GL_UNSIGNED_BYTE, pixels);
> +
> +    checkGLError();
> +
> +    // Flip pixels vertically ...
> +    OwnPtr<unsigned char> lineTemp(new unsigned char[bitmap.rowBytes()]);
> +    for (int row1 = 0, row2 = bitmap.height() - 1; row1 < bitmap.height() / 2; ++row1, --row2){
> +        unsigned char *ptr1 = static_cast<unsigned char *>(pixels) + row1 * bitmap.rowBytes();
> +        unsigned char *ptr2 = static_cast<unsigned char *>(pixels) + row2 * bitmap.rowBytes();
> +
> +        memcpy(lineTemp.get(), ptr1, bitmap.rowBytes());
> +        memcpy(ptr1, ptr2, bitmap.rowBytes());
> +        memcpy(ptr2, lineTemp.get(), bitmap.rowBytes());
> +    }
> +}
> +
> +
>  // Returns the id of the texture currently associated with the layer or
>  // -1 if the id hasn't been registered yet.
>  int LayerRendererChromium::getTextureId(LayerChromium* layer)
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> index e4474b5..0a20ca3 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> @@ -87,6 +87,8 @@ public:
>  
>      GraphicsContext* rootLayerGraphicsContext() const { return m_rootLayerGraphicsContext.get(); }
>  
> +    void readPixels(skia::PlatformCanvas* canvasPtr);
> +
>  private:
>      enum ShaderProgramType { DebugBorderProgram, ScrollLayerProgram, ContentLayerProgram, CanvasLayerProgram, NumShaderProgramTypes };
>  
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index c5b2a5d..c8c919b 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,18 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Added functions to allow readback of GPU buffer for generating Chromium thumbnails.
> +
> +        * public/WebView.h:
> +        (WebKit::WebView::readPixels):
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::readPixels):
> +        * src/WebViewImpl.h:
> +
>  2010-08-17  Sheriff Bot  <[e-mail]>
>  
>          Unreviewed, rolling out r65516.
> diff --git a/WebKit/chromium/public/WebView.h b/WebKit/chromium/public/WebView.h
> index 1b94da2..3433335 100644
> --- a/WebKit/chromium/public/WebView.h
> +++ b/WebKit/chromium/public/WebView.h
> @@ -212,6 +212,8 @@ public:
>      // (accept false) effect.  Return true on success.
>      virtual bool setDropEffect(bool accept) = 0;
>  
> +    virtual void readPixels(WebCanvas *canvas) {}
> +
>  
>      // Support for resource loading initiated by plugins -------------------
>  
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
> index 62b20d5..ff00c9a 100644
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -943,6 +943,12 @@ void WebViewImpl::layout()
>      }
>  }
>  
> +void WebViewImpl::readPixels(WebCanvas *canvas)
> +{
> +    ASSERT(isAcceleratedCompositingActive());
> +    m_layerRenderer->readPixels(canvas);
> +}
> +
>  void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  {
>  
> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> index c296121..9b0906c 100644
> --- a/WebKit/chromium/src/WebViewImpl.h
> +++ b/WebKit/chromium/src/WebViewImpl.h
> @@ -158,6 +158,7 @@ public:
>          const WebPoint& screenPoint);
>      virtual int dragIdentity();
>      virtual bool setDropEffect(bool accept);
> +    virtual void readPixels(WebCanvas *canvas);
>      virtual unsigned long createUniqueIdentifierForRequest();
>      virtual void inspectElementAt(const WebPoint& point);
>      virtual WebString inspectorSettings() const;

WebCore/ChangeLog:8
 +          Added functions to allow readback of GPU buffer for generating Chromium thumbnails.
Usually you would put the above line at the top of the changelog (following [chromium]) and then just provide a link to the bug. No need to repeat the bug description here.

WebKit/chromium/ChangeLog:8
 +          Added functions to allow readback of GPU buffer for generating Chromium thumbnails.
In this changelog entry you can be more specific about the changes you made to the WebKit/chromium side, for example something like "Adding a method to WebViewImpl to read the contents of a composited page"

WebKit/chromium/public/WebView.h:215
 +      virtual void readPixels(WebCanvas *canvas) {}
Does this method need to appear in WebView or just WebViewImpl?  Generally the WebView API is a pure virtual one.

WebKit/chromium/src/WebViewImpl.cpp:948
 +      ASSERT(isAcceleratedCompositingActive());
Is this method only called if acceleratedCompositing is active? It seems that this ASSERT() should be converted to an if () and have a different path if you're not using accelerated compositing.  In either case though, the code needs to be withing a:
#if USE(ACCELERATED_COMPOSITING)
block

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:509
 +  void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
Skia is only used on windows and linux. We'll need a CG path for the mac. A good example of similar code is in: GraphicsContext3DInternal::paintRenderingResultsToCanvas()

WebCore/platform/graphics/chromium/LayerRendererChromium.h:90
 +      void readPixels(skia::PlatformCanvas* canvasPtr);
nit: Use a more descriptive name for this method like: getFramebufferPixels() or something of that sort?

[David Levin]

Comment on attachment 64628 [details]
 Patch to allow pixel readback from GPU for thumbnail generation. 

r- see Vangelis' comment (and a minor style issue in the bug too) plus the style issue below.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:509
 +  void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
Just canvas should suffice.

WebKit avoids abbreviates and in general adding type information to variable names (so Ptr is out on both accounts).

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:526
 +      // Flip pixels vertically ...
"// Flip pixels vertically."
works too. 

This comes from a person who uses "..." to often in the past....


WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:529
 +          unsigned char *ptr1 = static_cast<unsigned char *>(pixels) + row1 * bitmap.rowBytes();
The * (when used as a pointer as opposed to multiplication) should be near the type.

There are lots of instances of this problem in this patch.

WebKit/chromium/public/WebView.h:215
 +      virtual void readPixels(WebCanvas *canvas) {}
The parameter name |canvas| adds no information in the function definition, so it should be omitted. (There are other instances of this.)

Also WebKit tends to put a space inside of braces like this, so s/{}/{ }/

(And the * is in the wrong place.)

[W. James MacLean]

(In reply to comment #5)
> 
> WebKit/chromium/public/WebView.h:215
>  +      virtual void readPixels(WebCanvas *canvas) {}
> Does this method need to appear in WebView or just WebViewImpl?  Generally the WebView API is a pure virtual one.

I put a default implementation in since I wasn't sure what else might be inheriting from WebView ... I've made it pure virtual.
 
> WebKit/chromium/src/WebViewImpl.cpp:948
>  +      ASSERT(isAcceleratedCompositingActive());
> Is this method only called if acceleratedCompositing is active? It seems that this ASSERT() should be converted to an if () and have a different path if you're not using accelerated compositing.  In either case though, the code needs to be withing a:
> #if USE(ACCELERATED_COMPOSITING)
> block

The chromium-side code ensures this method is only called if acceleratedCompositing is active. I can wrap it in #if USE(ACCELERATED_COMPOSITING) no problem, but then should I wrap all my plumbing methods as well?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:509
>  +  void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
> Skia is only used on windows and linux. We'll need a CG path for the mac. A good example of similar code is in: GraphicsContext3DInternal::paintRenderingResultsToCanvas()

OK. To make the interface in LayerRendererChromium as generic as possible I'd like to pass the context as a WebCanvas*, but I can't seem to figure out how to get WebCanvas.h included in LayerRendererChromium.h (file not found). Suggestions?

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:90
>  +      void readPixels(skia::PlatformCanvas* canvasPtr);
> nit: Use a more descriptive name for this method like: getFramebufferPixels() or something of that sort?

Done.

[Vangelis Kokkevis]

(In reply to comment #7)
> (In reply to comment #5)
> > 
> > WebKit/chromium/public/WebView.h:215
> >  +      virtual void readPixels(WebCanvas *canvas) {}
> > Does this method need to appear in WebView or just WebViewImpl?  Generally the WebView API is a pure virtual one.
> 
> I put a default implementation in since I wasn't sure what else might be inheriting from WebView ... I've made it pure virtual.

From the chromium code, do you use a pointer to a WebViewImpl or to a WebView?  If you can please post the chromium patch as well in the chrome bug tracker, I get a better picture of how things are hooked up.

> 
> > WebKit/chromium/src/WebViewImpl.cpp:948
> >  +      ASSERT(isAcceleratedCompositingActive());
> > Is this method only called if acceleratedCompositing is active? It seems that this ASSERT() should be converted to an if () and have a different path if you're not using accelerated compositing.  In either case though, the code needs to be withing a:
> > #if USE(ACCELERATED_COMPOSITING)
> > block
> 
> The chromium-side code ensures this method is only called if acceleratedCompositing is active. I can wrap it in #if USE(ACCELERATED_COMPOSITING) no problem, but then should I wrap all my plumbing methods as well?

A lot of the methods related to accelerated compositing are behind that guard so the code won't compile if ACCELERATED_COMPOSITING is not defined. isAcceleratedCompositingActive() is definitely one of them.
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:509
> >  +  void LayerRendererChromium::readPixels(skia::PlatformCanvas* canvasPtr)
> > Skia is only used on windows and linux. We'll need a CG path for the mac. A good example of similar code is in: GraphicsContext3DInternal::paintRenderingResultsToCanvas()
> 
> OK. To make the interface in LayerRendererChromium as generic as possible I'd like to pass the context as a WebCanvas*, but I can't seem to figure out how to get WebCanvas.h included in LayerRendererChromium.h (file not found). Suggestions?

I take that back. Skia::PlatformCanvas seems to be available on the mac as well as it's apparently used to transfer bits between the renderer and the browser process.  It's worth trying to compile the code on the mac to see how things do.  

> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.h:90
> >  +      void readPixels(skia::PlatformCanvas* canvasPtr);
> > nit: Use a more descriptive name for this method like: getFramebufferPixels() or something of that sort?
> 
> Done.

[W. James MacLean]

Created attachment 64877 [details]
Patch

[WebKit Review Bot]

Attachment 64877 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:961:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vangelis Kokkevis]

Comment on attachment 64877 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index ad3ea90fa97c706ed4ce0ea385d2c93998d925cc..0ad1ee5530eed1141cb54f4fe61aa03a059a9c50 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Replicates existing functionality, use existing tests.
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::getFramebufferPixels):
> +        * platform/graphics/chromium/LayerRendererChromium.h:
> +
>  2010-08-17  Darin Fisher  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 2f70efad02606b6a37a1218f93b6dcb73e1de796..04b69e11dc31dbf6786dc0569b2f3a5e9ade6106 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -506,6 +506,33 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>      m_needsDisplay = false;
>  }
>  
> +void LayerRendererChromium::getFramebufferPixels(void *pixels, const int width, const int height, const int rowBytes)
> +{
> +    if (!pixels)
> +        return;
> +
> +    makeContextCurrent();
> +
> +    checkGLError();
> +
> +    glReadPixels(0, 0, width, height,
> +                 GL_RGBA, GL_UNSIGNED_BYTE, pixels);
> +
> +    checkGLError();
> +
> +    // Flip pixels vertically.
> +    OwnPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
> +    for (int row1 = 0, row2 = height - 1; row1 < height / 2; ++row1, --row2) {
> +
> +        unsigned char* ptr1 = static_cast<unsigned char*>(pixels) + row1 * rowBytes;
> +        unsigned char* ptr2 = static_cast<unsigned char*>(pixels) + row2 * rowBytes;
> +
> +        memcpy(lineTemp.get(), ptr1, rowBytes);
> +        memcpy(ptr1, ptr2, rowBytes);
> +        memcpy(ptr2, lineTemp.get(), rowBytes);
> +    }
> +}
> +
>  // Returns the id of the texture currently associated with the layer or
>  // -1 if the id hasn't been registered yet.
>  int LayerRendererChromium::getTextureId(LayerChromium* layer)
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> index e4474b5a0251d53a073d4b7590a057d9f91b6639..ae9f55cb3e634211d62c30f0103b300278b8782b 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> @@ -87,6 +87,8 @@ public:
>  
>      GraphicsContext* rootLayerGraphicsContext() const { return m_rootLayerGraphicsContext.get(); }
>  
> +    void getFramebufferPixels(void *pixels, const int width, const int height, const int rowBytes);
> +
>  private:
>      enum ShaderProgramType { DebugBorderProgram, ScrollLayerProgram, ContentLayerProgram, CanvasLayerProgram, NumShaderProgramTypes };
>  
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index c5b2a5d467a378d74a06bd2021c0b2f4c9c74173..6c10f07439458543c6d5d4ba7bff181d5997cdc7 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Modified WebViewImpl::paint() to detect non-null canvas pointers when
> +        accelerated compositing is active, and instead fills the pixel buffer
> +        from the GPU framebuffer.
> +
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::paint):
> +
>  2010-08-17  Sheriff Bot  <[e-mail]>
>  
>          Unreviewed, rolling out r65516.
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
> index 62b20d5fb59477df2448b03ba95e883051a19b1d..5d862472afbb86e92aff3b33aa92284de4ba31c0 100644
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -114,6 +114,10 @@
>  #include "WebViewClient.h"
>  #include "wtf/OwnPtr.h"
>  
> +#if WEBKIT_USING_CG
> +#include <CoreGraphics/CGContext.h>
> +#endif
> +
>  #if OS(WINDOWS)
>  #include "RenderThemeChromiumWin.h"
>  #else
> @@ -954,6 +958,35 @@ void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>              webframe->paint(canvas, rect);
>  #if USE(ACCELERATED_COMPOSITING)
>      } else {
> +        // If canvas is non-NULL, we just read the pixels from
> +        // the GPU framebuffer.
> +        if (canvas) {
> +#if WEBKIT_USING_SKIA
> +            const SkBitmap bitmap = canvas->getDevice()->accessBitmap(false);
> +
> +            int width = bitmap.width();
> +            int height = bitmap.height();
> +            int rowBytes = bitmap.rowBytes();
> +
> +            SkAutoLockPixels bitmapLock(bitmap);
> +            void* pixels = bitmap.getPixels();
> +#elif WEBKIT_USING_CG
> +            CGContextRef bitmap = reinterpret_cast<CGContextRef>(canvas);
> +
> +            int width = CGBitmapContextGetWidth(bitmap);
> +            int height = CGBitmapContextGetHeight(bitmap);
> +            int rowBytes = CGBitmapContextGetBytesPerRow(bitmap);
> +
> +            void* pixels = CGBitmapContextGetData(bitmap);
> +            ASSERT(pixels);
> +#else
> +#error Must port to your platform.
> +#endif
> +
> +            m_layerRenderer->getFramebufferPixels(pixels, width, height, rowBytes);
> +            return;
> +        }
> +
>          // Draw the contents of the root layer.
>          updateRootLayerContents(rect);
>  

WebKit/chromium/src/WebViewImpl.cpp:981
 +              ASSERT(pixels);
This ASSERT could move outside the #if clause so that it runs for both Skia and CG

WebKit/chromium/src/WebViewImpl.cpp:963
 +          if (canvas) {
This code will probably need to move after the call to m_layerRenderer->drawLayers() so that we get fresh results. I guess the problem is that drawLayers calls swap buffers so it will be too late. A possible solution would be to split the call to swap buffers out to a different function (say: LayerRendererChromium::present()) and call that from WebViewImpl, after drawLayers and readFramebufferPixels.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:518
 +      glReadPixels(0, 0, width, height,
Are there any guarantees that the width and height passed in match those of the framebuffer? You should probably at a minimum put an assert there and/or clamp to the size of the framebuffer.

[W. James MacLean]

(In reply to comment #11)
> 
> WebKit/chromium/src/WebViewImpl.cpp:981
>  +              ASSERT(pixels);
> This ASSERT could move outside the #if clause so that it runs for both Skia and CG

Sure, done.
 
> WebKit/chromium/src/WebViewImpl.cpp:963
>  +          if (canvas) {
> This code will probably need to move after the call to m_layerRenderer->drawLayers() so that we get fresh results. I guess the problem is that drawLayers calls swap buffers so it will be too late. A possible solution would be to split the call to swap buffers out to a different function (say: LayerRendererChromium::present()) and call that from WebViewImpl, after drawLayers and readFramebufferPixels.

For thumbnail generation, the logic that invokes WebViewImpl::paint() and provides a non-Null canvas is guaranteed to make that call *after* the page has been rendered (at least) once. Thus far I haven't seen any issues with thumbnails coming back blank, and it would be nice to avoid re-rendering the page just to grab the thumbnail pixels.

It's possible to to do something like LayerRendererChromium::present(), but if we're trying to avoid re-rendering in the first place then is it needed?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:518
>  +      glReadPixels(0, 0, width, height,
> Are there any guarantees that the width and height passed in match those of the framebuffer? You should probably at a minimum put an assert there and/or clamp to the size of the framebuffer.

Good idea - should I compare/clamp against m_rootLayerTextureWidth/Height ?

[Vangelis Kokkevis]

(In reply to comment #12)
> (In reply to comment #11)
> > 
> > WebKit/chromium/src/WebViewImpl.cpp:981
> >  +              ASSERT(pixels);
> > This ASSERT could move outside the #if clause so that it runs for both Skia and CG
> 
> Sure, done.
> 
> > WebKit/chromium/src/WebViewImpl.cpp:963
> >  +          if (canvas) {
> > This code will probably need to move after the call to m_layerRenderer->drawLayers() so that we get fresh results. I guess the problem is that drawLayers calls swap buffers so it will be too late. A possible solution would be to split the call to swap buffers out to a different function (say: LayerRendererChromium::present()) and call that from WebViewImpl, after drawLayers and readFramebufferPixels.
> 
> For thumbnail generation, the logic that invokes WebViewImpl::paint() and provides a non-Null canvas is guaranteed to make that call *after* the page has been rendered (at least) once. Thus far I haven't seen any issues with thumbnails coming back blank, and it would be nice to avoid re-rendering the page just to grab the thumbnail pixels.
> 
> It's possible to to do something like LayerRendererChromium::present(), but if we're trying to avoid re-rendering in the first place then is it needed?

I think that since you opted for a more general solution of integrating the readback in the paint method (which is a good thing), you cannot make assumptions on how this method will be called and whether the compositor will have rendered something before.  I think splitting out the swapbuffers call is worth it here, and is a fairly small change.

> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:518
> >  +      glReadPixels(0, 0, width, height,
> > Are there any guarantees that the width and height passed in match those of the framebuffer? You should probably at a minimum put an assert there and/or clamp to the size of the framebuffer.
> 
> Good idea - should I compare/clamp against m_rootLayerTextureWidth/Height ?

yes, those are good values to clamp against.

[W. James MacLean]

(In reply to comment #13)
> 
> I think that since you opted for a more general solution of integrating the readback in the paint method (which is a good thing), you cannot make assumptions on how this method will be called and whether the compositor will have rendered something before.  I think splitting out the swapbuffers call is worth it here, and is a fairly small change.

No problem, I just wanted to be clear about this since we had discussed pure read-back earlier.

I'll implement this today, but as I'm working OOO today I may not be able to test the changes until Monday morning, so will re-submit the patch then.

> > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:518
> > >  +      glReadPixels(0, 0, width, height,
> > > Are there any guarantees that the width and height passed in match those of the framebuffer? You should probably at a minimum put an assert there and/or clamp to the size of the framebuffer.
> > 
> > Good idea - should I compare/clamp against m_rootLayerTextureWidth/Height ?
> 
> yes, those are good values to clamp against.

Done!

[W. James MacLean]

Created attachment 65465 [details]
Patch

[W. James MacLean]

(In reply to comment #15)
> Created an attachment (id=65465) [details]
> Patch

This new patch includes support for mis-match between the size of the current render-layer and the canvas provided to contain the read-back pixels. Includes revisions to merge with recent changes to LayerRendererChromium.[h|cpp].

[Vangelis Kokkevis]

Comment on attachment 65465 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index eb5910eda9a40bf990446c741d3d1a25b1621ae7..58447952d5a92685ee5ff989361a79e6b3cedbd2 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,23 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Replicates existing functionality, use existing tests.
> +
> +        Adds pixel-readback for GPU composited pages to allow for thumbnailing,
> +        printing and other services to work with GPU rendered pages.
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::drawLayers):
> +        (WebCore::LayerRendererChromium::present):
> +        (WebCore::LayerRendererChromium::getFramebufferPixels):
> +        * platform/graphics/chromium/LayerRendererChromium.h:
> +        (WebCore::LayerRendererChromium::getRootLayerTextureWidth):
> +        (WebCore::LayerRendererChromium::getRootLayerTextureHeight):
> +
>  2010-08-25  Ilya Tikhonovsky  <[e-mail]>
>  
>          Reviewed by Yury Semikhatsky.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index cf238718b21bb40bb6976627d9f233854e4469fb..bacc852b0db1cdc3e8fa2e27e722a8cd5f4daf32 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -310,11 +310,41 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>  
>      GLC(glDisable(GL_SCISSOR_TEST));
>  
> +    glFlush();
> +}
> +
> +void LayerRendererChromium::present()
> +{
>      m_gles2Context->swapBuffers();
>  
>      m_needsDisplay = false;
>  }
>  
> +void LayerRendererChromium::getFramebufferPixels(void *pixels, const int width, const int height, const int rowBytes)
> +{
> +    ASSERT(width == m_rootLayerTextureWidth && height == m_rootLayerTextureHeight);
> +
> +    if (!pixels)
> +        return;
> +
> +    makeContextCurrent();
> +
> +    GLC(glReadPixels(0, 0, width, height,
> +                     GL_RGBA, GL_UNSIGNED_BYTE, pixels));
> +
> +    // Flip pixels vertically.
> +    OwnPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
> +    for (int row1 = 0, row2 = height - 1; row1 < height / 2; ++row1, --row2) {
> +
> +        unsigned char* ptr1 = static_cast<unsigned char*>(pixels) + row1 * rowBytes;
> +        unsigned char* ptr2 = static_cast<unsigned char*>(pixels) + row2 * rowBytes;
> +
> +        memcpy(lineTemp.get(), ptr1, rowBytes);
> +        memcpy(ptr1, ptr2, rowBytes);
> +        memcpy(ptr2, lineTemp.get(), rowBytes);
> +    }
> +}
> +
>  // FIXME: This method should eventually be replaced by a proper texture manager.
>  unsigned LayerRendererChromium::createLayerTexture()
>  {
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> index 24bbe652828c1f98a531e2128f6918d1e9e8c363..b5c93871b09f7557f015f2f755e86e11d585505b 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> @@ -64,6 +64,7 @@ public:
>      // Updates the contents of the root layer that fall inside the updateRect and recomposites
>      // all the layers.
>      void drawLayers(const IntRect& updateRect, const IntRect& visibleRect, const IntRect& contentRect, const IntPoint& scrollPosition);
> +    void present(); // Perform buffer swap to present rendered buffer.
>  
>      void setRootLayer(PassRefPtr<LayerChromium> layer) { m_rootLayer = layer; }
>      LayerChromium* rootLayer() { return m_rootLayer.get(); }
> @@ -90,6 +91,10 @@ public:
>      const ContentLayerChromium::SharedValues* contentLayerSharedValues() const { return m_contentLayerSharedValues.get(); }
>      const CanvasLayerChromium::SharedValues* canvasLayerSharedValues() const { return m_canvasLayerSharedValues.get(); }
>  
> +    int getRootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
> +    int getRootLayerTextureHeight() const { return m_rootLayerTextureHeight; }
> +    void getFramebufferPixels(void *pixels, const int width, const int height, const int rowBytes);
> +
>  private:
>      void updateLayersRecursive(LayerChromium* layer, const TransformationMatrix& parentMatrix, float opacity);
>  
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 5d860ed71441c401246d8dfc17723e5ebe9bfa41..6c6942c16cdb56a607b1fb424e900864e8aa92bc 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,18 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Modified WebViewImpl::paint() to detect non-null canvas pointers when
> +        accelerated compositing is active, and instead fills the pixel buffer
> +        from the GPU framebuffer. Includes re-scaling support when provided
> +        canvas does not match size of current render layer.
> +
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::paint):
> +
>  2010-08-25  Satish Sampath  <[e-mail]>
>  
>          Reviewed by Jeremy Orlow.
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
> index 383b7167314edaf342daeb48f3b56610ed753148..4bd56c09910108fb626c068845c8add40249cf87 100644
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -115,6 +115,10 @@
>  #include "WebViewClient.h"
>  #include "wtf/OwnPtr.h"
>  
> +#if WEBKIT_USING_CG
> +#include <CoreGraphics/CGContext.h>
> +#endif
> +
>  #if OS(WINDOWS)
>  #include "RenderThemeChromiumWin.h"
>  #else
> @@ -955,6 +959,7 @@ void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>              webframe->paint(canvas, rect);
>  #if USE(ACCELERATED_COMPOSITING)
>      } else {
> +
>          // Draw the contents of the root layer.
>          updateRootLayerContents(rect);
>  
> @@ -971,6 +976,78 @@ void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  
>          // Ask the layer compositor to redraw all the layers.
>          m_layerRenderer->drawLayers(rect, visibleRect, contentRect, IntPoint(view->scrollX(), view->scrollY()));
> +
> +        // If a canvas was passed in, we use it to grab a copy of the
> +        // freshly-rendered pixels.
> +        if (canvas) {
> +            void* pixels = 0;
> +#if WEBKIT_USING_SKIA
> +            const SkBitmap bitmap = canvas->getDevice()->accessBitmap(false);
> +
> +            int width = bitmap.width();
> +            int height = bitmap.height();
> +            int rowBytes = bitmap.rowBytes();
> +
> +            SkAutoLockPixels bitmapLock(bitmap);
> +
> +            if (m_layerRenderer->getRootLayerTextureWidth() == width
> +                && m_layerRenderer->getRootLayerTextureHeight() == height) {
> +                pixels = bitmap.getPixels();
> +                m_layerRenderer->getFramebufferPixels(pixels, width, height, rowBytes);
> +            } else {
> +                width = m_layerRenderer->getRootLayerTextureWidth();
> +                height = m_layerRenderer->getRootLayerTextureHeight();
> +
> +                // Create temp bitmap of correct size to copy pixels into.
> +                OwnPtr<skia::PlatformCanvas> canvas2 = OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
> +                if (canvas2.get() && canvas2->initialize(width, height, true)) {
> +                    SkBitmap bitmap2 = canvas2->getDevice()->accessBitmap(false);
> +                    pixels = bitmap2.getPixels();
> +                    m_layerRenderer->getFramebufferPixels(pixels, width, height, rowBytes);
> +                    canvas->drawBitmap(bitmap2, 0, 0, 0);
> +                }
> +            }
> +#elif WEBKIT_USING_CG
> +            CGContextRef bitmap = reinterpret_cast<CGContextRef>(canvas);
> +
> +            int width = CGBitmapContextGetWidth(bitmap);
> +            int height = CGBitmapContextGetHeight(bitmap);
> +            int rowBytes = CGBitmapContextGetBytesPerRow(bitmap);
> +
> +            if (m_layerRenderer->getRootLayerTextureWidth() == width
> +                && m_layerRenderer->getRootLayerTextureHeight() == height) {
> +              pixels = CGBitmapContextGetData(bitmap);
> +              m_layerRenderer->getFramebufferPixels(pixels, width, height, rowBytes);
> +            } else {
> +                width = m_layerRenderer->getRootLayerTextureWidth();
> +                height = m_layerRenderer->getRootLayerTextureHeight();
> +
> +                // Create temp bitmap of same size as rendered layer to copy pixels into.
> +                CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> +                CGContextRef bitmap2 = CGBitmapContextCreate(0, width, height, 8, 4 * width, colorSpace, 
> +                                                             kCGImageAlphaPremultipliedLast);
> +                if (bitmap2) {
> +                  pixels = CGBitmapContextGetData(bitmap2);
> +                  m_layerRenderer->getFramebufferPixels(pixels, width, height, rowBytes);
> +
> +                  // Copy bitmap back to input bitmap. The image is inverted according to CG,
> +                  // so set up the appropriate transform to invert vertical axis and move origin
> +                  // to bottom left.
> +                  CGContextSaveGState(bitmap);
> +                  CGContextTranslateCTM(bitmap, 0, CGBitmapContextGetHeight(bitmap));
> +                  CGContextScaleCTM(bitmap, 1.0, -1.0);
> +                  CGContextDrawImage(bitmap,
> +                                     CGRectMake(0, 0, CGBitmapContextGetWidth(bitmap), CGBitmapContextGetHeight(bitmap)),
> +                                     CGBitmapContextCreateImage(bitmap2));
> +                  CGContextRestoreGState(bitmap);
> +                }
> +            }
> +#else
> +#error Must port to your platform.
> +#endif
> +        }
> +
> +        m_layerRenderer->present(); // Do final display by swapping buffers.
>      }
>  #endif
>  }

WebCore/platform/graphics/chromium/LayerRendererChromium.h:94
 +      int getRootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
Accessors don't typically start with get.  These two should be: rootLayerTextureWidth() and rootLayerTextureHeight()

WebKit/chromium/src/WebViewImpl.cpp:118
 +  #if WEBKIT_USING_CG
The WebCore code has been using
#if PLATFORM(CG) instead of WEBKIT_USING_CG

WebKit/chromium/src/WebViewImpl.cpp:984
 +  #if WEBKIT_USING_SKIA
#if PLATFORM(SKIA)

WebKit/chromium/src/WebViewImpl.cpp:989
 +              int rowBytes = bitmap.rowBytes();
In getFramebufferPixels() we assume that we have 4 bytes per pixel (RGBA).  If rowBytes doesn't match 4*width then we're in trouble.  Maybe better here to do an:

ASSERT(bitmap.config() == SkBitmap::kARGB_8888_Config)

and not pass the rowBytes down to getFrameBufferPixels.

WebKit/chromium/src/WebViewImpl.cpp:1002
 +                  OwnPtr<skia::PlatformCanvas> canvas2 = OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
Please use a more descriptive name for canvas2 and bitmap2

WebKit/chromium/src/WebViewImpl.cpp:1016
 +  
ASSERT(rowBytes == width * 4) 

WebKit/chromium/src/WebViewImpl.cpp:1007
 +                      canvas->drawBitmap(bitmap2, 0, 0, 0);
I think you want to call drawBitmapRect here to specify that the target size if the size of your WebCanvas so that you get the scaling.

WebKit/chromium/src/WebViewImpl.cpp:999
 +                  height = m_layerRenderer->getRootLayerTextureHeight();
these variables shadow the width and height defined in the outside scope and make things confusing (plus I think you need the canvas width further down anyway).  Please rename.

WebKit/chromium/src/WebViewImpl.cpp:1010
 +  #elif WEBKIT_USING_CG
#if PLATFORM(CG)

[W. James MacLean]

(In reply to comment #17)
> (From update of attachment 65465 [details])
> 
> WebCore/platform/graphics/chromium/LayerRendererChromium.h:94
>  +      int getRootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
> Accessors don't typically start with get.  These two should be: rootLayerTextureWidth() and rootLayerTextureHeight()

Done.
 
> WebKit/chromium/src/WebViewImpl.cpp:118
>  +  #if WEBKIT_USING_CG
> The WebCore code has been using
> #if PLATFORM(CG) instead of WEBKIT_USING_CG
> 
> WebKit/chromium/src/WebViewImpl.cpp:984
>  +  #if WEBKIT_USING_SKIA
> #if PLATFORM(SKIA)

I tried this initially, but it didn't work for me ... PLATFORM(SKIA) was true during compilation on the Mac, meaning the CG code didn't get compiled when it should have. This is why I was using WEBKIT_USING_SKIA and WEBKIT_USING_CG.

Any suggestions about what I may have been doing wrong in my use of "#if PLATFORM(SKIA)"?

> WebKit/chromium/src/WebViewImpl.cpp:989
>  +              int rowBytes = bitmap.rowBytes();
> In getFramebufferPixels() we assume that we have 4 bytes per pixel (RGBA).  If rowBytes doesn't match 4*width then we're in trouble.  Maybe better here to do an:
> 
> ASSERT(bitmap.config() == SkBitmap::kARGB_8888_Config)

Done.

> and not pass the rowBytes down to getFrameBufferPixels.

Done.
 
> WebKit/chromium/src/WebViewImpl.cpp:1002
>  +                  OwnPtr<skia::PlatformCanvas> canvas2 = OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
> Please use a more descriptive name for canvas2 and bitmap2

Done - now canvasResize and bitmapResize

> WebKit/chromium/src/WebViewImpl.cpp:1016
>  +  
> ASSERT(rowBytes == width * 4) 

Done.

> WebKit/chromium/src/WebViewImpl.cpp:1007
>  +                      canvas->drawBitmap(bitmap2, 0, 0, 0);
> I think you want to call drawBitmapRect here to specify that the target size if the size of your WebCanvas so that you get the scaling.

Done.

> WebKit/chromium/src/WebViewImpl.cpp:999
>  +                  height = m_layerRenderer->getRootLayerTextureHeight();
> these variables shadow the width and height defined in the outside scope and make things confusing (plus I think you need the canvas width further down anyway).  Please rename.

Done - widthResize, heightResize

> WebKit/chromium/src/WebViewImpl.cpp:1010
>  +  #elif WEBKIT_USING_CG
> #if PLATFORM(CG)

See comments above.

[W. James MacLean]

Created attachment 65918 [details]
Patch

[Vangelis Kokkevis]

Looks good! Just one small comment on the bitmap format test and otherwise should be ready to go.

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 65465 [details] [details])
> > 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.h:94
> >  +      int getRootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
> > Accessors don't typically start with get.  These two should be: rootLayerTextureWidth() and rootLayerTextureHeight()
> 
> Done.
> 
> > WebKit/chromium/src/WebViewImpl.cpp:118
> >  +  #if WEBKIT_USING_CG
> > The WebCore code has been using
> > #if PLATFORM(CG) instead of WEBKIT_USING_CG
> > 
> > WebKit/chromium/src/WebViewImpl.cpp:984
> >  +  #if WEBKIT_USING_SKIA
> > #if PLATFORM(SKIA)
> 
> I tried this initially, but it didn't work for me ... PLATFORM(SKIA) was true during compilation on the Mac, meaning the CG code didn't get compiled when it should have. This is why I was using WEBKIT_USING_SKIA and WEBKIT_USING_CG.
> 
> Any suggestions about what I may have been doing wrong in my use of "#if PLATFORM(SKIA)"?

Hmm, is this still an issue? I see that in your patch you're using the PLATFORM() test. As far as I can tell, PLATFORM(SKIA/CG) expands out to WTF_PLATFORM_SKIA/CG . These two are defined in Platform.h, with WTF_PLATFORM_SKIA only defined if OS(DARWIN). 

> 
> > WebKit/chromium/src/WebViewImpl.cpp:989
> >  +              int rowBytes = bitmap.rowBytes();
> > In getFramebufferPixels() we assume that we have 4 bytes per pixel (RGBA).  If rowBytes doesn't match 4*width then we're in trouble.  Maybe better here to do an:
> > 
> > ASSERT(bitmap.config() == SkBitmap::kARGB_8888_Config)
> 
> Done.
> 

We probably need to be a bit more defensive here and bail out if the config is not what we expect, otherwise we'll end up with memory corruption. What about:

if (bitmap.config() == SkBitmap::kARGB_8888_Config) {

// do the stuff
 pixels = blah;

} else {
 ASSERT_NOT_REACHED();
}

And similarly for the mac code.



> > and not pass the rowBytes down to getFrameBufferPixels.
> 
> Done.
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1002
> >  +                  OwnPtr<skia::PlatformCanvas> canvas2 = OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
> > Please use a more descriptive name for canvas2 and bitmap2
> 
> Done - now canvasResize and bitmapResize
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1016
> >  +  
> > ASSERT(rowBytes == width * 4) 
> 
> Done.
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1007
> >  +                      canvas->drawBitmap(bitmap2, 0, 0, 0);
> > I think you want to call drawBitmapRect here to specify that the target size if the size of your WebCanvas so that you get the scaling.
> 
> Done.
> 
> > WebKit/chromium/src/WebViewImpl.cpp:999
> >  +                  height = m_layerRenderer->getRootLayerTextureHeight();
> > these variables shadow the width and height defined in the outside scope and make things confusing (plus I think you need the canvas width further down anyway).  Please rename.
> 
> Done - widthResize, heightResize
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1010
> >  +  #elif WEBKIT_USING_CG
> > #if PLATFORM(CG)
> 
> See comments above.

[W. James MacLean]

(In reply to comment #20)
> Looks good! Just one small comment on the bitmap format test and otherwise should be ready to go.
> 
> Hmm, is this still an issue? I see that in your patch you're using the PLATFORM() test. As far as I can tell, PLATFORM(SKIA/CG) expands out to WTF_PLATFORM_SKIA/CG . These two are defined in Platform.h, with WTF_PLATFORM_SKIA only defined if OS(DARWIN). 
>

No, it seems fine. I'm not sure why I was having difficulty with it before, but it seems to be OK now.

> We probably need to be a bit more defensive here and bail out if the config is not what we expect, otherwise we'll end up with memory corruption. What about:
> 
> if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
> 
> // do the stuff
>  pixels = blah;
> 
> } else {
>  ASSERT_NOT_REACHED();
> }
> 
> And similarly for the mac code.

Done.

[W. James MacLean]

Created attachment 65932 [details]
Patch

[Vangelis Kokkevis]

(In reply to comment #21)
> (In reply to comment #20)
> > Looks good! Just one small comment on the bitmap format test and otherwise should be ready to go.
> > 
> > Hmm, is this still an issue? I see that in your patch you're using the PLATFORM() test. As far as I can tell, PLATFORM(SKIA/CG) expands out to WTF_PLATFORM_SKIA/CG . These two are defined in Platform.h, with WTF_PLATFORM_SKIA only defined if OS(DARWIN). 
> >
> 
> No, it seems fine. I'm not sure why I was having difficulty with it before, but it seems to be OK now.
> 
> > We probably need to be a bit more defensive here and bail out if the config is not what we expect, otherwise we'll end up with memory corruption. What about:
> > 
> > if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
> > 
> > // do the stuff
> >  pixels = blah;
> > 
> > } else {
> >  ASSERT_NOT_REACHED();
> > }
> > 
> > And similarly for the mac code.
> 
> Done.

Sorry, one last thing that just occurred to me: alongside with the ASSERT_NOT_REACHED() we should also clear out the canvas we get passed in so that we don't end up displaying garbage. Painting it white should be sufficient.

[W. James MacLean]

(In reply to comment #23)
> 
> Sorry, one last thing that just occurred to me: alongside with the ASSERT_NOT_REACHED() we should also clear out the canvas we get passed in so that we don't end up displaying garbage. Painting it white should be sufficient.

No problem ... is there a standard way (function call) for doing this?

[W. James MacLean]

(In reply to comment #24)
> 
> No problem ... is there a standard way (function call) for doing this?

I'll try SkBitmap::eraseColor(SkColor c), and see if I can find something similar for CG.

[W. James MacLean]

Created attachment 65943 [details]
Patch

[W. James MacLean]

Created attachment 65945 [details]
Patch

[W. James MacLean]

(In reply to comment #27)
> Created an attachment (id=65945) [details]
> Patch

Just to confirm: is it OK for me to use CGContextSetRGBFillColor() on a CG bitmap if it is not 32-bit RGBA?

[Vangelis Kokkevis]

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=65945) [details] [details]
> > Patch
> 
> Just to confirm: is it OK for me to use CGContextSetRGBFillColor() on a CG bitmap if it is not 32-bit RGBA?

That's a good question. I couldn't tell from the CG docs whether this will work. Another option would be to use: CGContextClearRect and in the Skia case paint to transparent black to match.

[W. James MacLean]

Created attachment 66041 [details]
Patch

[W. James MacLean]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Created an attachment (id=65945) [details] [details] [details]
> > > Patch
> > 
> > Just to confirm: is it OK for me to use CGContextSetRGBFillColor() on a CG bitmap if it is not 32-bit RGBA?
> 
> That's a good question. I couldn't tell from the CG docs whether this will work. Another option would be to use: CGContextClearRect and in the Skia case paint to transparent black to match.

I thought the CG docs was a bit unclear on this too.

I've changed the code as you suggested, and uploaded a fresh patch.

[Vangelis Kokkevis]

Comment on attachment 66041 [details]
Patch

Looks good!

[Darin Fisher (:fishd, Google)]

Comment on attachment 66041 [details]
Patch

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:337
> +    OwnPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
nit: use OwnArrayPtr here

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
> +
nit: remove this extra blank line

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:94
> +    int rootLayerTextureWidth() const { return m_rootLayerTextureWidth; }
nit: this should probably return IntSize:

  IntSize rootLayerTextureSize() const { ... }

There was a thread recently on webkit-dev about how we should be using IntSize,
IntPoint, etc. more to help clean up the codebase.

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:96
> +    void getFramebufferPixels(void *pixels, const int width, const int height);
nit: can this function take an IntSize parameter?

  void getFramebufferPixels(void* pixels, const IntSize&);

> WebKit/chromium/src/WebViewImpl.cpp:982
> +        if (canvas) {
nit: this function is now clearly way too long.  please break it up into
helper functions: one for SKIA and one for CG.

> WebKit/chromium/src/WebViewImpl.cpp:987
> +
nit: remove this blank line

> WebKit/chromium/src/WebViewImpl.cpp:1002
> +                    OwnPtr<skia::PlatformCanvas> canvasResize = OwnPtr<skia::PlatformCanvas>(new skia::PlatformCanvas());
why does this PlatformCanvas need to be heap allocated?

> WebKit/chromium/src/WebViewImpl.cpp:1010
> +                        canvas->drawBitmapRect(bitmapResize, &srcRect, dstRect, 0);
in ImageSkia.cpp, i see similar code, but it tends to enable filtering
when doing linear re-sampling.

have you considered replacing all of this code with some code that
uses GraphicsContext and BitmapImage?  save for the GraphicsContext
construction, i bet you could write some portable code for this if
you used those classes.  WebFrameImpl::paint shows how to setup the
GraphicsContext.

One more comment:  is it really best to implement this using HW readback
instead of just engaging the software painting path?

[W. James MacLean]

(In reply to comment #33)

I'll 

> have you considered replacing all of this code with some code that
> uses GraphicsContext and BitmapImage?  save for the GraphicsContext
> construction, i bet you could write some portable code for this if
> you used those classes.  WebFrameImpl::paint shows how to setup the
> GraphicsContext.

I can look into this, assuming the we agree the answer to the next question is 'yes'.

> One more comment:  is it really best to implement this using HW readback
> instead of just engaging the software painting path?

I would assume that using both the software and hardware paths is more resource intensive than using a single path, which in this case is (by definition) hardware. Secondly, presumably since we are using HW to speed things up, then this would also be desirable for thumbnail generation to also be accelerated (especially if the page is dynamic, and may need to be rendered repeatedly, meaning thumbnail generation could slow down subsequent page renders).

Finally, I don't know if it makes the controlling logic more complicated to have two different paths for rendering the same page, but I suspect it does.

Vangelis may have some additional thoughts here.

If you agree that HW readback is worthwhile for thumbnail generation, then I can proceed to look into the remaining issues.

[Darin Fisher (:fishd, Google)]

(In reply to comment #34)
> I would assume that using both the software and hardware paths is more resource intensive than using a single path, which in this case is (by definition) hardware. Secondly, presumably since we are using HW to speed things up, then this would also be desirable for thumbnail generation to also be accelerated (especially if the page is dynamic, and may need to be rendered repeatedly, meaning thumbnail generation could slow down subsequent page renders).
> 
> Finally, I don't know if it makes the controlling logic more complicated to have two different paths for rendering the same page, but I suspect it does.
> 
> Vangelis may have some additional thoughts here.
> 
> If you agree that HW readback is worthwhile for thumbnail generation, then I can proceed to look into the remaining issues.

Hmm, yeah upon reflection I agree that it seems like HW readback is the right answer.

[Vangelis Kokkevis]

(In reply to comment #35)
> (In reply to comment #34)
> > I would assume that using both the software and hardware paths is more resource intensive than using a single path, which in this case is (by definition) hardware. Secondly, presumably since we are using HW to speed things up, then this would also be desirable for thumbnail generation to also be accelerated (especially if the page is dynamic, and may need to be rendered repeatedly, meaning thumbnail generation could slow down subsequent page renders).
> > 
> > Finally, I don't know if it makes the controlling logic more complicated to have two different paths for rendering the same page, but I suspect it does.
> > 
> > Vangelis may have some additional thoughts here.
> > 
> > If you agree that HW readback is worthwhile for thumbnail generation, then I can proceed to look into the remaining issues.
> 
> Hmm, yeah upon reflection I agree that it seems like HW readback is the right answer.

Without h/w readback we'll be unable to capture the contents of WebGL, accelerated Canvas2D, and 3D CSS-transformed layers. So I don't think software is an option here.

[W. James MacLean]

(In reply to comment #33)
> 
> have you considered replacing all of this code with some code that
> uses GraphicsContext and BitmapImage?  save for the GraphicsContext
> construction, i bet you could write some portable code for this if
> you used those classes.  WebFrameImpl::paint shows how to setup the
> GraphicsContext.

I've looked through the WebCore/platform/graphics code, and I think I've identified a way to make the code more portable. The initial creation of a GraphicsContext requires some #if's, and it is slightly less efficient as it seems it's hard to get access to the input canvas pixels in a platform-agnostic way, necessitating an intermediary buffer (but perhaps I'm missing something ...). Any implementation will include appropriate use of IntSize.

Here's a tentative sketch of how this might work (but I'm looking for advice here, as there may be better ways to achieve this):

1) Create a GraphicsContext as follows:

#if PLATFORM(CG)
    GraphicsContext gc(canvas);
#elif PLATFORM(SKIA)
    PlatformContextSkia context(canvas);

    // PlatformGraphicsContext is actually a pointer to PlatformContextSkia
    GraphicsContext gc(reinterpret_cast<PlatformGraphicsContext*>(&context));
#else
    notImplemented();
#endif

Not platform independent, but it's small.

2) Create an ImageBuffer object (ARGB) of the same size as the rootTextureLayer (this requires us to always allocate another buffer ... potentially wasteful, but avoids checking for mis-match in canvas size)

ImageBuffer imageBuffer();

3) From the ImageBuffer object, extract pointer to underlying pixels:

imageBuffer.getUnmultipliedImageData(rootLayerRect)->data().get(0).data() 

- returns pointer to unsigned char*
- presumably getUnmultipliedImageData() doesn't mind us modifying the underlying pixels ...

4) Pass pointer to getFrameBufferPixels.

5) Draw ImageBuffer object into gc using drawImageBuffer()

- Not sure about what sort of re-scaling (if any) is available here ...
- I assume this would handle canvas-size-mismatch transparently, but needs verification ...

6) If any of this fails, call gc.clearRect(rootLayerRect);

Questions: 

There seems no obvious way to get the 'size' of a GraphicsContext ... is it assumed to be an infinite (but possibly clipped to some underlying bitmap) drawing plane?

[Darin Fisher (:fishd, Google)]

> Without h/w readback we'll be unable to capture the contents of WebGL, 
> accelerated Canvas2D, and 3D CSS-transformed layers. So I don't think software 
> is an option here.

d'oh, of course!


> Not platform independent, but it's small.

Yeah, that's basically what I was hoping for.


> 2) Create an ImageBuffer object (ARGB) of the same size as the rootTextureLayer 
> (this requires us to always allocate another buffer ... potentially wasteful, 
> but avoids checking for mis-match in canvas size)

If the result is less efficient, then it probably isn't worth it.  I was hoping
it would be possible to minimize platform dependent code without any change in
functionality.  If that doesn't look possible, then going with platform specific
code sounds better... just try to break up the function so that it is not so long.


> There seems no obvious way to get the 'size' of a GraphicsContext ... is it 
> assumed to be an infinite (but possibly clipped to some underlying bitmap) 
> drawing plane?

Yes.

[Vangelis Kokkevis]

(In reply to comment #37)
> (In reply to comment #33)
> > 
> > have you considered replacing all of this code with some code that
> > uses GraphicsContext and BitmapImage?  save for the GraphicsContext
> > construction, i bet you could write some portable code for this if
> > you used those classes.  WebFrameImpl::paint shows how to setup the
> > GraphicsContext.
> 
> I've looked through the WebCore/platform/graphics code, and I think I've identified a way to make the code more portable. The initial creation of a GraphicsContext requires some #if's, and it is slightly less efficient as it seems it's hard to get access to the input canvas pixels in a platform-agnostic way, necessitating an intermediary buffer (but perhaps I'm missing something ...). Any implementation will include appropriate use of IntSize.
> 
> Here's a tentative sketch of how this might work (but I'm looking for advice here, as there may be better ways to achieve this):
> 
> 1) Create a GraphicsContext as follows:
> 
> #if PLATFORM(CG)
>     GraphicsContext gc(canvas);
> #elif PLATFORM(SKIA)
>     PlatformContextSkia context(canvas);
> 
>     // PlatformGraphicsContext is actually a pointer to PlatformContextSkia
>     GraphicsContext gc(reinterpret_cast<PlatformGraphicsContext*>(&context));
> #else
>     notImplemented();
> #endif
> 
> Not platform independent, but it's small.
> 
> 2) Create an ImageBuffer object (ARGB) of the same size as the rootTextureLayer (this requires us to always allocate another buffer ... potentially wasteful, but avoids checking for mis-match in canvas size)
> 
> ImageBuffer imageBuffer();
> 
> 3) From the ImageBuffer object, extract pointer to underlying pixels:
> 
> imageBuffer.getUnmultipliedImageData(rootLayerRect)->data().get(0).data()
> 
> - returns pointer to unsigned char*
> - presumably getUnmultipliedImageData() doesn't mind us modifying the underlying pixels ...

Unfortunately this seems to be an output-only method.  You don't get a pointer to the actual pixels in the ImageBuffer.  I've been looking around at the code to see if there's any other way to poke new pixels into an ImageBuffer and the only way seems to be to create a GraphicsContext around it which is essentially the code you've had up to now.
 

> 
> 4) Pass pointer to getFrameBufferPixels.
> 
> 5) Draw ImageBuffer object into gc using drawImageBuffer()
> 
> - Not sure about what sort of re-scaling (if any) is available here ...
> - I assume this would handle canvas-size-mismatch transparently, but needs verification ...
> 
> 6) If any of this fails, call gc.clearRect(rootLayerRect);
> 
> Questions: 
> 
> There seems no obvious way to get the 'size' of a GraphicsContext ... is it assumed to be an infinite (but possibly clipped to some underlying bitmap) drawing plane?


One more issue which unfortunately I didn't notice in my previous reviews: The code doesn't currently respect the rect that gets passed into WebViewImpl::paint() but rather assumes you're grabbing the entire visible area. I suspect that thumbnails don't contain scroll bars (although I could be wrong) but more generally, we should be take into account the rect we're asked to grab.

I think the change should be straightforward: The intermediate canvas should be allocated to be the size of the passed rect and readPixels should only read the designated portion of the back buffer.

[W. James MacLean]

(In reply to comment #39)

> Unfortunately this seems to be an output-only method.  You don't get a pointer to the actual pixels in the ImageBuffer.  I've been looking around at the code to see if there's any other way to poke new pixels into an ImageBuffer and the only way seems to be to create a GraphicsContext around it which is essentially the code you've had up to now.

OK, I've already started the revisions based on this idea.

> One more issue which unfortunately I didn't notice in my previous reviews: The code doesn't currently respect the rect that gets passed into WebViewImpl::paint() but rather assumes you're grabbing the entire visible area. I suspect that thumbnails don't contain scroll bars (although I could be wrong) but more generally, we should be take into account the rect we're asked to grab.
> 
> I think the change should be straightforward: The intermediate canvas should be allocated to be the size of the passed rect and readPixels should only read the designated portion of the back buffer.

I guess if the input rect is the same size as the canvas we're OK (e.g. no scroll bars visible), but if it's smaller then we allocate an intermediate buffer.

[W. James MacLean]

(In reply to comment #39)
>area. I suspect that thumbnails don't contain scroll bars (although I could be wrong) but more generally, we should be take into account the rect we're asked to grab.

I was just looking at my thumbnail output, and it seems (for thumbnails at least) that the scroll bars are already being clipped. Perhaps the thumbnailer takes scroll bars into account when it creates the WebCanvas bitmap?

What would we do if rect was ever bigger than the canvas' bitmap? Clip it to the bitmap size? 

I'll play around with this shortly, once I fix my broken repository ...

[W. James MacLean]

(In reply to comment #41)

Does this seem reasonable?

My thinking is this: the WebRect passed to WebViewImpl::paint() has x, y, width, height. Since x, y may be non-zero, we need to consider whether 

(x1,y1) = (x+width-1, y+height-1) // location of corner of rect

satisfies

x1 < rootTextureLayerSize.width()
y1 < rootTextureLayerSize.height()

So we do 

x1 = min(x1, rootTextureLayerSize.width() - 1);
y1 = min(y1, rootTextureLayerSize.height() - 1);

We now create resizeRect(x,y,x1-x+1,y1-y+1) [i.e. we clip rect to the rootTextureLayer] and a readback buffer of size (x1+1,y1+1). If resizeRect.width == (0, 0, bitmap.width(), bitmap.height()) then we can readback directly to the existing bitmap pixel buffer and not allocate a secondary buffer, but otherwise we allocate a temporary buffer.

If we allocate a temp buffer, we read back the pixels, and draw them into the bitmap using

srcRect = resizeRect
dstRect = (0, 0, bitmap.width(), bitmap.height()) 

with re-scaling enabled.

resizeRect can be computed in paint() in a platform-independent way, and passed to the platform-specific code (bitmap rects can only be determined in the platform-specific code sections).

[W. James MacLean]

Created attachment 66402 [details]
Patch

[W. James MacLean]

(In reply to comment #42)

I've uploaded a patch related to Darin's requests, and that takes into account the rect passed in to paint(). I've tried it on Linux (seems fine, but not sure how to test out the non-zer0 rect.x & rect.y case in full-blown chrome), but have so far been unable to try it on Mac due to tree-brokenness (my repo's busted).

Still, I'd appreciate feedback on what's there so far. The rect management is based on my proposal in comment #42.

[Vangelis Kokkevis]

Comment on attachment 66402 [details]
Patch

>  
> +void LayerRendererChromium::getFramebufferPixels(void *pixels, const IntSize& size)

This method should take an IntRect as it shouldn't always assume that we're reading starting from (0,0)

> +{
> +    ASSERT(size == rootLayerTextureSize());
> +
> +    if (!pixels)
> +        return;
> +
> +    makeContextCurrent();
> +
> +    GLC(glReadPixels(0, 0, size.width(), size.height(),
> +                     GL_RGBA, GL_UNSIGNED_BYTE, pixels));
> +
> +    // Flip pixels vertically.
> +    const int rowBytes = 4 * size.width();
> +    OwnArrayPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
> +    for (int row1 = 0, row2 = size.height() - 1; row1 < size.height() / 2; ++row1, --row2) {
> +        unsigned char* ptr1 = static_cast<unsigned char*>(pixels) + row1 * rowBytes;
> +        unsigned char* ptr2 = static_cast<unsigned char*>(pixels) + row2 * rowBytes;
> +
> +        memcpy(lineTemp.get(), ptr1, rowBytes);
> +        memcpy(ptr1, ptr2, rowBytes);
> +        memcpy(ptr2, lineTemp.get(), rowBytes);
> +    }
> +}
> +




> +
> +        // If a canvas was passed in, we use it to grab a copy of the
> +        // freshly-rendered pixels.
> +        if (canvas) {
> +            // Clip rect to the confines of the rootLayerTexture.
> +            int xMax = min(rect.x + rect.width - 1, m_layerRenderer->rootLayerTextureSize().width() - 1);
> +            int yMax = min(rect.y + rect.height - 1, m_layerRenderer->rootLayerTextureSize().height() - 1);
> +            WebRect resizeRect(rect.x, rect.y, xMax - rect.x + 1, yMax - rect.y + 1);

A cleaner way to do this would be to create an IntRect out of WebRect and call IntRect::intersect() with the rect of rootLayerTextureSize.

> +            doPixelReadbackToCanvas(canvas, resizeRect);
> +        }
> +
> +        m_layerRenderer->present(); // Do final display by swapping buffers.
>      }
>  #endif
>  }
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +#if PLATFORM(SKIA)
> +static inline void clearSkBitmap(const SkBitmap& bitmap)
> +{
> +    bitmap.eraseColor(SkColorSetARGB(0, 0, 0, 0));
> +}
> +
> +void WebViewImpl::doPixelReadbackToCanvas(WebCanvas* canvas, WebRect& rect)
> +{
> +    ASSERT((rect.x + rect.width -1) < m_layerRenderer->rootLayerTextureSize().width()
> +           && (rect.y + rect.height -1) < m_layerRenderer->rootLayerTextureSize().height());

If you continue using IntRect, you can call IntRect::right and IntRect::bottom instead of doing the math. But I think what you really want to 
ASSERT is rect.intersect(rootLayerRect) == rect

> +
> +    void* pixels = 0;
> +    const SkBitmap bitmap = canvas->getDevice()->accessBitmap(false);
> +    if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
> +        WebRect bitmapRect(0, 0, bitmap.width(), bitmap.height());
> +        IntSize size(bitmapRect.width, bitmapRect.height);
> +
> +        SkAutoLockPixels bitmapLock(bitmap);
> +
> +        if (rect == bitmapRect) {
> +            pixels = bitmap.getPixels();
> +            m_layerRenderer->getFramebufferPixels(pixels, size);
> +        } else {
> +            IntSize resize(rect.x + rect.width, rect.y + rect.height);
> +
> +            // Create temp bitmap of correct size to copy pixels into.
> +            skia::PlatformCanvas canvasResize;
> +            if (canvasResize.initialize(resize.width(), resize.height(), true)) {
> +                SkBitmap bitmapResize = canvasResize.getDevice()->accessBitmap(false);
> +                pixels = bitmapResize.getPixels();
> +                m_layerRenderer->getFramebufferPixels(pixels, resize);

It seems like you're always reading from (0,0) up to the width you need.  Ideally you want to read only the rect you were asked to render.  Alternatively, you can eliminate a lot
of the trimming logic and always read the entire backbuffer and then use the srcRect in drawBitmapRect to specify which part you want out of it. I think though that reading only the region you need is more elegant.

 
> +                SkIRect srcRect;
> +                srcRect.set(rect.x, rect.y, rect.width, rect.height);
> +                SkRect dstRect = SkRect::MakeWH(bitmap.width(), bitmap.height());
> +                canvas->drawBitmapRect(bitmapResize, &srcRect, dstRect, 0);
> +            } else {
> +                clearSkBitmap(bitmap);
> +                ASSERT_NOT_REACHED();
> +            }
> +        }
> +    } else {
> +        clearSkBitmap(bitmap);
> +        ASSERT_NOT_REACHED();
> +    }
> +}
> +
> +#elif PLATFORM(CG)
> +static inline void clearCGBitmap(const CGContextRef& bitmap)
> +{
> +    CGContextClearRect(bitmap,
> +                       CGRectMake(0, 0, CGBitmapContextGetWidth(bitmap), CGBitmapContextGetHeight(bitmap)));
> +}
> +
> +void WebViewImpl::doPixelReadbackToCanvas(WebCanvas* canvas, WebRect& rect)
> +{
> +    ASSERT((rect.x + rect.width -1) < m_layerRenderer->rootLayerTextureSize().width()
> +           && (rect.y + rect.height -1) < m_layerRenderer->rootLayerTextureSize().height());
> +
> +    void* pixels = 0;
> +    CGContextRef bitmap = reinterpret_cast<CGContextRef>(canvas);
> +    WebRect bitmapRect(0, 0, CGBitmapContextGetWidth(bitmap), CGBitmapContextGetHeight(bitmap));
> +    IntSize size(bitmapRect.width, bitmapRect.height);
> +    if (CGBitmapContextGetRowBytes(bitmap) == 4 * bitmapRect.width) {
> +        if (rect == bitmapRect) {
> +          pixels = CGBitmapContextGetData(bitmap);
> +          m_layerRenderer->getFramebufferPixels(pixels, size);
> +        } else {
> +            IntSize resize(rect.x + rect.width, rect.y + rect.height);
> +
> +            // Create temp bitmap of same size as rendered layer to copy pixels into.
> +            CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> +            CGContextRef bitmapResize = CGBitmapContextCreate(0, resize.width(), resize.height(),
> +                                                              8, 4 * resize.width(), colorSpace,
> +                                                              kCGImageAlphaPremultipliedLast);
> +            if (bitmapResize) {
> +              pixels = CGBitmapContextGetData(bitmapResize);
> +              m_layerRenderer->getFramebufferPixels(pixels, resize);
> +
> +              // Copy bitmap back to input bitmap. The image is inverted according to CG,
> +              // so set up the appropriate transform to invert vertical axis and move origin
> +              // to bottom left.
> +              CGContextSaveGState(bitmap);
> +              CGContextTranslateCTM(bitmap, 0, bitmapRect.height);
> +              CGContextScaleCTM(bitmap, 1.0, -1.0);
> +              CGContextDrawImage(bitmap,
> +                                 CGRectMake(bitmapRect.x, bitmapRect.y, bitmapRect.width, bitmapRect.height),
> +                                 CGBitmapContextCreateImage(bitmapResize));
> +              CGContextRestoreGState(bitmap);
> +            } else {
> +                clearCGBitmap(bitmap);
> +                ASSERT_NOT_REACHED();
> +            }
> +        }
> +    } else {
> +        clearCGBitmap(bitmap);
> +        ASSERT_NOT_REACHED();
> +    }
> +}
> +#else
> +#error Must port to your platform.
> +#endif
> +
> +#endif
> +
>  // FIXME: m_currentInputEvent should be removed once ChromeClient::show() can
>  // get the current-event information from WebCore.
>  const WebInputEvent* WebViewImpl::m_currentInputEvent = 0;
> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> index c29612123f4514c94dd7d970c73114a4beeec355..fd75a20117f9ffdb98223c9498d4ee7ae98643fd 100644
> --- a/WebKit/chromium/src/WebViewImpl.h
> +++ b/WebKit/chromium/src/WebViewImpl.h
> @@ -386,6 +386,7 @@ private:
>  #if USE(ACCELERATED_COMPOSITING)
>      void setIsAcceleratedCompositingActive(bool);
>      void updateRootLayerContents(const WebRect&);
> +    void doPixelReadbackToCanvas(WebCanvas*, WebRect&);
>  #endif
>  
>      WebViewClient* m_client;

[W. James MacLean]

Created attachment 66505 [details]
Patch

[W. James MacLean]

(In reply to comment #45)
> 
> A cleaner way to do this would be to create an IntRect out of WebRect and call IntRect::intersect() with the rect of rootLayerTextureSize.

Thanks for suggesting this. I had been staying with WebRect only because that's what was used in paint(), and I thought it might be preferred. Why do we have WebRect?

> 
> If you continue using IntRect, you can call IntRect::right and IntRect::bottom instead of doing the math. But I think what you really want to 
> ASSERT is rect.intersect(rootLayerRect) == rect

So far I don't have a rootLayerRect around so I am still using bottom() and right(). Is there a strong preference for having rootLayerTextureRect() instead of rootLayerTextureSize()?
 
> It seems like you're always reading from (0,0) up to the width you need.  Ideally you want to read only the rect you were asked to render.  Alternatively, you can eliminate a lot
> of the trimming logic and always read the entire backbuffer and then use the srcRect in drawBitmapRect to specify which part you want out of it. I think though that reading only the region you need is more elegant.

D'oh! It's been so long since I started this that I started assuming glReadPixel() require (0,0) as the start point ... I've fixed this.

[Vangelis Kokkevis]

Comment on attachment 66505 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 44b15f411d30c6fef6233acc6ae7a7eede7eb10f..9926e8d0ab4b023aba4e4775050263441f55d62e 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Replicates existing functionality, use existing tests.
> +
> +        Adds pixel-readback for GPU composited pages to allow for thumbnailing,
> +        printing and other services to work with GPU rendered pages.
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::drawLayers):
> +        (WebCore::LayerRendererChromium::present):
> +        (WebCore::LayerRendererChromium::getFramebufferPixels):
> +        * platform/graphics/chromium/LayerRendererChromium.h:
> +        (WebCore::LayerRendererChromium::rootLayerTextureSize):
> +
>  2010-09-02  Andreas Kling  <[e-mail]>
>  
>          Rubber-stamped by Simon Hausmann.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 50338d2d5f7324e5a119c509190dfef8698b1769..5c3c0b1b316564f4dad5baef0feb458f11fee150 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -39,6 +39,7 @@
>  #include "GLES2Context.h"
>  #include "LayerChromium.h"
>  #include "NotImplemented.h"
> +#include <wtf/OwnArrayPtr.h>
>  #if PLATFORM(SKIA)
>  #include "NativeImageSkia.h"
>  #include "PlatformContextSkia.h"
> @@ -321,11 +322,41 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>  
>      GLC(glDisable(GL_SCISSOR_TEST));
>  
> +    glFlush();
> +}
> +
> +void LayerRendererChromium::present()
> +{
>      m_gles2Context->swapBuffers();
>  
>      m_needsDisplay = false;
>  }
>  
> +void LayerRendererChromium::getFramebufferPixels(void *pixels, const IntRect& rect)
> +{
> +    ASSERT(rect.size() == rootLayerTextureSize());

Shouldn't you be asserting that it's not larger than the rootLayerTextureSize?  It won't always be equal to it, right?

> +
> +    if (!pixels)
> +        return;
> +
> +    makeContextCurrent();
> +
> +    GLC(glReadPixels(rect.x(), rect.y(), rect.width(), rect.height(),
> +                     GL_RGBA, GL_UNSIGNED_BYTE, pixels));
> +
> +    // Flip pixels vertically.
> +    const int rowBytes = 4 * rect.width();
> +    OwnArrayPtr<unsigned char> lineTemp(new unsigned char[rowBytes]);
> +    for (int row1 = 0, row2 = rect.height() - 1; row1 < rect.height() / 2; ++row1, --row2) {
> +        unsigned char* ptr1 = static_cast<unsigned char*>(pixels) + row1 * rowBytes;
> +        unsigned char* ptr2 = static_cast<unsigned char*>(pixels) + row2 * rowBytes;
> +
> +        memcpy(lineTemp.get(), ptr1, rowBytes);
> +        memcpy(ptr1, ptr2, rowBytes);
> +        memcpy(ptr2, lineTemp.get(), rowBytes);
> +    }
> +}
> +
>  // FIXME: This method should eventually be replaced by a proper texture manager.
>  unsigned LayerRendererChromium::createLayerTexture()
>  {
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> index 8f44afebd0a2c7b475d04ba48723f1cdc5455091..ffe41423d53e3f540e710233faf2a0f81562ce57 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> @@ -64,6 +64,7 @@ public:
>      // Updates the contents of the root layer that fall inside the updateRect and recomposites
>      // all the layers.
>      void drawLayers(const IntRect& updateRect, const IntRect& visibleRect, const IntRect& contentRect, const IntPoint& scrollPosition);
> +    void present(); // Perform buffer swap to present rendered buffer.
>  
>      void setRootLayer(PassRefPtr<LayerChromium> layer) { m_rootLayer = layer; }
>      LayerChromium* rootLayer() { return m_rootLayer.get(); }
> @@ -90,6 +91,9 @@ public:
>      const ContentLayerChromium::SharedValues* contentLayerSharedValues() const { return m_contentLayerSharedValues.get(); }
>      const CanvasLayerChromium::SharedValues* canvasLayerSharedValues() const { return m_canvasLayerSharedValues.get(); }
>  
> +    IntSize rootLayerTextureSize() const { return IntSize(m_rootLayerTextureWidth, m_rootLayerTextureHeight); }
> +    void getFramebufferPixels(void *pixels, const IntRect& rect);
> +
>  private:
>      void updateLayersRecursive(LayerChromium* layer, const TransformationMatrix& parentMatrix, float opacity);
>  
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 527e8efccae39045fd985ded3bf20d7166afc69d..65f0e9d4d9a42d16bdccd97988b4abb8b19227ab 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,23 @@
> [phone]  W. James MacLean  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Thumbnails not generated for GPU Rendered Pages
> +        https://bugs.webkit.org/show_bug.cgi?id=44127
> +
> +        Modified WebViewImpl::paint() to detect non-null canvas pointers when
> +        accelerated compositing is active, and instead fills the pixel buffer
> +        from the GPU framebuffer. Includes re-scaling support when provided
> +        canvas does not match size of current render layer. Limits pixel
> +        readback to rect passed to paint(), clipped by size of rootLayerTexture.
> +
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::paint):
> +        (WebKit::clearSkBitmap):
> +        (WebKit::WebViewImpl::doPixelReadbackToCanvas):
> +        (WebKit::clearCGBitmap):
> +        * src/WebViewImpl.h:
> +
>  2010-09-02  Ilya Sherman  <[e-mail]>
>  
>          Reviewed by Eric Seidel.
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp b/WebKit/chromium/src/WebViewImpl.cpp
> index 4b129d694b1f112f27b21ae724db67a68a74e02a..d10b2a8f1acbcbe8493bca580de81e319f3fa952 100644
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -64,7 +64,6 @@
>  #include "HTMLNames.h"
>  #include "Image.h"
>  #include "InspectorController.h"
> -#include "IntRect.h"
>  #include "KeyboardCodes.h"
>  #include "KeyboardEvent.h"
>  #include "MIMETypeRegistry.h"
> @@ -115,6 +114,10 @@
>  #include "WebViewClient.h"
>  #include "wtf/OwnPtr.h"
>  
> +#if PLATFORM(CG)
> +#include <CoreGraphics/CGContext.h>
> +#endif
> +
>  #if OS(WINDOWS)
>  #include "RenderThemeChromiumWin.h"
>  #else
> @@ -952,6 +955,7 @@ void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>              webframe->paint(canvas, rect);
>  #if USE(ACCELERATED_COMPOSITING)
>      } else {
> +

nit: please remove the added newline

>          // Draw the contents of the root layer.
>          updateRootLayerContents(rect);
>  
> @@ -968,10 +972,118 @@ void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  
>          // Ask the layer compositor to redraw all the layers.
>          m_layerRenderer->drawLayers(rect, visibleRect, contentRect, IntPoint(view->scrollX(), view->scrollY()));
> +
> +        // If a canvas was passed in, we use it to grab a copy of the
> +        // freshly-rendered pixels.
> +        if (canvas) {
> +            // Clip rect to the confines of the rootLayerTexture.
> +            IntRect resizeRect(rect.x, rect.y, rect.width, rect.height);
> +            resizeRect.intersect(IntRect(IntPoint(), m_layerRenderer->rootLayerTextureSize()));
> +            doPixelReadbackToCanvas(canvas, resizeRect);
> +        }
> +
> +        m_layerRenderer->present(); // Do final display by swapping buffers.
>      }
>  #endif
>  }
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +#if PLATFORM(SKIA)
> +static inline void clearSkBitmap(const SkBitmap& bitmap)
> +{
> +    bitmap.eraseColor(SkColorSetARGB(0, 0, 0, 0));
> +}
> +
> +void WebViewImpl::doPixelReadbackToCanvas(WebCanvas* canvas, IntRect& rect)
> +{
> +    ASSERT(rect.right() <= m_layerRenderer->rootLayerTextureSize().width()
> +           && rect.bottom() <= m_layerRenderer->rootLayerTextureSize().height());
> +
> +    void* pixels = 0;
> +    const SkBitmap bitmap = canvas->getDevice()->accessBitmap(false);
> +    if (bitmap.config() == SkBitmap::kARGB_8888_Config) {
> +        IntRect bitmapRect(0, 0, bitmap.width(), bitmap.height());
> +
> +        SkAutoLockPixels bitmapLock(bitmap);
> +

The SkAutoLockPixels should move inside the if (rect==bitmapRect) 

> +        if (rect == bitmapRect) {
> +            pixels = bitmap.getPixels();
> +            m_layerRenderer->getFramebufferPixels(pixels, rect);
> +        } else {
> +            // Create temp bitmap of correct size to copy pixels into.
> +            skia::PlatformCanvas canvasResize;
> +            if (canvasResize.initialize(rect.width(), rect.height(), true)) {
> +                SkBitmap bitmapResize = canvasResize.getDevice()->accessBitmap(false);
> +                pixels = bitmapResize.getPixels();
> +                m_layerRenderer->getFramebufferPixels(pixels, rect);
> +                SkIRect srcRect(rect);

I think at this point srcRect should be (0, 0, rect.width(), rect.height())

> +                SkRect dstRect(bitmapRect);
> +                canvas->drawBitmapRect(bitmapResize, &srcRect, dstRect, 0);
> +            } else {
> +                clearSkBitmap(bitmap);
> +                ASSERT_NOT_REACHED();
> +            }
> +        }
> +    } else {
> +        clearSkBitmap(bitmap);
> +        ASSERT_NOT_REACHED();
> +    }
> +}
> +
> +#elif PLATFORM(CG)
> +static inline void clearCGBitmap(const CGContextRef& bitmap)
> +{
> +    CGContextClearRect(bitmap,
> +                       CGRectMake(0, 0, CGBitmapContextGetWidth(bitmap), CGBitmapContextGetHeight(bitmap)));
> +}
> +
> +void WebViewImpl::doPixelReadbackToCanvas(WebCanvas* canvas, IntRect& rect)
> +{
> +    ASSERT(rect.right() <= m_layerRenderer->rootLayerTextureSize().width()
> +           && rect.bottom() <= m_layerRenderer->rootLayerTextureSize().height());
> +
> +    void* pixels = 0;
> +    CGContextRef bitmap = reinterpret_cast<CGContextRef>(canvas);
> +    IntRect bitmapRect(0, 0, CGBitmapContextGetWidth(bitmap), CGBitmapContextGetHeight(bitmap));
> +    if (CGBitmapContextGetRowBytes(bitmap) == 4 * bitmapRect.width()) {
> +        if (rect == bitmapRect) {
> +          pixels = CGBitmapContextGetData(bitmap);
> +          m_layerRenderer->getFramebufferPixels(pixels, rect);
> +        } else {
> +            // Create temp bitmap of same size as rendered layer to copy pixels into.
> +            CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> +            CGContextRef bitmapResize = CGBitmapContextCreate(0, rect.width(), rect.height(),
> +                                                              8, 4 * rect.width(), colorSpace,
> +                                                              kCGImageAlphaPremultipliedLast);
> +            if (bitmapResize) {
> +              pixels = CGBitmapContextGetData(bitmapResize);
> +              m_layerRenderer->getFramebufferPixels(pixels, rect);
> +
> +              // Copy bitmap back to input bitmap. The image is inverted according to CG,
> +              // so set up the appropriate transform to invert vertical axis and move origin
> +              // to bottom left.
> +              CGContextSaveGState(bitmap);
> +              CGContextTranslateCTM(bitmap, 0, bitmapRect.height());
> +              CGContextScaleCTM(bitmap, 1.0, -1.0);
> +              CGContextDrawImage(bitmap, bitmapRect,
> +                                 CGBitmapContextCreateImage(bitmapResize));
> +              CGContextRestoreGState(bitmap);
> +            } else {
> +                clearCGBitmap(bitmap);
> +                ASSERT_NOT_REACHED();
> +            }
> +        }
> +    } else {
> +        clearCGBitmap(bitmap);
> +        ASSERT_NOT_REACHED();
> +    }
> +}
> +#else
> +#error Must port to your platform.
> +#endif
> +
> +#endif
> +
>  // FIXME: m_currentInputEvent should be removed once ChromeClient::show() can
>  // get the current-event information from WebCore.
>  const WebInputEvent* WebViewImpl::m_currentInputEvent = 0;
> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> index c29612123f4514c94dd7d970c73114a4beeec355..67cce0163e0a4f5551a2e608dee26305b5c55766 100644
> --- a/WebKit/chromium/src/WebViewImpl.h
> +++ b/WebKit/chromium/src/WebViewImpl.h
> @@ -45,6 +45,7 @@
>  #include "EditorClientImpl.h"
>  #include "GraphicsLayer.h"
>  #include "InspectorClientImpl.h"
> +#include <IntRect.h>
>  #include "LayerRendererChromium.h"
>  #include "NotificationPresenterImpl.h"
>  #include "SpeechInputClientImpl.h"
> @@ -386,6 +387,7 @@ private:
>  #if USE(ACCELERATED_COMPOSITING)
>      void setIsAcceleratedCompositingActive(bool);
>      void updateRootLayerContents(const WebRect&);
> +    void doPixelReadbackToCanvas(WebCanvas*, WebCore::IntRect&);

const WebCore::IntRect&  ? 

>  #endif
>  
>      WebViewClient* m_client;

[W. James MacLean]

Created attachment 66521 [details]
Patch

[W. James MacLean]

(In reply to comment #48)
> > +    ASSERT(rect.size() == rootLayerTextureSize());
> 
> Shouldn't you be asserting that it's not larger than the rootLayerTextureSize?  It won't always be equal to it, right?

Correct, thanks.
 
> >  #if USE(ACCELERATED_COMPOSITING)
> >      } else {
> > +
> 
> nit: please remove the added newline

Done.
 
> The SkAutoLockPixels should move inside the if (rect==bitmapRect) 

OK. I thought it might be necessary for the drawBitmapRect call too.

> 
> const WebCore::IntRect&  ? 

Yup!

Could I ask if you can remove the excess code when you post a comment like this? Without coloured highlighting in the reply box (when I'm replying to your comments), it's hard to separate your comments from the code (I'm concerned about missing a comment somewhere).

[W. James MacLean]

BTW, this latest patch has yet to be tested on Linux or Mac (I can't do GLX 1.3 over NXclient from home). Will do this Monday morning.

[Vangelis Kokkevis]

Comment on attachment 66521 [details]
Patch

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

> WebKit/chromium/src/WebViewImpl.cpp:1017
> +                SkIRect srcRect(IntRect(IntPoint(), rect.size()));
It was pointed out to me that the semantics of the WebViewImpl::paint() method call for the rect to be painted on the WebCanvas with the same offset it was grabbed from. In other words, the dest rect must be the same as the source rect. Sorry about the confusion!

[W. James MacLean]

Created attachment 66755 [details]
Patch

[W. James MacLean]

(In reply to comment #52)
> (From update of attachment 66521 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66521&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:1017
> > +                SkIRect srcRect(IntRect(IntPoint(), rect.size()));
> It was pointed out to me that the semantics of the WebViewImpl::paint() method call for the rect to be painted on the WebCanvas with the same offset it was grabbed from. In other words, the dest rect must be the same as the source rect. Sorry about the confusion!

Fixed. The line of code you quote above needs to stay as is, as srcRect refers to the temporary bitmap we allocate to handle the (potential) size difference, but the next line (dstRect) changes.

I've also put the vertical-flip code inside a PLATFORM(SKIA) block since
1) CG doesn't need it flipped, so if we leave it in a secondary transform is required,
2) the secondary transform screws up if 'rect' is a sub-set of the canvas, and
3) and it's faster for Mac.

[Vangelis Kokkevis]

Comment on attachment 66755 [details]
Patch

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

> WebCore/ChangeLog:10
> +        printing and other services to work with GPU rendered pages.
Just curious, does any of the pages that the thumbnailing tests use trigger the compositor? If not, we should probably add one just to make sure this functionality doesn't break over time.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:349
> +    // Flip pixels vertically.
If flipping the image is only necessary when stuffing the pixels into a skia canvas then it occurs to me that this isn't the right spot for it. This function knows nothing
about who's going to be using the pixels as it writes straight into a void* memory address.  The flipping, if necessary, needs to happen by the caller. Seems to me that in the Skia case it would be cleanest if you always create a temporary canvas and use a SkCanvas transform to do the flipping as you draw from the temp canvas into the WebCanvas.

If you think that's a reasonable avenue then it might be possible to merge the two platform paths in the following way (which I think is what Darin was suggesting in the first place):

1. Create an ImageBuffer object for your temporary storage. Create an ImageData object to go along with it.
2. Get a CanvasPixelArray out of the ImageData object and use it to store the pixels you get out of the getFrameBufferPixels call.
3. Use, across all platforms, drawImageBuffer() to copy the temporary image onto the GraphicsContext that WebViewImpl::paint gets passed in.

On the surface of it it seems that this should work without any platform specific code... but maybe I'm not seeing something here.

> WebKit/chromium/src/WebViewImpl.cpp:1062
> +              // to bottom left.
The comment above is no longer valid as you're not doing an inversion here anymore. You don't need to save and restore the state either.

[W. James MacLean]

(In reply to comment #55)
> (From update of attachment 66755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66755&action=prettypatch
> 
> > WebCore/ChangeLog:10
> > +        printing and other services to work with GPU rendered pages.
> Just curious, does any of the pages that the thumbnailing tests use trigger the compositor? If not, we should probably add one just to make sure this functionality doesn't break over time.

I don't think thumbnails are tested directly in WebKit (there's a chromium unit test, but it's currently disabled). I'm happy to add in a test that requires the compositor, but haven't the first idea how to do it. Any thoughts as to were this functionality gets tested?
 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:349
> > +    // Flip pixels vertically.
> If flipping the image is only necessary when stuffing the pixels into a skia canvas then it occurs to me that this isn't the right spot for it. This function knows nothing
> about who's going to be using the pixels as it writes straight into a void* memory address.  The flipping, if necessary, needs to happen by the caller. Seems to me that in the Skia case it would be cleanest if you always create a temporary canvas and use a SkCanvas transform to do the flipping as you draw from the temp canvas into the WebCanvas.

It could be handled in WebViewImpl, in the platform-specific code for Skia?

> If you think that's a reasonable avenue then it might be possible to merge the two platform paths in the following way (which I think is what Darin was suggesting in the first place):
> 
> 1. Create an ImageBuffer object for your temporary storage. Create an ImageData object to go along with it.
> 2. Get a CanvasPixelArray out of the ImageData object and use it to store the pixels you get out of the getFrameBufferPixels call.
> 3. Use, across all platforms, drawImageBuffer() to copy the temporary image onto the GraphicsContext that WebViewImpl::paint gets passed in.
> 
> On the surface of it it seems that this should work without any platform specific code... but maybe I'm not seeing something here.

This will work, but:

1) We will still need a few lines of platform-specific code to create the graphics context,
2) since there is no way to get a (writable) pointer to the underlying ImageBuffer pixels, as you suggest we need to create an ImageData object in addition to the ImageBuffer (now we have two additional buffer allocations/deallocations), and
3) copy from the GPU to the ImageData object, then copy to the ImageBuffer, then draw to the original canvas object.

In the event that the input rect and canvas are the same size and smaller than the root layer texture, then we currently need only one pixel transfer for Mac, two for Skia (due to the vertical flip) and no allocation/deallocation of intermediate buffers. I suspect this condition is true in most (if not all) cases.

So the platform-specific code, if not pretty, is probably much more efficient.

That being said, I'm happy to do the platform independent code if that's what is preferred. Darin, do you have any thoughts on this?

I'll upload a new patch as soon as this is decided.

> > WebKit/chromium/src/WebViewImpl.cpp:1062
> > +              // to bottom left.
> The comment above is no longer valid as you're not doing an inversion here anymore. You don't need to save and restore the state either.

Thanks, I missed the comment. I wasn't sure about the context save/restore, but have now removed these.

[W. James MacLean]

Created attachment 66911 [details]
Patch

[W. James MacLean]

This new patch stills keeps the platform-specific code in place (I won't change this until I hear back for sure about making platform-independent), but it does (1) move the Skia image flip into WebViewImpl (using Skia transforms if a temp buffer is already in use), and (2) a conditional compile define to allow easy switching to force the Skia temp buffer (I'd like suggestions for a better name for the #define name - is there a standard for naming these). We don't have to keep (2), but for now it highlights the difference between always using the temporary Skia buffer versus trying to use only the existing buffer.

It also fixes the minor CG issues in the previous comments.

[Vangelis Kokkevis]

Comment on attachment 66911 [details]
Patch

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

You make a good point about the extra allocations and unnecessary copies that would be introduced if we used an intermediate ImageBuffer and ImageData to store the pixels.  However, since this code won't be called too often (only for thumbnailing and printing?) it's probably not worth worrying about it. WebKit allocates and destroys GraphicsContexts on a whim every time it renders a page....  I think the reduced code size and complexity will more than make up for it.

> WebKit/chromium/src/WebViewImpl.cpp:117
> +#include <wtf/OwnArrayPtr.h>
WK uses quotes instead of angle brackets for includes.

> WebKit/chromium/src/WebViewImpl.cpp:998
> +    bitmap.eraseColor(SkColorSetARGB(0, 0, 0, 0));
Since this is a one-liner, it's probably better to just call it directly instead of making a function for it.

> WebKit/chromium/src/WebViewImpl.cpp:1046
> +                clearSkBitmap(bitmap);
Do we really have to worry about the canvasResize not being able to initialize? What would make it fail? Maybe the else path can be removed.

> WebKit/chromium/src/WebViewImpl.cpp:1061
> +    CGContextClearRect(bitmap,
same as above. probably not worth making a separate function for it.

> WebKit/chromium/src/WebViewImpl.cpp:1090
> +                clearCGBitmap(bitmap);
Again, do we have to worry about the failing to create the temporary bitmap?

> WebKit/chromium/src/WebViewImpl.h:48
> +#include <IntRect.h>
Quotes instead of angle brackets

[Adam Barth]

> (From update of attachment 66911 [details])

> > WebKit/chromium/src/WebViewImpl.cpp:117
> > +#include <wtf/OwnArrayPtr.h>
> WK uses quotes instead of angle brackets for includes.

Except for stuff in other frameworks.  In particular, we use <> for grabbing things out of WTF (check out Document.h for examples).

[W. James MacLean]

Created attachment 67071 [details]
Patch

[W. James MacLean]

This patch represents a re-design of the thumbnail read-back code. It is, so far as possible, platform-independent (although a bit of platform-dependent code is required to set up the graphics context and to get the bitmap height for vertical flipping).

The patch assumes that GraphicsContext::drawImageBuffer() is capable of detecting (and ignoring) draw requests if the input canvas/bitmap format is not RGBA.

[Vangelis Kokkevis]

Comment on attachment 67071 [details]
Patch

I like the new clean look!  Thanks for taking the time to make the change.

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

> WebKit/chromium/src/WebViewImpl.cpp:969
> +#error Must port to your platform.
Looks like other parts of the code (e.g. WebFrameImpl::paint()) call notImplemented() instead of giving a compiler error.  Maybe follow that precedent here?

> WebKit/chromium/src/WebViewImpl.cpp:978
> +        gc.translate(FloatSize(0.0f, bitmapHeight));
Isn't the flip necessary for only one of the two platforms?

[W. James MacLean]

(In reply to comment #63)
> (From update of attachment 67071 [details])
> I like the new clean look!  Thanks for taking the time to make the change.

No problem!

> View in context: https://bugs.webkit.org/attachment.cgi?id=67071&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:969
> > +#error Must port to your platform.
> Looks like other parts of the code (e.g. WebFrameImpl::paint()) call notImplemented() instead of giving a compiler error.  Maybe follow that precedent here?

Sure.

> > WebKit/chromium/src/WebViewImpl.cpp:978
> > +        gc.translate(FloatSize(0.0f, bitmapHeight));
> Isn't the flip necessary for only one of the two platforms?

I've tried it on both Mac & Linux (expecting Mac not to need the flip), but I'm guessing that something inside GraphicsContext must unflip the flip for Mac, so that both platforms have the same interface.

I'm verifying this all works properly for rect not equal to the texture layer rect, and will re-upload a patch when I have done so, and made the other change.

[W. James MacLean]

Created attachment 67085 [details]
Patch

[W. James MacLean]

It took a little more work to get this correct for rect's that don't match the texture layer rect, but I've now tested this on a variety of rect sizes and locations, and it seems good.

[Vangelis Kokkevis]

Comment on attachment 67085 [details]
Patch

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

> WebKit/chromium/src/WebViewImpl.cpp:971
> +    // Compute rect to sample from in inverted GPU buffer.
typo: in inverted

> WebKit/chromium/src/WebViewImpl.cpp:972
> +    IntRect invertRect(rect.x(), bitmapHeight - (rect.y() + rect.height()), rect.width(), rect.height());
rect.y + rect.height I believe is returned by rect.bottom() .  I'm curious though, isn't adding a Y offset here equivalent to some gc.translate further down?  It seems that you would only need one of the two, with the appropriate values. I'd venture to say maybe replace your current gc.translate by a gc.translate(FloatSize(0, rect.bottom())  but use the original rect?

[W. James MacLean]

(In reply to comment #67)
> (From update of attachment 67085 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67085&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:971
> > +    // Compute rect to sample from in inverted GPU buffer.
> typo: in inverted

Thanks, will fix.
 
> > WebKit/chromium/src/WebViewImpl.cpp:972
> > +    IntRect invertRect(rect.x(), bitmapHeight - (rect.y() + rect.height()), rect.width(), rect.height());
> rect.y + rect.height I believe is returned by rect.bottom() .  I'm curious though, isn't adding a Y offset here equivalent to some gc.translate further down?  It seems that you would only need one of the two, with the appropriate values. I'd venture to say maybe replace your current gc.translate by a gc.translate(FloatSize(0, rect.bottom())  but use the original rect?

Yes, I could use rect.bottom(). Note that the expression above changes the sign of rect.y() also.

Assuming that the value (x,y)=(0,0) in the WebRect passed into paint() is meant to be top-left (with y values increasing downward), then it will be necessary to transform the rect *before* passing it to getFramebufferPixels, as the GPU is mapping (0,0) to the bottom-left, with y-values increasing upwards.

If rect is (0,0,root_texture_width, root_texture_height) then this transformation doesn't change the rect, and then the gc transforms invert what is returned. But if rect is anything else, then it must be transformed before the pixels are read back, and the returned bitmap must be transformed again when writing into the gc to flip it vertically.

I tried it using just the original rect for the gc writeback, but it seems the draw function wants to transform the destRect coordinates prior to doing the draw. 

It seems obvious that rect must be transformed before the gpu read-back can be done. If glReadPixels() allowed one to flip the pixels vertically during the read it would help, but it doesn't.

Does that make sense?

[Vangelis Kokkevis]

Comment on attachment 67085 [details]
Patch

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

> WebKit/chromium/src/WebViewImpl.cpp:972
> +    IntRect invertRect(rect.x(), bitmapHeight - (rect.y() + rect.height()), rect.width(), rect.height());
Ah, ok, I think I see it now! Thanks for the detailed explanation.

[W. James MacLean]

Created attachment 67178 [details]
Patch

[W. James MacLean]

Created attachment 67419 [details]
Patch

[W. James MacLean]

Revised to accommodate Nat's patch.

The merge seemed to think there should be no glFlush() at the end of drawLayers() ... is this OK? (Seems to work on Linux/Mac.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 67419 [details]
Patch

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

> WebKit/chromium/src/WebViewImpl.cpp:980
> +    OwnPtr<ImageBuffer> imgBuffer(ImageBuffer::create(rect.size()));
nit: imgBuffer -> imageBuffer, imgData -> imageData

webkit style prefers spelling out variable names

> WebKit/chromium/src/WebViewImpl.cpp:1006
> +            IntRect resizeRect(rect.x, rect.y, rect.width, rect.height);
nit: you can also just write:  IntRect resizeRect(rect);

there is an implicit conversion operator from WebRect to IntRect


R=me otherwise

[W. James MacLean]

Created attachment 67444 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 67444 [details]
Patch

Clearing flags on attachment: 67444

Committed r67445: <http://trac.webkit.org/changeset/67445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67445 might have broken GTK Linux 32-bit Debug