Bug 84808

Summary: [chromium] Move ProgramBinding definitions to LayerRendererChromium and normalize naming
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, eric.carlson, feature-media-reviews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch enne: review+

Description James Robinson 2012-04-24 17:16:41 PDT
[chromium] Move ProgramBinding definitions to LayerRendererChromium and normalize naming
Comment 1 James Robinson 2012-04-24 17:24:34 PDT
Created attachment 138698 [details]
Patch
Comment 2 Dana Jansens 2012-04-25 09:23:41 PDT
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 :)
Comment 3 Adrienne Walker 2012-04-25 17:55:15 PDT
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.
Comment 4 James Robinson 2012-04-25 22:53:55 PDT
(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.
Comment 5 Adrienne Walker 2012-04-25 23:58:10 PDT
(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?
Comment 6 James Robinson 2012-04-26 18:08:36 PDT
Created attachment 139115 [details]
Patch
Comment 7 James Robinson 2012-04-26 18:10:57 PDT
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;
Comment 8 Adrienne Walker 2012-04-26 18:15:33 PDT
Comment on attachment 139115 [details]
Patch

R=me.
Comment 9 James Robinson 2012-04-26 20:21:37 PDT
Committed r115403: <http://trac.webkit.org/changeset/115403>