Bug 33405 - [OpenVG] Implement a basic GraphicsContext on top of a new PainterOpenVG class
Summary: [OpenVG] Implement a basic GraphicsContext on top of a new PainterOpenVG class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 33403 33959
Blocks: 33987
  Show dependency treegraph
 
Reported: 2010-01-08 14:44 PST by Jakob Petsovits
Modified: 2010-01-29 12:21 PST (History)
11 users (show)

See Also:


Attachments
Patch (55.64 KB, patch)
2010-01-08 14:46 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Patch (55.58 KB, patch)
2010-01-08 15:05 PST, Jakob Petsovits
no flags Details | Formatted Diff | Diff
Patch (57.36 KB, patch)
2010-01-21 20:35 PST, Jakob Petsovits
zimmermann: review-
Details | Formatted Diff | Diff
Implement a basic GraphicsContext on top of a new PainterOpenVG class (3.51 MB, application/octet-stream)
2010-01-28 17:03 PST, Jakob Petsovits
no flags Details
Implement a basic GraphicsContext on top of a new PainterOpenVG class (57.30 KB, patch)
2010-01-28 17:11 PST, Jakob Petsovits
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Petsovits 2010-01-08 14:44:16 PST
[OpenVG] Implement a basic GraphicsContext on top of a new PainterOpenVG class
Comment 1 Jakob Petsovits 2010-01-08 14:46:07 PST
Created attachment 46165 [details]
Patch
Comment 2 Jakob Petsovits 2010-01-08 15:01:05 PST
Patch #2 in the OpenVG graphics backend series. Depends on bug 33403 which provides the means to switch surfaces and contexts.

This patch starts to actually implement WebKit functionality. In shared files, OpenVG stuff is wrapped in PLATFORM(OPENVG), internally it also uses PLATFORM(EGL) as EGL is not strictly a hard dependency for OpenVG.

So yeah, this patch provides the painting basics including a GraphicsContextOpenVG.cpp with lots of notImplemented(), and a PainterOpenVG with platform save/restore (on top of an existing SurfaceOpenVG) that provides pretty much all of the state, but for now only a single drawing operation (drawRect(), others will follow in subsequent patches).

Also, I just noticed that diff screwed me up on the ChangeLog. Let's see if I can get a patch going that will actually apply once the other one is reviewed and landed.
Comment 3 Jakob Petsovits 2010-01-08 15:05:50 PST
Created attachment 46166 [details]
Patch
Comment 4 Dirk Schulze 2010-01-09 01:54:34 PST
Comment on attachment 46166 [details]
Patch

> +++ b/WebCore/platform/graphics/openvg/GraphicsContextOpenVG.cpp

> +void GraphicsContext::fillRect(const FloatRect& rect, const Color& color, ColorSpace colorSpace)
> +{
> +    if (paintingDisabled())
> +        return;
> +
> +    PainterOpenVG* painter = m_data->painter();
> +    Color oldColor = painter->fillColor();
> +    painter->setFillColor(color);
> +    painter->drawRect(rect, VG_FILL_PATH);
> +    painter->setFillColor(oldColor);
> +}

Is it realy neccessary to store the current fill color of the painter and reset is after the painting of the rect? The Cairo port also overwrites the current color, but we do not reset it after the drawing. But maybe it's a bug there. Do you have an example, where this is needed?
Comment 5 Jakob Petsovits 2010-01-11 07:59:53 PST
> Is it realy neccessary to store the current fill color of the painter and reset
> is after the painting of the rect?

Well, given the absence of any specification/apidox for this method (and many others in WebKit), it's not so easy to tell which of the two behaviors is standard. I read the function signature "fillRect(rect, color)" as "fill this rect with color, ignoring the current fill paint" which implies not changing the paint state permanently.

