Bug 84808 – [chromium] Move ProgramBinding definitions to LayerRendererChromium and normalize naming

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Move ProgramBinding definitions to LayerRendererChromium and normalize naming

[James Robinson]

Created attachment 138698 [details]
Patch

[Dana Jansens]

Comment on attachment 138698 [details]
Patch

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

+1

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:214
> +    // FIXME: Add a quad type for render surfaces and get rid of this friendlyness.

nit: quad type exists. We want to move the drawing of said quads into LRC :)

[Adrienne Walker]

Comment on attachment 138698 [details]
Patch

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

R♥ to all the program renames, inclusion removal, and program deduplication.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1012
> -            binding.set(textureLayerProgramStretchFlip());
> +            binding.set(textureTexRectProgramFlip());

...but you're changing the fragment shader here from FragmentShaderRGBATexFlipAlpha to FragmentShaderRGBATexRectFlipAlpha (the io surface one) for the non-io surface path.  Good thing our testing covers this.

[James Robinson]

(In reply to comment #3)
> (From update of attachment 138698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138698&action=review
> 
> R♥ to all the program renames, inclusion removal, and program deduplication.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1012
> > -            binding.set(textureLayerProgramStretchFlip());
> > +            binding.set(textureTexRectProgramFlip());
> 
> ...but you're changing the fragment shader here from FragmentShaderRGBATexFlipAlpha to FragmentShaderRGBATexRectFlipAlpha (the io surface one) for the non-io surface path.  Good thing our testing covers this.

Damn!  Guessing the testing comment is sarcasm?  TexRect is for ARB_texture_rectangle I guess but it's too easy to confuse with all the other things talking about textures and rectangles.  Will take another shot at this.

[Adrienne Walker]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 138698 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138698&action=review
> > 
> > R♥ to all the program renames, inclusion removal, and program deduplication.
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1012
> > > -            binding.set(textureLayerProgramStretchFlip());
> > > +            binding.set(textureTexRectProgramFlip());
> > 
> > ...but you're changing the fragment shader here from FragmentShaderRGBATexFlipAlpha to FragmentShaderRGBATexRectFlipAlpha (the io surface one) for the non-io surface path.  Good thing our testing covers this.
> 
> Damn!  Guessing the testing comment is sarcasm?  TexRect is for ARB_texture_rectangle I guess but it's too easy to confuse with all the other things talking about textures and rectangles.  Will take another shot at this.

As a random thought, maybe it'd be worth changing the name of it to textureIOSurfaceProgramFlip() to talk more about how it should be used and less about what its implementation details are?

[James Robinson]

Created attachment 139115 [details]
Patch

[James Robinson]

I've renamed the IOSurface programs to have "IOSurface" in the name and fixed TextureProgram / TextureProgramFlip to be what we want and removed a somewhat silly no-vertex-transform path for flipped texture quads.  Doing this also meant that the VideoRGBAProgram could be combined with the TextureProgram

Diff to previous patch:


diff --git a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
index 7c30165..034b547 100644
--- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
@@ -868,7 +868,7 @@ void LayerRendererChromium::drawSingleTextureVideoQuad(const CCVideoDrawQuad* qu
 
 void LayerRendererChromium::drawRGBA(const CCVideoDrawQuad* quad)
 {
-    const VideoRGBAProgram* program = videoRGBAProgram();
+    const TextureProgram* program = textureProgram();
     const CCVideoLayerImpl::Texture& texture = quad->textures()[WebKit::WebVideoFrame::rgbPlane];
     float widthScaleFactor = static_cast<float>(texture.m_visibleSize.width()) / texture.m_texture->size().width();
     drawSingleTextureVideoQuad(quad, program, widthScaleFactor, texture.m_texture->textureId(), GraphicsContext3D::TEXTURE_2D);
@@ -876,7 +876,7 @@ void LayerRendererChromium::drawRGBA(const CCVideoDrawQuad* quad)
 
 void LayerRendererChromium::drawNativeTexture2D(const CCVideoDrawQuad* quad)
 {
-    drawSingleTextureVideoQuad(quad, videoRGBAProgram(), 1, quad->frame()->textureId(), GraphicsContext3D::TEXTURE_2D);
+    drawSingleTextureVideoQuad(quad, textureProgram(), 1, quad->frame()->textureId(), GraphicsContext3D::TEXTURE_2D);
 }
 
 void LayerRendererChromium::drawStreamTexture(const CCVideoDrawQuad* quad)
@@ -986,9 +986,9 @@ void LayerRendererChromium::drawTextureQuad(const CCTextureDrawQuad* quad)
     if (quad->ioSurfaceTextureId()) {
         TexTransformTextureProgramBinding binding;
         if (quad->flipped())
-            binding.set(textureTexRectProgramFlip());
+            binding.set(textureIOSurfaceProgramFlip());
         else
-            binding.set(textureTexRectProgram());
+            binding.set(textureIOSurfaceProgram());
 
         GLC(context(), context()->activeTexture(GraphicsContext3D::TEXTURE0));
 
@@ -998,20 +998,12 @@ void LayerRendererChromium::drawTextureQuad(const CCTextureDrawQuad* quad)
 
         matrixLocation = binding.matrixLocation;
         alphaLocation = binding.alphaLocation;
-    } else if (quad->flipped() && quad->uvRect() == FloatRect(0, 0, 1, 1)) {
-        // A flipped quad with the default UV mapping is common enough to use a special shader.
-        // Canvas 2d and WebGL layers use this path always and plugin/external texture layers use this by default.
-        const TextureProgramFlip* program = textureProgramFlip();
-        GLC(context(), context()->useProgram(program->program()));
-        GLC(context(), context()->uniform1i(program->fragmentShader().samplerLocation(), 0));
-        matrixLocation = program->vertexShader().matrixLocation();
-        alphaLocation = program->fragmentShader().alphaLocation();
     } else {
         TexTransformTextureProgramBinding binding;
         if (quad->flipped())
-            binding.set(textureTexRectProgramFlip());
+            binding.set(textureProgramFlip());
         else
-            binding.set(textureTexRectProgram());
+            binding.set(textureProgram());
         GLC(context(), context()->useProgram(binding.programId));
         GLC(context(), context()->uniform1i(binding.samplerLocation, 0));
         const FloatRect& uvRect = quad->uvRect();
@@ -1532,48 +1524,48 @@ const LayerRendererChromium::TileProgramSwizzleAA* LayerRendererChromium::tilePr
     return m_tileProgramSwizzleAA.get();
 }
 
+const LayerRendererChromium::TextureProgram* LayerRendererChromium::textureProgram()
+{
+    if (!m_textureProgram)
+        m_textureProgram = adoptPtr(new TextureProgram(m_context.get()));
+    if (!m_textureProgram->initialized()) {
+        TRACE_EVENT("LayerRendererChromium::textureProgram::initialize", this, 0);
+        m_textureProgram->initialize(m_context.get());
+    }
+    return m_textureProgram.get();
+}
+
 const LayerRendererChromium::TextureProgramFlip* LayerRendererChromium::textureProgramFlip()
 {
     if (!m_textureProgramFlip)
         m_textureProgramFlip = adoptPtr(new TextureProgramFlip(m_context.get()));
     if (!m_textureProgramFlip->initialized()) {
-        TRACE_EVENT("LayerRendererChromium::textureProgram::initialize", this, 0);
+        TRACE_EVENT("LayerRendererChromium::textureProgramFlip::initialize", this, 0);
         m_textureProgramFlip->initialize(m_context.get());
     }
     return m_textureProgramFlip.get();
 }
 
-const LayerRendererChromium::TextureTexRectProgram* LayerRendererChromium::textureTexRectProgram()
-{
-    if (!m_textureTexRectProgram)
-        m_textureTexRectProgram = adoptPtr(new TextureTexRectProgram(m_context.get()));
-        TRACE_EVENT("LayerRendererChromium::textureTexRectProgram::initialize", this, 0);
-        m_textureTexRectProgram->initialize(m_context.get());
-    }
-    return m_textureTexRectProgram.get();
-}
-
-const LayerRendererChromium::TextureTexRectProgramFlip* LayerRendererChromium::textureTexRectProgramFlip()
+const LayerRendererChromium::TextureIOSurfaceProgram* LayerRendererChromium::textureIOSurfaceProgram()
 {
-    if (!m_textureTexRectProgramFlip)
-        m_textureTexRectProgramFlip = adoptPtr(new TextureTexRectProgramFlip(m_context.get()));
-    if (!m_textureTexRectProgramFlip->initialized()) {
-        TRACE_EVENT("LayerRendererChromium::textureTexRectProgramFlip::initialize", this, 0);
-        m_textureTexRectProgramFlip->initialize(m_context.get());
+    if (!m_textureIOSurfaceProgram)
+        m_textureIOSurfaceProgram = adoptPtr(new TextureIOSurfaceProgram(m_context.get()));
+    if (!m_textureIOSurfaceProgram->initialized()) {
+        TRACE_EVENT("LayerRendererChromium::textureIOSurfaceProgram::initialize", this, 0);
+        m_textureIOSurfaceProgram->initialize(m_context.get());
     }
-    return m_textureTexRectProgramFlip.get();
+    return m_textureIOSurfaceProgram.get();
 }
 
-const LayerRendererChromium::VideoRGBAProgram* LayerRendererChromium::videoRGBAProgram()
+const LayerRendererChromium::TextureIOSurfaceProgramFlip* LayerRendererChromium::textureIOSurfaceProgramFlip()
 {
-    if (!m_videoRGBAProgram)
-        m_videoRGBAProgram = adoptPtr(new VideoRGBAProgram(m_context.get()));
-    if (!m_videoRGBAProgram->initialized()) {
-        TRACE_EVENT("LayerRendererChromium::videoRGBAProgram::initialize", this, 0);
-        m_videoRGBAProgram->initialize(m_context.get());
+    if (!m_textureIOSurfaceProgramFlip)
+        m_textureIOSurfaceProgramFlip = adoptPtr(new TextureIOSurfaceProgramFlip(m_context.get()));
+    if (!m_textureIOSurfaceProgramFlip->initialized()) {
+        TRACE_EVENT("LayerRendererChromium::textureIOSurfaceProgramFlip::initialize", this, 0);
+        m_textureIOSurfaceProgramFlip->initialize(m_context.get());
     }
-    return m_videoRGBAProgram.get();
+    return m_textureIOSurfaceProgramFlip.get();
 }
 
 const LayerRendererChromium::VideoYUVProgram* LayerRendererChromium::videoYUVProgram()
@@ -1628,15 +1620,15 @@ void LayerRendererChromium::cleanupSharedObjects()
     if (m_renderSurfaceProgramAA)
         m_renderSurfaceProgramAA->cleanup(m_context.get());
 
+    if (m_textureProgram)
+        m_textureProgram->cleanup(m_context.get());
     if (m_textureProgramFlip)
         m_textureProgramFlip->cleanup(m_context.get());
-    if (m_textureTexRectProgram)
-        m_textureTexRectProgram->cleanup(m_context.get());
-    if (m_textureTexRectProgramFlip)
-        m_textureTexRectProgramFlip->cleanup(m_context.get());
+    if (m_textureIOSurfaceProgram)
+        m_textureIOSurfaceProgram->cleanup(m_context.get());
+    if (m_textureIOSurfaceProgramFlip)
+        m_textureIOSurfaceProgramFlip->cleanup(m_context.get());
 
-    if (m_videoRGBAProgram)
-        m_videoRGBAProgram->cleanup(m_context.get());
     if (m_videoYUVProgram)
         m_videoYUVProgram->cleanup(m_context.get());
     if (m_videoStreamTextureProgram)
diff --git a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h
index 490ca72..2f7dbc3 100644
--- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h
+++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h
@@ -211,7 +211,7 @@ private:
 
     // Render surface shaders.
     // CCRenderSurface::drawLayers() needs to see these programs currently.
-    // FIXME: Add a quad type for render surfaces and get rid of this friendlyness.
+    // FIXME: Draw with a quad type for render surfaces and get rid of this friendlyness.
     friend class CCRenderSurface; 
     typedef ProgramBinding<VertexShaderPosTex, FragmentShaderRGBATexAlpha> RenderSurfaceProgram;
     typedef ProgramBinding<VertexShaderPosTex, FragmentShaderRGBATexAlphaMask> RenderSurfaceMaskProgram;
@@ -219,12 +219,12 @@ private:
     typedef ProgramBinding<VertexShaderQuad, FragmentShaderRGBATexAlphaMaskAA> RenderSurfaceMaskProgramAA;
 
     // Texture shaders.
-    typedef ProgramBinding<VertexShaderPosTex, FragmentShaderRGBATexFlipAlpha> TextureProgramFlip;
-    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectAlpha> TextureTexRectProgram;
-    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectFlipAlpha> TextureTexRectProgramFlip;
+    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexAlpha> TextureProgram;
+    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexFlipAlpha> TextureProgramFlip;
+    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectAlpha> TextureIOSurfaceProgram;
+    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectFlipAlpha> TextureIOSurfaceProgramFlip;
 
     // Video shaders.
-    typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexFlipAlpha> VideoRGBAProgram;
     typedef ProgramBinding<VertexShaderVideoTransform, FragmentShaderOESImageExternal> VideoStreamTextureProgram;
     typedef ProgramBinding<VertexShaderPosTexYUVStretch, FragmentShaderYUVVideo> VideoYUVProgram;
 
@@ -248,11 +248,11 @@ private:
     const RenderSurfaceMaskProgram* renderSurfaceMaskProgram();
     const RenderSurfaceMaskProgramAA* renderSurfaceMaskProgramAA();
 
+    const TextureProgram* textureProgram();
     const TextureProgramFlip* textureProgramFlip();
-    const TextureTexRectProgram* textureTexRectProgram();
-    const TextureTexRectProgramFlip* textureTexRectProgramFlip();
+    const TextureIOSurfaceProgram* textureIOSurfaceProgram();
+    const TextureIOSurfaceProgramFlip* textureIOSurfaceProgramFlip();
 
-    const VideoRGBAProgram* videoRGBAProgram();
     const VideoYUVProgram* videoYUVProgram();
     const VideoStreamTextureProgram* videoStreamTextureProgram();
 
@@ -273,11 +273,11 @@ private:
     OwnPtr<RenderSurfaceMaskProgram> m_renderSurfaceMaskProgram;
     OwnPtr<RenderSurfaceMaskProgramAA> m_renderSurfaceMaskProgramAA;
 
+    OwnPtr<TextureProgram> m_textureProgram;
     OwnPtr<TextureProgramFlip> m_textureProgramFlip;
-    OwnPtr<TextureTexRectProgram> m_textureTexRectProgram;
-    OwnPtr<TextureTexRectProgramFlip> m_textureTexRectProgramFlip;
+    OwnPtr<TextureIOSurfaceProgram> m_textureIOSurfaceProgram;
+    OwnPtr<TextureIOSurfaceProgramFlip> m_textureIOSurfaceProgramFlip;
 
-    OwnPtr<VideoRGBAProgram> m_videoRGBAProgram;
     OwnPtr<VideoYUVProgram> m_videoYUVProgram;
     OwnPtr<VideoStreamTextureProgram> m_videoStreamTextureProgram;

[Adrienne Walker]

Comment on attachment 139115 [details]
Patch

R=me.

[James Robinson]

Committed r115403: <http://trac.webkit.org/changeset/115403>