Re: buffer too small

[Jim Graham]

I thought this sounded familiar.  I remember having the thought before, 
but forgot that I actually did something about this already in another 
part of the code.


Both of those sets of methods already exist and they are used in the 
decora ImagePool object to avoid having to allocate a larger scratch 
texture when this over-allocation occurs.  Similar code should be used 
in the updates of the maskTex object.  It would probably be worthwhile 
to immediately adjust it to maxContent dimensions right after it is 
allocated.


That doesn't obviate the fix below, but it adds an optimization...

...jim

On 3/9/16 3:26 PM, Jim Graham wrote:

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

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

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


  

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

Re: buffer too small

[Jim Graham]

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

Re: buffer too small

[Jim Graham]

Hi Johan,

Notes in line below...

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.


This is one of the primary reasons why content dimensions and physical 
dimensions are not the same.



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.


Where is this code that you referred to above?  Where is the physical 
width passed from/to?  Lines of code?


The capacity checks at the top of updateMaskTexture() are all using 
content dimensions which are appropriate, so where are the capacity 
checks you see which are using the physical width?



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());


That may mask the problem, but it doesn't fix the underlying problem. 
newTexWH are supposed to be the new content dimensions and should not be 
based on the old phyiscal dimensions or we will grow the texture 
unnecessarily large in many cases.



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.


The buffer is used to update the content portion of the texture, it 
should only ever take the content dimensions of the texture into 
account.  The physical dimensions are not relevant to the discussion here.


There is an optimization that could avoid allocating a new texture that 
could be based on physical dimensions, but you aren't tying into it with 
the above analysis.  I want to get up to speed on what you've found 
above before I can recommend a more appropriate solution...


...jim