Bug 177903 – Lots of missing frames in YouTube360 when fullscreen on MacBook

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Lots of missing frames in YouTube360 when fullscreen on MacBook

[Dean Jackson]

Created attachment 322747 [details]
Patch

[Sam Weinig]

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)

[Simon Fraser (smfr)]

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?

[Jon Lee]

<rdar://problem/33273300>

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 322848 [details]
Patch

[Dean Jackson]

Created attachment 322849 [details]
Patch

[Dean Jackson]

Committed r222951: <http://trac.webkit.org/changeset/222951>

[Ryan Haddad]

(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

[Ryan Haddad]

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

[Ryan Haddad]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 177992

[Dean Jackson]

Committed r222961: <http://trac.webkit.org/changeset/222961>