Bug 91169 – Chromium Mac: Add TEXTURE_RECTANGLE_ARB support to CCVideoLayerImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sailesh Agrawal]

Chromium Mac: Add TEXTURE_RECTANGLE_ARB support to CCVideoLayerImpl

[Sailesh Agrawal]

Created attachment 152110 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Sailesh Agrawal]

Created attachment 152114 [details]
Patch

[Dana Jansens]

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/ ?

[Adrienne Walker]

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?

[Sailesh Agrawal]

Created attachment 152801 [details]
Patch

[Sailesh Agrawal]

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.

[Adrienne Walker]

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?

[Sailesh Agrawal]

Created attachment 153058 [details]
Patch

[Sailesh Agrawal]

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.

[Adrienne Walker]

Comment on attachment 153058 [details]
Patch

R=me.  Thanks for that change.  It looks like this still doesn't apply, though.

[Sailesh Agrawal]

Created attachment 153064 [details]
Patch

[Sailesh Agrawal]

(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!

[Adrienne Walker]

Comment on attachment 153064 [details]
Patch

R=meagain.

[WebKit Review Bot]

Comment on attachment 153064 [details]
Patch

Clearing flags on attachment: 153064

Committed r123009: <http://trac.webkit.org/changeset/123009>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.