HEADS-UP: Jigsaw is coming for FX 9

[Kevin Rushforth]

We are preparing for the upcoming Jigsaw integration [1] and I wanted to 
give a brief heads-up concerning the steps leading up to this.


* As an interim step we will start building FX with a pre-Jigsaw version 
of JDK 9 (likely build 109) [2]


* I will send out a webrev soon with the changes for JDK 9 based on the 
then-current jake sandbox [3]


* I will integrate the FX 9 Jigsaw changes the same week (for the same 
build) that the Jigsaw changes go into JDK 9


The target for the integration is later this month, so we plan to do the 
first two steps within the next week: the webrev will likely go out on 
Monday and the upgrade to JDK 9 will probably be next Thursday, if we 
encounter no problems.


-- Kevin

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-March/006525.html
[2] https://bugs.openjdk.java.net/browse/JDK-8149964
[3] http://hg.openjdk.java.net/openjfx/sandbox-9-jake/rt

[9] Review request: 8151565: Exclude FX class files from javadoc classpath to workaround JDK-8151191

[Kevin Rushforth]

Chien,

Please review the following change to workaround a JDK 9 javac bug when 
generating our javadocs"


https://bugs.openjdk.java.net/browse/JDK-8151565
http://cr.openjdk.java.net/~kcr/8151565/webrev.00/

-- Kevin

Re: buffer too small

[Jim Graham]

Hi Johan,

That sounds like the fix then.

Note that there is another optimization issue here that could be 
addressed in a follow-on - basically when a larger physical size is 
allocated, then we could expand the content dimensions to match what was 
allocated.  We would possibly need a new method on the Texture to do 
something like "getMaxContentW/H()" and "setContentW/H()", and that 
could avoid reallocating a texture in that case...


...jim

On 3/9/16 2:01 AM, Johan Vos wrote:

Hi Jim,

Passing the contentWidth to the update() fixes the problem as well, and
if the excessive memory is not needed (as in my proposal), this is of
course much better.
Can I create an issue and suggest the following patch (this is for 8,
but I can do a similar for 9)?

- Johan

---
a/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaThu
Mar 03 03:22:09 2016 +0100

+++
b/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaWed
Mar 09 10:59:35 2016 +0100

@@ -104,7 +104,7 @@

  // since it was bound and unflushed...

  maskTex.update(maskBuffer, maskTex.getPixelFormat(),

 0, 0, 0, 0, highMaskCol, nextMaskRow,

-   maskTex.getPhysicalWidth(), true);

+   maskTex.getContentWidth(), true);

  maskTex.unlock();

  curMaskRow = curMaskCol = nextMaskRow = highMaskCol = 0;

  }


On Tue, Mar 8, 2016 at 10:43 PM, Jim Graham mailto:james.gra...@oracle.com>> wrote:

I think I see the issue.

In the code that calls maskTex.update(...) it passes
maskTex.physicalWidth() as the scan stride of the buffer, but the
scan stride of the buffer is based on the content size, not the
physical size.  Thus, the checkUpdateParams() method overestimates
how many bytes are consumed by the operation.

Changing that to maskTex.getContentWidth() should be fine...

 ...jim

On 3/8/16 6:14 AM, Johan Vos wrote:

We got a number of bug reports (on Android and iOS) reported by
developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266
elements, but
only 1549938 elements remain in the buffer

at
com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at
com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at
com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps

.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)


I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes
bigger than 1024
(w or h), a new buffer is allocated with capacity [newTexW *
newTexH] with
newTexW and newTexH the new width/height that are passed when
creating a
new Texture. However, the physical size of the texture can be
different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int,
bool) will
in some cases set the real texWidth/height to the next power of 2.

Subsequently, in next rendering loops when the Texture needs to
be updated,
it is checked whether the capacity of the buffer is large enough
to hold
the texture. In this case, the physical width is passed and the
buffer is
not large enough.

Adding the following two lines in
BaseContext.validateMaskTexture() (line
220) fixes the problem:

  newTexW = Math.max(newTexW,
maskTex.getPhysicalWidth());

  newTexH = Math.max(newTexH,
maskTex.getPhysicalHeight());

Using this patch, the size of the buffer will take the physical
size of the
texture into account. I'm not sure this is the best approach though.

- Johan

review for paper test

[David Hill]

Phil,
   could you review:

JobSettingsTest.testPaper fails with small paper sizes
Simple fix, make the margins that don't fit 10% of the page dimension

https://bugs.openjdk.java.net/browse/JDK-8149756
http://cr.openjdk.java.net/~ddhill/8149756/

--
David Hill
Java Embedded Development

"A man's feet should be planted in his country, but his eyes should survey the 
world."
-- George Santayana (1863 - 1952)

Re: buffer too small

[Kevin Rushforth]

Yes, please. Just get Jim to code review it and you can push it to 9-dev 
yourself, since you are a committer.


For JDK 8, you need to request approval to backport. If approved by me, 
then you can push it to 8u-dev.


-- Kevin


Johan Vos wrote:

Hi Jim,

Passing the contentWidth to the update() fixes the problem as well, and if
the excessive memory is not needed (as in my proposal), this is of course
much better.
Can I create an issue and suggest the following patch (this is for 8, but I
can do a similar for 9)?

- Johan

--- a/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.java Thu
Mar 03 03:22:09 2016 +0100

+++ b/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.java Wed
Mar 09 10:59:35 2016 +0100

