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

[Jim Graham]

Hi Sergey,

Please revert all "iff => if" changes in Region - those are not typos. 
"iff" is mathematical shorthand for "if and only if" and those changes 
modify the correctness of those comments.

The new method looks good, I was thinking it would be better to 
hand-code the filling of the array, but it looks like appendSpan() does 
clipping so we can be sure that the data remains in range/valid. 
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().

Why does line 272 add "+1" for ?endIndex?  Other places simply use 
"#rows * 5".

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 
end up with a valid, though potentially incorrect, region as a result. 
Validity is essential to prevent disasters, and that should be covered 
here.  Correctness adds to the reliability of this code, but we seem to 
draw the line at "trusting the correctness" of the output of the 
SpanIterator for instance.  I think the current code is OK, but it might 
be worth a comment "While we guarantee that all spans in the returned 
Region will be inside the bounding box, any overflow in adding the 
values in the edges array to the lox, loy values may cause one of the 
resulting spans to be missing or incorrect (though it will never exceed 
the indicated bounding box even with overflow occuring)".

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