[Webkit-unassigned] [Bug 44127] [chromium] Thumbnails not generated for GPU Rendered Pages

Wed Sep 8 08:18:24 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=44127

--- Comment #56 from W. James MacLean <wjmaclean at chromium.org>  2010-09-08 08:18:24 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.