Bug 54299 – need makeContextCurrent() called in prepareForSoftwareDraw()

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Mike Reed]

when skia is backed by a gpu, we need to ensure that the current gl context is always set before we render.

[Mike Reed]

Created attachment 82154 [details]
fix for 54299 -- call makeContextCurrent() in prepareForSoftwareDraw() when Skia is backed by the GPU

[Mike Reed]

Created attachment 82157 [details]
fix weird loss of comments in ChangeLog

[James Robinson]

I don't totally understand this patch - why doesn't the code issuing the GL calls (in this case skia) take care of setting the current context?

[Mike Reed]

The library explicitly relies on its client to take care of that, since creation of management of clients is platform dependent, and clients of ganesh also can use the gpu directly (ganesh is just there to translate 2D into gpu calls).

[Mike Reed]

As a 2nd note, the existing GraphicsContext3DOpenGL.cpp issues this call before every gl... call. This patch follows the same logical pattern, but happens to be a bit more efficient, in that we only issue the call before each high-level 2D primitive, which may turn into may gl... calls.

[James Robinson]

Comment on attachment 82157 [details]
fix weird loss of comments in ChangeLog

Makes sense.  I think the confusing part is that prepareForSoftwareDraw() is misnamed - what it really means in this context is prepareForSkiaDraw(), as opposed to the h/w drawing backend.

[Vangelis Kokkevis]

Comment on attachment 82157 [details]
fix weird loss of comments in ChangeLog

I think we need to make the same change to ::prepareForHardwareDraw() as well.  In addition we need to replace:
#if ENABLE(SKIA_GPU) by #if ENABLE(ACCELERATED_2D_CANVAS) && ENABLE(SKIA_GPU)

[James Robinson]

(In reply to comment #7)
> (From update of attachment 82157 [details])
> I think we need to make the same change to ::prepareForHardwareDraw() as well.  In addition we need to replace:
> #if ENABLE(SKIA_GPU) by #if ENABLE(ACCELERATED_2D_CANVAS) && ENABLE(SKIA_GPU)

prepareForHardwareDraw() is never reached in the skia GPU path

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 82157 [details]:

http/tests/websocket/tests/alert-in-event-handler.html bug 54316 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 82157 [details]
fix weird loss of comments in ChangeLog

Clearing flags on attachment: 82157

Committed r78377: <http://trac.webkit.org/changeset/78377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Vangelis Kokkevis]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 82157 [details] [details])
> > I think we need to make the same change to ::prepareForHardwareDraw() as well.  In addition we need to replace:
> > #if ENABLE(SKIA_GPU) by #if ENABLE(ACCELERATED_2D_CANVAS) && ENABLE(SKIA_GPU)
> 
> prepareForHardwareDraw() is never reached in the skia GPU path

Good point! But then we have another problem... prepareForSoftwareDraw() isn't called before every call that renders via platformContext()->canvas() which means that there are calls  (e.g. GraphicsContext::clearRect()) which won't set the current GL context correctly.