Hi Phil,
Thanks for taking the time to review the code.
I uploaded the new/resulting webrev to:
http://93.83.133.214/webrev-xrender-jules-0.0.2.zip
> The "Jules" cairo pisces replacement obviously isn't all that useful
> without the backend .. is it likely the required cairo modifications
> would ever make it into linux distros ? If not I'm not sure its
> worth putting the java glue code into openjdk.
No, the modifications won't make it into cairo, I already talked with the cairo
maintainers about that.
However the only difference between using a forked version of cairo
and writing a new rasterizer from scratch is that the cairo-code can't
be under Sun's
SCA.
At least when looking at benchmarks, Jules does well when used as
AATileGenerator,
and it shines when used in conjunction with the XRender pipeline for
antialiased rendering.
XRender implementations usually dont handle uploading those small AA
tile masks generated by Ductus well.
> Dimitri's suggestion
> is to have build code in the make files that looks for the files
> and builds them if they are there. This would make it easier for
> people to get those sources, drop them in and experiment
I really like that idea.
My hope is that Jules will be shipped by distributors, so this would
fit my goal best :)
> Aside from that I think the biggest concern I have is to understand
> the changes to glyph caching. I don't think its necessary to change
> it at least for any existing pipeline. I think that's why you only
> call StrikeCache.addGlyphDisposeListener in XRGlyphCache.java
Right, only XRGlyphCache uses the DisposeListener.
> ............
> So far so good except that I don't see how the requirements
> for xrender are that much different from opengl - we also
> there have a limited size texture and need to manage the
> contents of that. I'd like to understand better the inadequacies
> of that code or mismatch in the requirements. Near as I can
> tell its that you are counting pixels whereas we count cells?
I already had a private glyph-cache implementation in the "old"
xrender pipeline.
As far as I can remember this is because the pipeline doesn't handle
fallbacks because
exhausted cache space (this is done by the xserver).
The pipeline depends on the fact that all glyphs used in a
DrawGlyphList() call are cached.
The pixel-couting isn't a strict limit, only a hint how much should
stay cached. All the low-level details are handled by the xserver
anyway.
One of the goals of the new pipeline was to write as much as possible
in java (partly to avoid JNI, partly to make maintenance and porting
easier).
> So we have a new per glyph variable "managed" which could use
> some documentation on its meaning.
> I think its something like " 1 means this glyph has a hardware cached
> copy and its freeing is managed by the usual 2D disposer code
> managed by GC and reference queues. A value of 0 means its either
> unaccelerated (and so has no cellInfos) or we want to free this
> in a different way.
Exactly. If managed=0 and CellInfo!=0 the entry is managed by the
XRender glyph cache.
The drawback is that all code which uses AccelGlyphCache has to set managed=1.
The whole thing is about getting notified when GlyphInfo's are deleted
by a GC, so that the native cache can be flused as well.
I'll add a comment.
> So in freetypeScaler.c
>
> lines 785 : you set managed = 0.
> Looks like this maybe just initialisation to non-garbage although
> 0 which is part of the trigger to make the existing glyph caching code know
> to use your new disposer code instead.
Yes
> I see you also made a change in AccelGlypheCache.c so that
> (eg) OpenGL accelerated glyphs will set managed = 1
> The Xrender code doesn't use this function.
Yes
> Then in sunFont.c we call RemoveAllCellInfos only for
> the "managed" ones (managed != 0)
Yes, because for the XRender glyph's are java objects, so nothing to free here.
> Next in StrikeCache.java if you detect an Xrender glyph
> (implied by managed==0 and there being a cached cell pointer)
> its added to a list to be disposed by Xrender's listener or its
> delegate.
Right.
> BTW I see you inserted this "managed" byte into the GlyphInfo struct
> in fontscalerdefs.h at a point where the compiler probably had
> some padding. So it probably doesn't matter if its there even
> if we don't use this field on windows, as it isn't
> increasing the size of the struct. Perhaps you could
> add a comment to that effect.
Yes, I made sure it uses padding and the structures doesn't grow.
I'll add a comment on this.
> I suppose I'll find out later why you changed
> 95 struct _CacheCellInfo *cellInfo;
> to void*
I am not really used to C, I mis-use a pointer as integer anyway so I
guess changing the type to (void*) doesn't make any sence.
The XRender pipeline uses it to store an integer, which is used to map
a native GlyphInfo structure to a XRGlyphCache java object using a
HashMap.
> - in src/share/classes/sun/font/StrikeCache.java
> I'd create disposeListeners() only for the xrender pipe.
> Then there's no need to synchronize on that variable
> if its null because you don't even enter the block.
Well, I am no threading expert.
Couldn't that lead to troubles if the list is initialized by the
XRender pipeline-thread, but e.g. freeCachedIntMemory is called from
another Thread?
In this case the freeCachedIntMemory-thread could still see
disposeListeners==null, although it has already been initialized?
DONE:
-----------------------------------------------
Ok, I moved the function into the static block.
Got rid of the extra indention by combinding both conditions into a single if().
X11SurfaceData.java
210 static {
211 initX11SurfaceData();
212 }
213
214 private static void initX11SurfaceData()
215 {
216 if(!X11GraphicsEnvironment.isX11SurfaceDataInitialized()) {
217
218 if (!GraphicsEnvironment.isHeadless()) {
I'm not sure but it looks like you were trying to avoid extra indentation here,
and probably re-breaking all the lines. And I not convinced it was necessary
to introduce the function since its only used in the class static
initialiser, but if you do, you could redo it like
private static void initX11SurfaceData() {
if (X11GraphicsEnvironment.isX11SurfaceDataInitialized()) {
return;
}
/// ... the rest of the code goes here
}
static {
initX11SurfaceData();
}
------------------------------------------------------------------------------------------------
Those two were caused by using IcedTea's mecurial repository as base.
I've merged the code now with a fresh pull of jdk7's main repo.
awt_GraphicsEnv.c
This fix is already in now courtesy of Andrew Hughes :
http://cr.openjdk.java.net/~andrew/xshm/webrev.01/
freetypeScaler.c
lines 131-134
It seems this free of the stream isn't specific to Xrender, its
a bug fix? Nothing wrong with that .. just noting it.
> freetypeScaler.c
> 1134 /* Bit 1 is meaningful for �off� points only.
> What happened here? Looks like your editor locale settings caused
> a changed to the apostropheses.
What is the default encoding for the openjdk sources.
No matter with which editor I try to correct this (eclipse or kwrite),
I always get that result :-/
------------------------------------------------------------------------------------------------
Thanks, should be fixed now:
Style nit pick :
171 if(xrenderSupported) {
175 }else {
JDK convention (and what I prefer too) is that there always
be a space before/after the keyword, ie "if (..)"
and "} else" not "}else". It helps readability.
------------------------------------------------------------------------------------------------------
Forgot those remainders, should all be removed now.
src/solaris/classes/sun/awt/X11GraphicsEnvironment.java
Is this line (and others like it) a reminder of some sort ?
110 /*CE*/
----------------------------------------------------------------------------------------------------
Thanks, no idea how that came in:
src/solaris/classes/sun/awt/X11GraphicsDevice.java
137 return configs.clone();
->
138 return configs;
removing that clone looks to me as if it its introducing
a vulnerability so that untrusted apps can mutate the internal
graphics configs array, presumably causing some kind of mayhem.
---------------------------------------------------------------------------------------------------
