Integration of the xrender pipeline rewite

Hello again,

I am sorry, but currently I can't spend a lot of time working on the
integration of the xrender pipeline.
I am well aware the code isn't production ready, and sometimes just
plain ugly ;)

There are some global changes, most touching glyph handling, because
the xrender glyph cache logic is written in plain java, and e.g. it
wasn't possible to get notifications of glyph disposal on pure java
code as far as I have seen.
It would be cool to get comments on that stuff first, the other things
could follow piece by piece later.
I did some changes in the OGL pipeline, but I don't have windows, so
D3D changes are missing, as well as the proprietary scaler
modifications.

The webrev is located at: http://93.83.133.214/glyph_changes_001.zip

The changes are in:
> StrikeCache.java
> fontscalerdefs.h
> freetypeScaler.c
> sunFont.c
> OGLTextRenderer.c
> GlyphDisposedListener.java

The only overhead I can see if the additional locking in freeXMemory,
the structure itself should remain at the same size (using otherwise
unused padding).

Any idea wether this requirement could be solved more elegant / efficient?

Thank you in advance, Clemens

[phil.race]

I think the main barrier to pushing this is not so much the pipeline code itself,
and whether it works 100% (since it would be off by default) but
making sure it is architecturally sound and doesn't break anything that exists.

I have a very few superficial comments from my quick look at a few files
that are *changed*.
X11SurfaceData and the glyph caching changes need a slightly longer
look. I do note we'd need to make some closed source changes to
match some of it.

1) src/share/classes/sun/font/FontManager.java
   shows that this isn't synced with Roman's refactoring.
   Shouldn't be a big deal though as you only change
one line and I don't think even that line should be changed
where you changed an array size from 20 to 50 :
 >private static CompositeFont [] compFonts = new CompositeFont[50];

I can't guess how x render defines new logical fonts.

2) src/share/classes/sun/java2d/pipe/AAShapePipe.java
imports sun.java2d.pisces.*
but doesn't (and shouldn't) do this directly, and
also has new CairoRenderingEngine(); //.getInstance();

I'm not sure what's intended here. Obviously it
shouldn't directly instantiate that here, but also
I don't even see this class in the webrev.

3)  src/share/classes/sun/java2d/pipe/RenderQueue.java
the size change from 32000 to 12000 probably should be explained :
   75     private static final int BUFFER_SIZE = 12000;

4) src/share/classes/sun/java2d/pisces/PiscesRenderingEngine.java
This is just adding white space ..

5) The makefiles introduce a compile time dependency on libxrender.so
   which I believe should be fine for any *nix platform we'd consider
   supporting in JDK 7+ and should also be OK on our build platforms.

6) src/solaris/native/sun/awt/awt_GraphicsEnv.c

What was wrong with using the #define here ?

  969     if (xerr->minor_code == X_ShmAttach) {
->
  969     if (xerr->minor_code == 1) {

-phil.

Clemens Eisserer wrote:

> Hello again,
>
> I am sorry, but currently I can't spend a lot of time working on the
> integration of the xrender pipeline.
> I am well aware the code isn't production ready, and sometimes just
> plain ugly ;)
>
> There are some global changes, most touching glyph handling, because
> the xrender glyph cache logic is written in plain java, and e.g. it
> wasn't possible to get notifications of glyph disposal on pure java
> code as far as I have seen.
> It would be cool to get comments on that stuff first, the other things
> could follow piece by piece later.
> I did some changes in the OGL pipeline, but I don't have windows, so
> D3D changes are missing, as well as the proprietary scaler
> modifications.
>
> The webrev is located at: http://93.83.133.214/glyph_changes_001.zip
>
> The changes are in:
>> StrikeCache.java
>> fontscalerdefs.h
>> freetypeScaler.c
>> sunFont.c
>> OGLTextRenderer.c
>> GlyphDisposedListener.java
>
> The only overhead I can see if the additional locking in freeXMemory,
> the structure itself should remain at the same size (using otherwise
> unused padding).
>
> Any idea wether this requirement could be solved more elegant / efficient?
>
> Thank you in advance, Clemens

Hi Phil,

Thanks for taking a quick look.
Seems the webrev  is in a rather chaotic state, I'll try to clean it
up a bit to make review easier.

> I have a very few superficial comments from my quick look at a few files
> that are *changed*.
> X11SurfaceData and the glyph caching changes need a slightly longer
> look. I do note we'd need to make some closed source changes to
> match some of it.
Yes, the glyph caching changes and the modifications to X11SurfaceData
are basically the modifications I think would be
best included first.

Most of the other stuff hasn't even been reviewed by myself.

> 1) src/share/classes/sun/font/FontManager.java
>  shows that this isn't synced with Roman's refactoring.
>  Shouldn't be a big deal though as you only change
> one line and I don't think even that line should be changed
> where you changed an array size from 20 to 50 :
>>private static CompositeFont [] compFonts = new CompositeFont[50];
>
> I can't guess how x render defines new logical fonts.
Argh, I thought I did a sync with the 2d-repo. I really don't like Mercurial ;)
I remember I did that change, because without it I gout
ArrayIndexOutOfBoundsExceptions on my system,
independent from my changes. I just tried to resize the Array without
looking a lot why or what, and it worked.

