Bug 54330 – [chromium] Cannot use multiple accelerated 2D canvas' in the same render process

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

This is an issue with the accelerated Skia path (ENABLE(SKIA_GPU)) only.

Canvas elements residing in different pages handled by the same Render process all share one GrContext (globally allocated per process) but use different GL contexts (one SharedGraphicsContext3D per page) to draw. This appears to be breaking an assumption in the code that all canvas' using the same GrContext must draw via the same GL context too (which allows for proper state tracking).

An easy way to reproduce a failure is to run chrome in --single-process mode and in two different tabs open pages that contain a canvas element.  The first tab will work fine but the second one won't.

We should either create a new GrContext per SharedGraphicsContext3D or use a single SharedGraphicsContext3D per process.

[Mike Reed]

Working on  a patch...

[Mike Reed]

Created attachment 86781 [details]
fix for 54330

Move ownership of the GrContext into SharedGraphicsContext3D, instead of a global.

[Stephen White]

Comment on attachment 86781 [details]
fix for 54330

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

Looks ok to me, modulo nits.

> WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:177
> +    SkRefCnt_SafeAssign(m_grContext, context);

With both DrawingBuffer and SGC3D taking a ref on the GrContext, ownership semantics are a bit muddy.  If jamesr can confirm that the DrawingBuffer never outlives the SGC3D, we could use a bare pointer, and not take a ref here.

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:469
> +        m_grContext->setTextureCacheLimits(512, 50 * 1024 * 1024);

These magic numbers should probably be in a constant whose name explains their meaning.

[Vangelis Kokkevis]

Comment on attachment 86781 [details]
fix for 54330

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

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:738
> +        drawingBuffer->setGrContext(gr);

I realize that calling resetContext() here isn't the right spot but given that the underlying GC3D context is now (by definition) shared, shouldn't all the clients be calling resetContext() every time before the GrContext is used?  Should that be encapsulated in SGC3D::getGrContext() ?

[Mike Reed]

I have done some testing of multiple uses, and didn't see a need for the resetContext() call. I propose we add it only when we see it fix a bug (to my mind, it should get called when a client knowingly changes the gl state only).

If we get confirmation that DrawingBuffers always die before the SharedGraphicsContext3D that created the grcontext, then I'll gladly simplify the setter. I'll leave it reference counted until then.

Good point about the texture cache limits. I'll add comments (of course stating that I just made up those values :)

[Mike Reed]

Created attachment 86811 [details]
comment two parameters to setTextureCacheLimits

[James Robinson]

Comment on attachment 86811 [details]
comment two parameters to setTextureCacheLimits

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

DrawingBuffer holds a RefPtr to its associated GraphicsContext3D, so the GraphicsContext3D will never be destroyed before the DrawingBuffer.

Does this code assuming that the GrContext is the only user of the SharedGraphicsContext3D?

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:53
> +// limit the number of textures we hold in the bitmap->texture cache
> +#define GR_MAX_TEXTURE_COUNT    512
> +// limit the bytes allocated toward textures in the bitmap->texture cache

Comments in WebKit should start with a capital letter and end with a period.

Also, we typically use static variables of the appropriate type (size_t here) instead of #defines for stuff like this.

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:474
> +        m_grContext->setTextureCacheLimits(GR_MAX_TEXTURE_COUNT,
> +                                           GR_MAX_TEXTURE_BYTES);

unnecessary line wrap

[Mike Reed]

Created attachment 86815 [details]
Capitalize comments, use static consts, remove LF

[James Robinson]

Comment on attachment 86815 [details]
Capitalize comments, use static consts, remove LF

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

Style still isn't quite right, and there are still two unanswered questions:
1.) Given that the lifetime of the GraphicsContext3D is >= the DrawingBuffer, do we need the refcounting on the GrContext here?
2.) Does this code assume that ganesh is the exclusive user of SharedGraphicsContext3D?  That's not really consistent with the intended design of SGC3D (after all it's supposed to be shared by multiple cooperative users).

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:54
> +// Limit the number of textures we hold in the bitmap->texture cache
> +static const int cMaxTextureCacheCount = 512;
> +// Limit the bytes allocated toward textures in the bitmap->texture cache
> +static const size_t cMaxTextureCacheBytes = 50 * 1024 * 1024;

missing trailing periods on the comments.

we don't use the "c" or any other prefix for static constants in WebKit.

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:143
> +    GrContext* getGrContext();

Accessors in WebKit do not use a 'get' prefix - this should be 'grContext()'.  See http://www.webkit.org/coding/coding-style.html.

[Mike Reed]

Created attachment 86824 [details]
rename getter (remove get...), take unreferenced ptr in DrawingBuffer, add resetContext(), remove cPrefix to static consts.

[James Robinson]

Comment on attachment 86824 [details]
rename getter (remove get...), take unreferenced ptr in DrawingBuffer, add resetContext(), remove cPrefix to static consts.

Seems OK.

This patch was created from within Source/ instead of from the root of the WebKit repo (which you seem to do often).  Please try to cut that out, it makes reviewing harder.

[Vangelis Kokkevis]

Comment on attachment 86824 [details]
rename getter (remove get...), take unreferenced ptr in DrawingBuffer, add resetContext(), remove cPrefix to static consts.

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

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:738
>          gr->resetContext();

I'm not sure that this is the right spot to call resetContext().  Doesn't reset need to be called before you start using Ganesh if you don't how what state the underlying GL context is in?   Like I mentioned in my previous comments, at this point Ganesh will be the only client of the shared context and therefore it's safe to do this.  I just wanted us to have a comment somewhere mentioning that there will be unexpected behavior if the shared GC3D is used elsewhere as well.

[WebKit Commit Bot]

Comment on attachment 86824 [details]
rename getter (remove get...), take unreferenced ptr in DrawingBuffer, add resetContext(), remove cPrefix to static consts.

Clearing flags on attachment: 86824

Committed r81915: <http://trac.webkit.org/changeset/81915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81915 might have broken Windows 7 Release (Tests)
The following tests are not passing:
fast/images/embed-image.html