Bug 54299 - need makeContextCurrent() called in prepareForSoftwareDraw()
Summary: need makeContextCurrent() called in prepareForSoftwareDraw()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-11 11:28 PST by Mike Reed
Modified: 2011-02-11 15:22 PST (History)
6 users (show)

See Also:


Attachments
fix for 54299 -- call makeContextCurrent() in prepareForSoftwareDraw() when Skia is backed by the GPU (1.26 KB, patch)
2011-02-11 11:33 PST, Mike Reed
no flags Details | Formatted Diff | Diff
fix weird loss of comments in ChangeLog (1.43 KB, patch)
2011-02-11 11:39 PST, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-02-11 11:28:06 PST
when skia is backed by a gpu, we need to ensure that the current gl context is always set before we render.
Comment 1 Mike Reed 2011-02-11 11:33:12 PST
Created attachment 82154 [details]
fix for 54299 -- call makeContextCurrent() in prepareForSoftwareDraw() when Skia is backed by the GPU
Comment 2 Mike Reed 2011-02-11 11:39:24 PST
Created attachment 82157 [details]
fix weird loss of comments in ChangeLog
Comment 3 James Robinson 2011-02-11 11:44:54 PST
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?
Comment 4 Mike Reed 2011-02-11 12:50:08 PST
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).
Comment 5 Mike Reed 2011-02-11 13:01:21 PST
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.
Comment 6 James Robinson 2011-02-11 14:19:30 PST
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.
Comment 7 Vangelis Kokkevis 2011-02-11 14:36:13 PST
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)
Comment 8 James Robinson 2011-02-11 14:50:57 PST
(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
Comment 9 WebKit Commit Bot 2011-02-11 15:04:12 PST
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: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2011-02-11 15:05:42 PST
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>
Comment 11 WebKit Commit Bot 2011-02-11 15:05:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Vangelis Kokkevis 2011-02-11 15:22:46 PST
(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.