[Webkit-unassigned] [Bug 108899] Coordinated Graphics : Refactor GraphicsSurface and ShareableBitmap to be responsible for their GraphicsContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108899





--- Comment #20 from Jae Hyun Park <jae.park at company100.net>  2013-02-25 18:09:05 PST ---
I've been working on this bug for a while now, and I still don't see a proper direction.

The purpose of this refactoring is to use ThreadSafeCoordinatedSurface (which will be used in Threaded Coordinated Graphics). I am planning to implement ThreadSafeCoordinatedSurface to use GraphicsSurface and ImageBuffer as a backend (WIP patch https://bugs.webkit.org/show_bug.cgi?id=109661). Since ImageBuffer only provides GraphicsContext* to get GraphicsContext whereas ShareableBitmap and GraphicsSurface provides PassOwnPtr<GraphicsContext>, a refactoring was needed.

Over the past few weeks, the reviewers, noamr, benjaminp, and smfr, have suggested several ways. I have tried them all but each had problems (some were big, some were trivial).

During the refactoring, I found an abnormal behavior in which GraphicsContext is created twice through recursive calls before releasing even one GraphicsContext (this behavior occurs only in iframe cases). The call stack can be found in https://bugs.webkit.org/show_bug.cgi?id=108899#c18. Due to this behavior, the class that manages GraphicsContext, whether it be GraphicsSurface, ShareableBitmap, or even CoordinatedSurface, must hold GraphicsContext in a stack, which is pretty ugly. 

I have tried 3 different approaches in refactoring.
1. GraphicsContextHolder pattern.
-> Rejected by noamr, since "holder" pattern makes the code hard to read.

2. beginPaint & endPaint pattern where CoordinatedSurface manages GraphicsContext.
-> Rejected by benjaminp. CoordinatedSurface managing GraphicsContext is not reasonable.

3. beginPaint & endPaint pattern where GraphicsSurface & ShareableBitmap manages GraphicsContext.
-> Rejected by smfr. GraphicsSurface and ShareableBitmap managing GraphicsContext as a stack is unreasonable. Also, beginPaint and endPaint pattern is burdensome.

More importantly, all three cases must hold GraphicsContext in a stack due to the abnormal behavior I mentioned above.

I have dug further about this behavior, and also discussed with noamr and dshuang about it. Noam thought this can be solved by triggering a flush directly using TBS timer instead of triggering a paint for the particular tile. However, since Coordinated Graphics doesn't use TBS timer, I think this cannot be a solution. The problem of this is that two different CoordinatedTile can use the same UpdateAtlas. 

I thought about the ways to move this patch forward.

1. Add an API in ImageBuffer that creates a new GraphicsContext and returns as PassOwnPtr<GraphicsContext> (like current behavior in GraphicsSurface).
-> This is the most proper approach that I can think of right now, even though it may not be the best approach.

2. Make CoordinatedTile use separate UpdateAtlas.
-> This is practically impossible since it will dramatically increase the memory usage.

3. Allow GraphicsContext to be managed in Vector form.
-> IMHO, this is unacceptable since it is clearly wrong.

4. Forbid sub-layers to flush when the root layer is performing flush.
-> The GraphicsContext is created 2 times in a row before even releasing one because the root layer flush is calling the sub layers to flush. However, I'm not even sure if this behavior is wrong. As I mentioned above, I think the problem is different CoordinatedTile sharing an UpdateAtlas. 

I could really use some help. Please leave comments/thoughts. 

Thanks.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.