[webkit-reviews] review denied: [Bug 44127] [chromium] Thumbnails not generated for GPU Rendered Pages : [Attachment 66505] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 3 10:09:51 PDT 2010


Vangelis Kokkevis <vangelis at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 44127: [chromium] Thumbnails not generated for GPU Rendered Pages
https://bugs.webkit.org/show_bug.cgi?id=44127

Attachment 66505: Patch
https://bugs.webkit.org/attachment.cgi?id=66505&action=review

------- Additional Comments from Vangelis Kokkevis <vangelis at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index
44b15f411d30c6fef6233acc6ae7a7eede7eb10f..9926e8d0ab4b023aba4e4775050263441f55d
62e 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> +2010-09-02  W. James MacLean  <wjmaclean at chromium.org>
> +
> +	   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  <andreas.kling at nokia.com>
>  
>	   Rubber-stamped by Simon Hausmann.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index
50338d2d5f7324e5a119c509190dfef8698b1769..5c3c0b1b316564f4dad5baef0feb458f11fee
150 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..ffe41423d53e3f540e710233faf2a0f81562c
e57 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..65f0e9d4d9a42d16bdccd97988b4abb8b1922
7ab 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,23 @@
> +2010-09-02  W. James MacLean  <wjmaclean at chromium.org>
> +
> +	   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  <isherman at google.com>
>  
>	   Reviewed by Eric Seidel.
> diff --git a/WebKit/chromium/src/WebViewImpl.cpp
b/WebKit/chromium/src/WebViewImpl.cpp
> index
4b129d694b1f112f27b21ae724db67a68a74e02a..d10b2a8f1acbcbe8493bca580de81e319f3fa
952 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..67cce0163e0a4f5551a2e608dee26305b5c55
766 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;


More information about the webkit-reviews mailing list