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