[Webkit-unassigned] [Bug 50833] [chromium] Add support to compositor to composite to offscreen texture.

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from W. James MacLean <wjmaclean at chromium.org>  2010-12-14 11:26:49 PST ---
(In reply to comment #8)
> (From update of attachment 76232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76232&action=review
> 
> Generally looks fine. I think if this were a real implementation you would eliminate the copyOffscreenTextureToDisplay call and you would allocate the additional texture for the default render surface only when the compositor is told to render off screen. Otherwise you would just render straight to the display backbuffer as it's done today.
> 
> I also think there might be some additional merging that needs to happen with jamesr's texture manager CL.

This is done in the current patch. Most of the changes are in copyOffscreenTextureToDisplay(), which will disappear once this patch is past "discussion" and into "review".

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:201
> > +        if (m_rootLayer->m_renderSurface.get())
> 
> you don't have to include the .get().  It's enough to test m_rootLayer->m_renderSurface .

Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:294
> > +    if (!m_rootLayer->m_renderSurface.get())
> 
> remove .get()

Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:666
> > +    m_renderOffscreen = compositeOffscreen;
> 
> nit: Ideally the member name and the variable name should match.  I think I prefer compositeOffscreen to renderOffscreen

Agrred, I thought of this too after submitting the previous patches. Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:696
> > +        GLC(m_context, m_context->colorMask(true, true, true, false));
> 
> I don't know that masking is necessary as it should have been done when you drew into the default render surface so hopefully the alpha channel is ok.

Removed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:702
> > +                                        contentLayerValues->shaderMatrixLocation(), contentLayerValues->shaderAlphaLocation());
> 
> You need to use m_textureLayerShaderMatrixLocation and m_textureLayerShaderAlphaLocation here. Also, after Jamesr patch landed, RenderSurfaceChromium has its own draw method I believe.

Redundant, as new patch uses RenderSurfaceChromium::draw() which looks after this.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:796
> > +    // But, if rendering to offscreen texture, we reverse our sense of 'upside down'.
> 
> This seems a little strange.  I can see how maybe you don't need to render upside down when rendering to the default render surface but I don't see why you need to flip all the other render surfaces too.

I'm guessing that the final drawing of the texture also gets flipped, so we need to do this so everything comes out right in the end. Without it, clipping on pages like http://webkit.org/blog/386/3d-transforms/ and http://peter.sh/2010/06/chromium-now-features-gpu-acceleration-and-css-3d-transforms/ are upside-down and translate the wrong way.

I'd like to submit this as a patch for review/commit, so please let me know if it's ready (assuming I'll remove copyOffscreenTextureToDisplay() and the two lines of test code in WebViewImpl.cpp first ...).

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