[OpenJDK 2D-Dev] [9] Review Request: 8059942 Default implementation of DrawImage.renderImageXform() should be improved for d3d/ogl

[Sergey Bylokhov]

Hi, Jim.
Please review an updated version of the fix:
http://cr.openjdk.java.net/~serb/8059942/webrev.17
Two notes:
 - one additional byte still is added to the bands, since I am still checking the code where we use endIndex-1/endIndex+1/endIndex++ or swap it with curYrow, etc. This isn't urgent.
 - "end < bands.length" was added to the loop for the reason of overall loop performance(+15%).

----- james.graham at oracle.com wrote:

> On 11/13/14 2:56 PM, Sergey Bylokhov wrote:
> > Hi, Jim
> >
> >> Please revert all "iff => if" changes in Region - those are not
> typos.
> >
> > ok
> >
> >> Hand-coding would require adding a bunch of code for clipping,
> though
> >> it might be faster still by avoiding all of the "are we on a new
> row?"
> >> testing in appendSpan/endRow().
> >
> > Looks like we get a big drawimage performance improvement from the
> previous fix - 30%(which is 10000% for the current jdk code) So I will
> try to go deeper and eliminate the call to appendSpan()/endRow.
> >
> >>
> >> Why does line 272 add "+1" for ?endIndex?  Other places simply use
> >> "#rows * 5".
> >
> > This is because of needSpace() implementation:
> > private void needSpace(int num) {
> >      if (endIndex + num >= bands.length) { }
> > If we have only one row then we need 5 bytes and num will be 5 =>
> equal to the bands.length => unnecessary copyArray will occures.
> > I suppose this is because endIndex should be a valid index in the
> bands array.
> 
> That is probably a bug.  That should probably be just ">" since I
> don't 
> see anywhere where we ever place anything in that last entry.
> 
> >> Other places in the code use the "clipAdd" and "dimAdd" methods to
> >> avoid
> >> overflow.  In this particular case we might have overflow for
> >> box[0-3].
> >>    On the other hand, since appendSpan() clips against the bounds
> we'd
> >
> >> resulting spans to be missing or incorrect (though it will never
> >> exceed
> >> the indicated bounding box even with overflow occuring)".
> >
> > I do not clip edges intentionally, because I trust the output of
> TransformHelper, which I assume should validate possible overflow.
> I'll update a javadoc.
> 
> Exactly, this falls more under the realm of "we're creating a new API
> 
> and even though it is only currently used by TransformHelper and we
> know 
> how that works, we should consider that this API might get re-used in
> 
> the future in other cases".  As I said, this code ensures that the 
> Region is valid, it just might produce unexpected (i.e. incorrect) 
> results if we ever overflow.  TransformHelper is unlikely to ever 
> produce values that are much outside of the 0-<couple thousand> range
> 
> since it only generates drawable-bounded coordinates so we should be 
> very far from overflow, though what if someone creates a BufferedImage
> 
> that is 1<<30 in dimensions (with the 64-bit VM and a lot of RAM)?
> 
> 			...jim