Sorry, I forgot about that change, its not ment to go upstream.


> 2) src/share/classes/sun/java2d/pipe/AAShapePipe.java
> imports sun.java2d.pisces.*
> but doesn't (and shouldn't) do this directly, and
> also has new CairoRenderingEngine(); //.getInstance();
>
> I'm not sure what's intended here. Obviously it
> shouldn't directly instantiate that here, but also
> I don't even see this class in the webrev.
Sorry, this is from my experiments replacing Pisces with Cairo:
http://linuxhippy.blogspot.com/2009/09/ductus-vs-cairo-vs-pisces.html

Because it requires a slightly modified version of cairo, which I
guess would cause build-system troubles, I don't think its ready for
upstream too.

> 3)  src/share/classes/sun/java2d/pipe/RenderQueue.java
> the size change from 32000 to 12000 probably should be explained :
>  75     private static final int BUFFER_SIZE = 12000;
Accidential.

> 4) src/share/classes/sun/java2d/pisces/PiscesRenderingEngine.java
> This is just adding white space ..
Sorry.

> 6) src/solaris/native/sun/awt/awt_GraphicsEnv.c
>
> What was wrong with using the #define here ?
>
>  969     if (xerr->minor_code == X_ShmAttach) {
> ->
>  969     if (xerr->minor_code == 1) {

Gave me a compile-time problem on Rawhide, the IcedTea guys said hard
to fix, so I worked arround it.

- Clemens

Hi again,

I've uploaded a new version, which includes some fixes for the
problems you mentioned, including one fix from icedtea using static
initializers for X11SurfaceData. Would be great if you could have
another look.

http://93.83.133.214/xrender_002.zip

Thanks, Clemens

[gnu_andrew]

2009/10/30 Clemens Eisserer <linuxhippy@...>:

> Hi again,
>
> I've uploaded a new version, which includes some fixes for the
> problems you mentioned, including one fix from icedtea using static
> initializers for X11SurfaceData. Would be great if you could have
> another look.
>
> http://93.83.133.214/xrender_002.zip
>
> Thanks, Clemens
>

Hi Clemens,

How close is what you have to what we have in IcedTea?  I'm thinking
it might be a good idea to have your XRender work in the IcedTea
forest rather than as patches in IcedTea7.  That would give you a
clean webrev against the latest JDK7 code.
--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Hi Andrew,

> How close is what you have to what we have in IcedTea?  I'm thinking
> it might be a good idea to have your XRender work in the IcedTea
> forest rather than as patches in IcedTea7.  That would give you a
> clean webrev against the latest JDK7 code.

Its a complete rewrite and doesn't share much code with what is in IcedTea.
I'll try to update to the lastest JDK7 code from the mercurial repos
later today, after backing that stuff up ;)

- Clemens

[gnu_andrew]

2009/10/30 Clemens Eisserer <linuxhippy@...>:

> Hi Andrew,
>
>> How close is what you have to what we have in IcedTea?  I'm thinking
>> it might be a good idea to have your XRender work in the IcedTea
>> forest rather than as patches in IcedTea7.  That would give you a
>> clean webrev against the latest JDK7 code.
>
> Its a complete rewrite and doesn't share much code with what is in IcedTea.
> I'll try to update to the lastest JDK7 code from the mercurial repos
> later today, after backing that stuff up ;)
>
> - Clemens
>

