[webkit-reviews] review granted: [Bug 178254] Performance: Skip texture upload if source image and destination texture haven't changed : [Attachment 323720] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 13 12:17:12 PDT 2017


Dean Jackson <dino at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 178254: Performance: Skip texture upload if source image and destination
texture haven't changed
https://bugs.webkit.org/show_bug.cgi?id=178254

Attachment 323720: Patch

https://bugs.webkit.org/attachment.cgi?id=323720&action=review




--- Comment #7 from Dean Jackson <dino at apple.com> ---
Comment on attachment 323720
  --> https://bugs.webkit.org/attachment.cgi?id=323720
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323720&action=review

I'm not sure I like the "seed" terminology, but this is ok for now. I'd prefer
it if all this state could be tracked directly inside VideoTextureCopierCV
using a separate context, but we can do that later.

>> Source/WTF/wtf/UnsafePointer.h:34
>> +template<typename T>
> 
> Couldn't this just be a void* wrapper, and not even templatized? Or do you
care about pointer type?

Yeah, I'm not sure this is needed in this bug. Maybe it could be a separate
patch and just use IOSurfaceRef for now.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.h:113
> +    UnsafePointer<IOSurfaceRef> m_lastSurface;

I think we can just use a raw IOSurfaceRef here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:264
> +    ::glBindTexture(GL_TEXTURE_2D, m_state.boundTarget(GL_TEXTURE0) ==
GL_TEXTURE_2D ? m_state.boundTexture(GL_TEXTURE0) : 0);

Since boundTexture() will return 0 if the value doesn't exist, I don't think
you need the conditional.

>
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1937
> +    m_state.textureSeedCount.add(o);

Has it been seeded at this point? I don't think so.


More information about the webkit-reviews mailing list