Chromium Mac: Add TEXTURE_RECTANGLE_ARB support to CCVideoLayerImpl
Created attachment 152110 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 152110 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/ChangeLog:22: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 152114 [details] Patch
Comment on attachment 152114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152114&action=review > Source/WebCore/ChangeLog:10 > + No new tests (OOPS!). You'll need to add tests or justify why none are possible here. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1675 > +const LayerRendererChromium::TextureRectProgramFlip* LayerRendererChromium::textureRectProgramFlip() bikeshed: s/Flip/Flipped/ ?
Comment on attachment 152114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152114&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1675 >> +const LayerRendererChromium::TextureRectProgramFlip* LayerRendererChromium::textureRectProgramFlip() > > bikeshed: s/Flip/Flipped/ ? Do we really need another program? Can't the rect that's being sent to the shader be adjusted? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:204 > - typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectFlipAlpha> TextureIOSurfaceProgram; > + typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectAlpha> TextureRectProgram; > + typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectFlipAlpha> TextureRectProgramFlip; bikshed: I'll take other suggestions, but I don't like this renaming. TextureRect is just too ambiguous, since TextureProgram and TextureProgramFlip take texture rects as uniforms. > Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:211 > + case GraphicsContext3D::TEXTURE_2D: > + case Extensions3D::TEXTURE_RECTANGLE_ARB: { Can you reuse the CCIOSurfaceDrawQuad rather than shoehorning this into the CCTextureDrawQuad?
Created attachment 152801 [details] Patch
Comment on attachment 152114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152114&action=review >>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1675 >>> +const LayerRendererChromium::TextureRectProgramFlip* LayerRendererChromium::textureRectProgramFlip() >> >> bikeshed: s/Flip/Flipped/ ? > > Do we really need another program? Can't the rect that's being sent to the shader be adjusted? Removed. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:204 >> + typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderRGBATexRectFlipAlpha> TextureRectProgramFlip; > > bikshed: I'll take other suggestions, but I don't like this renaming. TextureRect is just too ambiguous, since TextureProgram and TextureProgramFlip take texture rects as uniforms. Removed. >> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:211 >> + case Extensions3D::TEXTURE_RECTANGLE_ARB: { > > Can you reuse the CCIOSurfaceDrawQuad rather than shoehorning this into the CCTextureDrawQuad? Done. Reused CCIOSurfaceDrawQuad and added a flipped field.
Comment on attachment 152801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152801&action=review This patch is much improved. Thanks for all the changes. Along with rebasing to get this to apply to ToT, I have one more small change, but then this should be good: > Source/WebCore/platform/graphics/chromium/cc/CCIOSurfaceLayerImpl.cpp:102 > - quadList.append(CCIOSurfaceDrawQuad::create(sharedQuadState, quadRect, m_ioSurfaceSize, m_ioSurfaceTextureId)); > + quadList.append(CCIOSurfaceDrawQuad::create(sharedQuadState, quadRect, m_ioSurfaceSize, m_ioSurfaceTextureId, true)); Ack, boolean parameter trap (http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html). Even if you store flipped as a boolean member variable, can you add an enum { Flipped, Unflipped } to the create function signature so it's obvious what the parameter means at the callsite?
Created attachment 153058 [details] Patch
Comment on attachment 152801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152801&action=review Rebased to tip of tree. >> Source/WebCore/platform/graphics/chromium/cc/CCIOSurfaceLayerImpl.cpp:102 >> + quadList.append(CCIOSurfaceDrawQuad::create(sharedQuadState, quadRect, m_ioSurfaceSize, m_ioSurfaceTextureId, true)); > > Ack, boolean parameter trap (http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html). Even if you store flipped as a boolean member variable, can you add an enum { Flipped, Unflipped } to the create function signature so it's obvious what the parameter means at the callsite? Done.
Comment on attachment 153058 [details] Patch R=me. Thanks for that change. It looks like this still doesn't apply, though.
Created attachment 153064 [details] Patch
(In reply to comment #12) > (From update of attachment 153058 [details]) > R=me. Thanks for that change. It looks like this still doesn't apply, though. Oops, updated. New patch seems to apply now. Thanks for the review!
Comment on attachment 153064 [details] Patch R=meagain.
Comment on attachment 153064 [details] Patch Clearing flags on attachment: 153064 Committed r123009: <http://trac.webkit.org/changeset/123009>
All reviewed patches have been landed. Closing bug.