So I should just ditch what we have in IcedTea7 and use this webrev?
I had to reroll all the patches for b74 anyway so get rid of that
maintenance headache would be nice.
--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

Hi,

Thanks for all your work, integrating and maintaining the pipeline.

> So I should just ditch what we have in IcedTea7 and use this webrev?
> I had to reroll all the patches for b74 anyway so get rid of that
> maintenance headache would be nice.

Yes, in the long run I hope this code will replace what is currently in IcedTea.
However the rewrite has more impact on independent parts (like the
StrikeCache changes), so even with the pipeline disabled it could
break some stuff. Thats why some review would be great...

- Clemens

Hello,

I've merged the xrender pipeline with master, please take a look at the webrev:
http://93.83.133.214/webrev-xrender-jules-0.0.1.zip

It would be great to get the review-process rolling for the critical
parts like StrikeCache.java or sunFont.c

It also includes a preview of "Jules", a cairo based RenderingEngine
implementation:
Because of its complex build-system and the need for a modified
version of cairo I've seperate the native components and build them
indepent from OpenJDK - falling back to pisces when loading fails.

The native part can be found at:
http://93.83.133.214/jules-0.0.1.tar.gz

Simply copy the resulting libjules.so into lib/i386, and activate it
on the command-line:
-Dsun.java2d.renderer=sun.java2d.jules.JulesRenderingEngine

Jules is more or less proof-of-concept, especially the native code is
ugly, full of dirty assumptions and its probably not 64-bit clean.
(Although it should get a major performance boost, in the case it
works ;) ).
However it runs Java2Demo quite well, and usually is a good deal
faster than pisces even when rendering to software-surfaces.

Known problems:
- Some clipping problems when rendering to software surfaces
- Paints get wrong transformation when rendering to XRender surfaces

Thanks, Clemens

PS: Is there an easy way to apply a webrev to a local repository?

[Dmitri Trembovetski]

 > PS: Is there an easy way to apply a webrev to a local repository?

   The webrev/ dir contains a patch, you could do
     patch -p1 -i webrev/stuff.patch

   Thanks,
     Dmitri



Clemens Eisserer wrote:

> Hello,
>
> I've merged the xrender pipeline with master, please take a look at the webrev:
> http://93.83.133.214/webrev-xrender-jules-0.0.1.zip
>
> It would be great to get the review-process rolling for the critical
> parts like StrikeCache.java or sunFont.c
>
> It also includes a preview of "Jules", a cairo based RenderingEngine
> implementation:
> Because of its complex build-system and the need for a modified
> version of cairo I've seperate the native components and build them
> indepent from OpenJDK - falling back to pisces when loading fails.
>
> The native part can be found at:
> http://93.83.133.214/jules-0.0.1.tar.gz
>
> Simply copy the resulting libjules.so into lib/i386, and activate it
> on the command-line:
> -Dsun.java2d.renderer=sun.java2d.jules.JulesRenderingEngine
>
> Jules is more or less proof-of-concept, especially the native code is
> ugly, full of dirty assumptions and its probably not 64-bit clean.
> (Although it should get a major performance boost, in the case it
> works ;) ).
> However it runs Java2Demo quite well, and usually is a good deal
> faster than pisces even when rendering to software-surfaces.
>
> Known problems:
> - Some clipping problems when rendering to software surfaces
> - Paints get wrong transformation when rendering to XRender surfaces
>
> Thanks, Clemens
>