Lots of missing frames in YouTube360 when fullscreen on MacBook
Created attachment 322747 [details] Patch
Comment on attachment 322747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322747&action=review > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:49 > + int _width; > + int _height; IntSize? > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:75 > + [super setTransform:CATransform3DScale(t, 1., -1., 1.)]; This can be CATransform3DScale(t, 1, -1, 1)
Comment on attachment 322747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322747&action=review > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:678 > + LOG(WebGL, "Tell the WebGL layer to create IOSurface backing store. (%p)", this); Would make more sense to just log the function name, size, and m_attrs.alpha. > Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:684 > + LOG(WebGL, "Tell the WebGL layer to bind to the framebuffer. (%p)", this); Log the function name > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:47 > + std::unique_ptr<WebCore::IOSurface> _contentsBuffer; > + std::unique_ptr<WebCore::IOSurface> _drawingBuffer; > + std::unique_ptr<WebCore::IOSurface> _spareBuffer; What about frontBuffer, backBuffer and secondaryBackBuffer as we use in RemoteLayerBackingStore? >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:49 >> + int _height; > > IntSize? Yes. > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:59 > + self.transform = CATransform3DIdentity; Is this necessary? > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:68 > +// means that any incoming transform (unlikely, since this Unlikely, or impossible? >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:75 >> + [super setTransform:CATransform3DScale(t, 1., -1., 1.)]; > > This can be CATransform3DScale(t, 1, -1, 1) Seems odd to do this as an override. Maybe the init method should just set the transform and anchor point? What happens with object-fit on a WebGL? Is the WebGLLayer itself just a content layer? > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:149 > +static std::unique_ptr<WebCore::IOSurface> createAppropriateIOSurface(int width, int height) "Appropriate" in what sense? > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:152 > + auto surface = WebCore::IOSurface::create(IntSize(width, height), sRGBColorSpaceRef()); > + return surface; No need for the local variable. > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:155 > +- (void)createIOSurfaceBackingStoreWithWidth:(int)width height:(int)height usingAlpha:(BOOL)usingAlpha A function called "create" is normally expected to return a retained object. > Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:181 > +#ifndef NDEBUG This should be ASSERTIONS_ENABLED, but probably cleaner to just ASSERT_UNUSED() > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:78 > + for (int i = 0; i < totalBytes; i += 4) > + pixels[i + 3] = 255; Seems like Accelerate might have a faster way to do this? I assume the buffers never have rowByte padding?
<rdar://problem/33273300>
Comment on attachment 322747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322747&action=review >> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:678 >> + LOG(WebGL, "Tell the WebGL layer to create IOSurface backing store. (%p)", this); > > Would make more sense to just log the function name, size, and m_attrs.alpha. Done. >> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:684 >> + LOG(WebGL, "Tell the WebGL layer to bind to the framebuffer. (%p)", this); > > Log the function name Done. >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:47 >> + std::unique_ptr<WebCore::IOSurface> _spareBuffer; > > What about frontBuffer, backBuffer and secondaryBackBuffer as we use in RemoteLayerBackingStore? I like my names better, because they make more sense with respect to how they are used. WebGL is drawing into a buffer that it thinks is the front. And what does secondaryBackBuffer mean? It's really a third spare buffer. >>> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:49 >>> + int _height; >> >> IntSize? > > Yes. Changed. >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:59 >> + self.transform = CATransform3DIdentity; > > Is this necessary? It is, so that we call the flipping code. I could set it directly, but then it would be unflipped by the code in setTransform :) >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:68 >> +// means that any incoming transform (unlikely, since this > > Unlikely, or impossible? Impossible. >>> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:75 >>> + [super setTransform:CATransform3DScale(t, 1., -1., 1.)]; >> >> This can be CATransform3DScale(t, 1, -1, 1) > > Seems odd to do this as an override. Maybe the init method should just set the transform and anchor point? > > What happens with object-fit on a WebGL? Is the WebGLLayer itself just a content layer? It can't be just the init method, since GraphicsLayerCA::setupContentsLayer comes along and sets an anchor point (and maybe a transform) after creation. I thought it would be better to specialize here rather than in the higher-level code. >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:149 >> +static std::unique_ptr<WebCore::IOSurface> createAppropriateIOSurface(int width, int height) > > "Appropriate" in what sense? I removed this method. >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:155 >> +- (void)createIOSurfaceBackingStoreWithWidth:(int)width height:(int)height usingAlpha:(BOOL)usingAlpha > > A function called "create" is normally expected to return a retained object. Renamed to allocate. >> Source/WebCore/platform/graphics/cocoa/WebGLLayer.mm:181 >> +#ifndef NDEBUG > > This should be ASSERTIONS_ENABLED, but probably cleaner to just ASSERT_UNUSED() FIxed.
Created attachment 322848 [details] Patch
Created attachment 322849 [details] Patch
Committed r222951: <http://trac.webkit.org/changeset/222951>
(In reply to Dean Jackson from comment #8) > Committed r222951: <http://trac.webkit.org/changeset/222951> This change failed to build on iOS EWS before it was landed. It broke the build: https://build.webkit.org/builders/Apple%20iOS%2011%20Release%20%28Build%29/builds/529
LayoutTest webgl/1.0.2/conformance/textures/copy-tex-image-2d-formats.html is also failing on macOS after this change: https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r222951%20(5013)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgl%2F1.0.2%2Fconformance%2Ftextures%2Fcopy-tex-image-2d-formats.html
I see there was an iOS build fix in http://trac.webkit.org/changeset/222952, but the build is still failing: https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20%28Build%29/builds/481 It looks like another function is outside the #if PLATFORM(MAC) block, so it should be easy to fix. Still, I am going to roll out due to the build breakage and LayoutTest regression.
Re-opened since this is blocked by bug 177992
Committed r222961: <http://trac.webkit.org/changeset/222961>