Performance: Skip texture upload if source image and destination texture haven't changed
<rdar://problem/34968181>
Created attachment 323653 [details] Patch for landing
Created attachment 323676 [details] Patch
Created attachment 323720 [details] Patch
Attachment 323720 [details] did not pass style-queue: ERROR: Source/WTF/wtf/UnsafePointer.h:40: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/UnsafePointer.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 323720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323720&action=review > 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?
Comment on attachment 323720 [details] 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.
(In reply to Dean Jackson from comment #7) > Comment on attachment 323720 [details] > 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. I'd prefer that too, but that's going to be a Buch > >> 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? I think you'd 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. I'm concerned that someone might try to actually call something on it in the future and cause a use-after-free security bug. So I'd prefer to leave this in. > > 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. It's not just checking whether the value exists, but also that the target is TEXTURE2D, so I think you do 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. I'd like to keep separate the concepts of "this texture exists and has a seed of 0" from "this texture does not exist and has a seed of 0". Making the former start at "1" means those two states are distinguishable.
(In reply to Jer Noble from comment #8) > (In reply to Dean Jackson from comment #7) > > Comment on attachment 323720 [details] > > 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. > > I'd prefer that too, but that's going to be a Buch ...bunch more work before it's technically possible. This is, I think, a good short term performance fix.
Created attachment 323754 [details] Patch for landing
Attachment 323754 [details] did not pass style-queue: ERROR: Source/WTF/wtf/UnsafePointer.h:40: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/UnsafePointer.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 323754 [details] Patch for landing Clearing flags on attachment: 323754 Committed r223315: <https://trac.webkit.org/changeset/223315>
All reviewed patches have been landed. Closing bug.