Bug 33405 – [OpenVG] Implement a basic GraphicsContext on top of a new PainterOpenVG class

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Jakob Petsovits]

[OpenVG] Implement a basic GraphicsContext on top of a new PainterOpenVG class

[Jakob Petsovits]

Created attachment 46165 [details]
Patch

[Jakob Petsovits]

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.

[Jakob Petsovits]

Created attachment 46166 [details]
Patch

[Dirk Schulze]

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?

[Jakob Petsovits]

> 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.

[Eric Seidel (no email)]

Looks like this patch fails to apply.  Is that expected?

[Jakob Petsovits]

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.)

[Jakob Petsovits]

Comment on attachment 46166 [details]
Patch

r- to clear up the review queue until bug 33403 is landed.

[Jakob Petsovits]

Created attachment 47172 [details]
Patch

[Jakob Petsovits]

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!

[Nikolas Zimmermann]

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 :-)

[Jakob Petsovits]

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).

[Jakob Petsovits]

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.

[Nikolas Zimmermann]

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.

[Jakob Petsovits]

Committed r54063: <http://trac.webkit.org/changeset/54063>

[Jakob Petsovits]

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.