[Webkit-unassigned] [Bug 45069] VideoLayerChromium uses GPU YUV to RGB color conversion

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Vangelis Kokkevis <vangelis at chromium.org>  2010-09-08 23:26:54 PST ---
(From update of attachment 66298)
View in context: https://bugs.webkit.org/attachment.cgi?id=66298&action=prettypatch

Looks good!  A few comments inline.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:90
> +        "  gl_FragColor = vec4(rgb, 1);                       \n"
I think you still need to pass in an alpha value to the shader so that we can get respect the layer's opacity.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:185
> +    if (!frame)
Shouldn't the required texture size be the size of the frame rather than m_bounds?  m_bounds reflects the size that the layer will be displayed at which could be different than the frame size. You actually seem to ignore the requiredTextureSize value passed into the update*Contents method anyway.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:215
> +    glActiveTexture(GL_TEXTURE0);
If you remove the glActiveTexture calls from the update methods then you won't need to call this here either.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:228
> +            glDeleteTextures(1, &m_vTextureId);
This could be somewhat optimized if you put all 3 texture values in an array and call glDeleteTextures(3, array).  Also, for good measure, maybe put glDeleteTextures within a GLC() macro?

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:241
> +void VideoLayerChromium::createYUVTextures(VideoFrameChromium* frame, const IntSize& requiredTextureSize,
requiredTextureSize isn't used in this method

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:246
> +    GLC(glActiveTexture(GL_TEXTURE1));
I don't believe you need to call glActiveTexture here and below.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:259
> +void VideoLayerChromium::updateYUVTextures(VideoFrameChromium* frame, const IntRect& updateRect)
Do you ever do partial frame updates or is the updateRect always the same as the size of the frame? I'm wondering whether this parameter could be eliminated.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:263
> +    GLC(glActiveTexture(GL_TEXTURE1));
Same here:  glActiveTexture isn't necessary.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:296
> +void VideoLayerChromium::createTexture(unsigned textureId, const void* data, unsigned width, unsigned height, GLenum format)
Maybe better to name this method allocateTexture or initializeTexture as it doesn't really create a texture?

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:303
> +void VideoLayerChromium::updateTexture(unsigned textureId, const void* data, unsigned xOffset, unsigned yOffset,
it might be more convenient to pass an IntRect here instead of 4 separate values

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:343
> +    glActiveTexture(GL_TEXTURE0);
Maybe this call could be moved in the drawYUV method as drawRGB doesn't change the active texture.

> WebCore/platform/graphics/chromium/VideoLayerChromium.h:49
>      virtual void updateContents();
You should probably also define a destructor which calls glDeleteTexture on all the textures used by the layer, otherwise they will never be freed.

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