The CG port does revert to the old fill color. The Qt port seems not to. Skia does it differently, so it doesn't really apply there, but for all practical purposes it also doesn't change state. So I guess you don't really see any effects because WebKit drawing code doesn't rely a lot on the state, or doesn't use that function very often. From a semantic point of view, I find it hard to see that someone would specify a function like this with state modification included, so my suspicion is that it's rather a bug in Cairo and Qt than in CG, Skia and OpenVG.
Comment 6 Eric Seidel (no email) 2010-01-14 17:45:58 PST
Looks like this patch fails to apply.  Is that expected?
Comment 7 Jakob Petsovits 2010-01-14 19:02:33 PST
Yes, this won't apply unless the patch from bug 33403 is landed (and might need a small update after that as well). I posted it here so that people can get a better grasp of how the core of the OpenVG backends will look like, and what the other patch is actually good for.

On the other hand, I guess I can r- it to get it out of the review queue so as to not distract reviewers before they can actually r+ it. webkit-patch upload set the r? flag by itself and I didn't further think about it, sorry if it caused trouble.

(This notwithstanding, it seems posting this patch early was not the worst idea ever... I already got a suggestion for a fillRect() with solid-color paint, although that's rather something to be added in a later patch, I think.)
Comment 8 Jakob Petsovits 2010-01-14 19:03:51 PST
Comment on attachment 46166 [details]
Patch

r- to clear up the review queue until bug 33403 is landed.
Comment 9 Jakob Petsovits 2010-01-21 20:35:55 PST
Created attachment 47172 [details]
Patch
Comment 10 Jakob Petsovits 2010-01-21 20:52:39 PST
Alright, back in the game. I took some lessons from Niko's first patch review and preemptively removed a couple of method parameter names in header files, plus UNUSED_PARAM() all over the place in the new GraphicsContext implementation. I've got implementations for most of the methods in GraphicsContext, so the UNUSED_PARAM()s will go away anyways over the next couple of patches.

Also made GraphicsContextPlatformPrivate inherit from PainterOpenVG instead of having the latter contained in the former as a member. I guess that's the second best option after typedef'ing which the compiler didn't like so much.

Now waiting for reviews! Come and get me!
Comment 11 Nikolas Zimmermann 2010-01-28 15:42:19 PST
Comment on attachment 47172 [details]
Patch

r- as discussed with Jakob. We've went through the patch line-by-line, and there are some issues in GraphicsContext regarding not checking for m_data != 0 and/or surface->makeCurrent() calls missing. A new patch will be prepared :-)
Comment 12 Jakob Petsovits 2010-01-28 17:03:36 PST
Created attachment 47657 [details]
Implement a basic GraphicsContext on top of a new PainterOpenVG class

New patch, incorporating Niko's comments above (and a few more that he only told me on IRC).
Comment 13 Jakob Petsovits 2010-01-28 17:11:22 PST
Created attachment 47659 [details]
Implement a basic GraphicsContext on top of a new PainterOpenVG class

Gah. Same patch as in the comment above, but without the huge ChangeLog.orig patch artifact. Also, mark this attachment as patch so the bots can drool over it.
Comment 14 Nikolas Zimmermann 2010-01-28 18:53:06 PST
Comment on attachment 47659 [details]
Implement a basic GraphicsContext on top of a new PainterOpenVG class

r=me, nice patch! Please fix some last style issues before landing:
 330     inline bool strokeDisabled() const
 331     {
 332         return (compositeOperation == CompositeSourceOver
 333             && (strokeStyle == NoStroke || !strokeColor.alpha()));
 334     }
 335 
 336     inline bool fillDisabled() const
 337     {
 338         return (compositeOperation == CompositeSourceOver && !fillColor.alpha());
 339     }
 340 };
Outer parenthesis shoul dbe removed.

 88     void drawRect(const FloatRect&, VGbitfield paintModes = (VG_STROKE_PATH | VG_FILL_PATH));

Ditto.
Comment 15 Jakob Petsovits 2010-01-29 07:43:25 PST
Committed r54063: <http://trac.webkit.org/changeset/54063>
Comment 16 Jakob Petsovits 2010-01-29 12:21:52 PST
For all those who are interested in further OpenVG patches, I've created bug 33987 as OpenVG master bug - if you add yourself to the CC list of that one, you'll get notified about all the new patches that I'll put up for review, and whatever progress is made on the OpenVG graphics backend.