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

[Sergey Bylokhov]

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.

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

> 
> 		...jim
> 
> On 11/13/14 8:35 AM, Sergey Bylokhov wrote:
> > Hi, Jim.
> > Please review an updated version of the fix:
> > http://cr.openjdk.java.net/~serb/8059942/webrev.16
> > The new method in Region class was added.
> >
> >
> > On 11.11.2014 5:43, Jim Graham wrote:
> >>
> >>
> >> On 11/10/14 2:52 PM, Sergey Bylokhov wrote:
> >>> Hi, Jim.
> >>> To simply fill a span in the Region, I'll need to open
> >>> Region.appendSpan() method,
> >>>  but I do not want to do it.
> >>
> >> No, that method should remain private.  All region objects should
> be
> >> fully constructed at all times when in the hands of calling code.
> >>
> >>>  So I need to create my own EdgesSpanIterator, or totally
> eliminate
> >> this loop and
> >>>  return Region object from the TransformHelper directly.
> >>
> >> No, all you need is a method:
> >>
> >> /**
> >>  * (describe edges array - 0,1 are y range, 2N,2N+1 are x ranges,
> 1
> >> per y range)
> >>  * (dxoff, dyoff are offsets that the edges array values are
> relative to)
> >>  */
> >> static Region getInstance(int dxoff, int dyoff, int edges[]) {
> >>     // copy loop from DrawImage.java here, but use direct
> manipulation
> >>     // of the spans array instead of union operations.
> >>     // You should be able to pre-allocate the spans array as well
> to
> >>     // save on growth using appendSpan() et al.
> >> }
> >>
> >> That should take only a few minutes to write, then DrawImage simply
> does:
> >>
> >> Region blitclip = Region.getInstance(dx1, dy1, edges);
> >>
> >>> Probably some logic can be moved from TransformHelper to the java
> level
> >>> (at least generation of edges array). Additionally it will be
> simpler to
> >>>  check a performance improvement, because now it is not so
> obvious
> >> comparing
> >>>  to the current percents improvement.
> >>
> >> Unfortunately the exact calculations to compute the edges array
> depend
> >> on a lot of calculations done at the native level.  This would be
> a
> >> major repartitioning of the code...
> >>
> >>         ...jim
> >
> >