@@ -104,7 +104,7 @@

 // since it was bound and unflushed...

 maskTex.update(maskBuffer, maskTex.getPixelFormat(),

0, 0, 0, 0, highMaskCol, nextMaskRow,

-   maskTex.getPhysicalWidth(), true);

+   maskTex.getContentWidth(), true);

 maskTex.unlock();

 curMaskRow = curMaskCol = nextMaskRow = highMaskCol = 0;

 }

On Tue, Mar 8, 2016 at 10:43 PM, Jim Graham  wrote:

  

I think I see the issue.

In the code that calls maskTex.update(...) it passes
maskTex.physicalWidth() as the scan stride of the buffer, but the scan
stride of the buffer is based on the content size, not the physical size.
Thus, the checkUpdateParams() method overestimates how many bytes are
consumed by the operation.

Changing that to maskTex.getContentWidth() should be fine...

...jim

On 3/8/16 6:14 AM, Johan Vos wrote:



We got a number of bug reports (on Android and iOS) reported by developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266 elements, but
only 1549938 elements remain in the buffer

at com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps
.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)


I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes bigger than
1024
(w or h), a new buffer is allocated with capacity [newTexW * newTexH] with
newTexW and newTexH the new width/height that are passed when creating a
new Texture. However, the physical size of the texture can be different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int, bool) will
in some cases set the real texWidth/height to the next power of 2.

Subsequently, in next rendering loops when the Texture needs to be
updated,
it is checked whether the capacity of the buffer is large enough to hold
the texture. In this case, the physical width is passed and the buffer is
not large enough.

Adding the following two lines in BaseContext.validateMaskTexture() (line
220) fixes the problem:

 newTexW = Math.max(newTexW, maskTex.getPhysicalWidth());

 newTexH = Math.max(newTexH, maskTex.getPhysicalHeight());

Using this patch, the size of the buffer will take the physical size of
the
texture into account. I'm not sure this is the best approach though.

- Johan


  

com.sun.media.jfxmedia.locator.Locator: Unsupported protocol "bundle"

[Maurice]

When I tried to load a wav file from my OSGi bundle's resources JavaFX 
reported that it could not load the file because the protocol "bundle" 
is unsupported. I checked the code and indeed, it is not supported.


However I wondered why the Locator doesn't use the same mechanism as the 
font loader. The font loader opens the input stream, downloads the file 
to a temporary folder and then opens the file, whereas the sound locator 
just balks at the protocol.


Maurice.

Re: buffer too small

[Johan Vos]

Hi Jim,

Passing the contentWidth to the update() fixes the problem as well, and if
the excessive memory is not needed (as in my proposal), this is of course
much better.
Can I create an issue and suggest the following patch (this is for 8, but I
can do a similar for 9)?

- Johan

--- a/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.java Thu
Mar 03 03:22:09 2016 +0100

+++ b/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.java Wed
Mar 09 10:59:35 2016 +0100

@@ -104,7 +104,7 @@

 // since it was bound and unflushed...

 maskTex.update(maskBuffer, maskTex.getPixelFormat(),

0, 0, 0, 0, highMaskCol, nextMaskRow,

-   maskTex.getPhysicalWidth(), true);

+   maskTex.getContentWidth(), true);

 maskTex.unlock();

 curMaskRow = curMaskCol = nextMaskRow = highMaskCol = 0;

 }

On Tue, Mar 8, 2016 at 10:43 PM, Jim Graham  wrote:

> I think I see the issue.
>
> In the code that calls maskTex.update(...) it passes
> maskTex.physicalWidth() as the scan stride of the buffer, but the scan
> stride of the buffer is based on the content size, not the physical size.
> Thus, the checkUpdateParams() method overestimates how many bytes are
> consumed by the operation.
>
> Changing that to maskTex.getContentWidth() should be fine...
>
> ...jim
>
> On 3/8/16 6:14 AM, Johan Vos wrote:
>
>> We got a number of bug reports (on Android and iOS) reported by developers
>> using large images:
>>
>> java.lang.IllegalArgumentException: Upload requires 2475266 elements, but
>> only 1549938 elements remain in the buffer
>>
>> at com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)
>>
>> at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)
>>
>> at com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)
>>
>> at com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)
>>
>> at
>> com.sun.prism.impl.ps
>> .BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)
>>
>>
>> I traced this down to the following:
>>
>> initially, a buffer of [1024 * 1024] is allocated by
>> BaseContext.validateMaskTexture. When the MaskData becomes bigger than
>> 1024
>> (w or h), a new buffer is allocated with capacity [newTexW * newTexH] with
>> newTexW and newTexH the new width/height that are passed when creating a
>> new Texture. However, the physical size of the texture can be different --
>> e.g.
>>
>> ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int, bool) will
>> in some cases set the real texWidth/height to the next power of 2.
>>
>> Subsequently, in next rendering loops when the Texture needs to be
>> updated,
>> it is checked whether the capacity of the buffer is large enough to hold
>> the texture. In this case, the physical width is passed and the buffer is
>> not large enough.
>>
>> Adding the following two lines in BaseContext.validateMaskTexture() (line
>> 220) fixes the problem:
>>
>>  newTexW = Math.max(newTexW, maskTex.getPhysicalWidth());
>>
>>  newTexH = Math.max(newTexH, maskTex.getPhysicalHeight());
>>
>> Using this patch, the size of the buffer will take the physical size of
>> the
>> texture into account. I'm not sure this is the best approach though.
>>
>> - Johan
>>
>>