[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 #8 from Victoria <vrk at google.com>  2010-09-13 13:29:03 PST ---
Thanks so much for the helpful comments, Vangelis! I fixed my code to fit all of your suggestions.

I also made a small but important change: in my original patch, I had forgot to address the stride of the video frames! Unfortunately, it doesn't seem like there is an easy way to set the stride for the texture in GLES2... the way I've seen this done in other GL code is by calling the command glPixelStorei(GL_UNPACK_ALIGNMENT, <stride>);, but GLES2 doesn't recognize GL_UNPACK_ALIGNMENT. So instead what I do is I allocate textures to the width of the stride, copy the entire data into the texture, then in the vertex shader I adjust the x component of the v_texCoord accordingly to clip off the padding between the edge of the frame and the stride. (Big thanks to Alpha for the idea!) I was planning to talk to Gregg and James to see if they could get the GL_UNPACK_ALIGNMENT parameter working, but if that's not possible, I think this is an okay solution.

Please take a look at my patch again and let me know what you think!

(In reply to comment #4)
> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:90
> I think you still need to pass in an alpha value to the shader so that we can get respect the layer's opacity.
Done.

> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:185
> 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.
Done.

> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:215
> If you remove the glActiveTexture calls from the update methods then you won't need to call this here either.
Done (as well as other places mentioned).

> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:228
> 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?
Done.

> > 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.
That is a good point! I don't think we ever do partial frame updates; the whole updateRect/dirtyFrame/etc stuff was mostly carryover from when I was copying most of my logic from LayerChromium. I modified the code (in here and other places as appropriate) to get rid of all the partial frame stuff.

> > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:296
> Maybe better to name this method allocateTexture or initializeTexture as it doesn't really create a texture?
Done here and elsewhere.

> > 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
I decided to pass an IntSize for the width and height since the xOffset/yOffset only makes sense for updating partial frames.

> > 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.
Done. 

> > 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.
Thanks for the catch! Done.

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