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

[Jim Graham]

Looks good...

			...jim

On 11/19/14 11:44 AM, Sergey Bylokhov wrote:
> Hi, Jim.
> Please review an updated version of the fix:
> http://cr.openjdk.java.net/~serb/8059942/webrev.18
> I hope that all possible overflows/correctness/validity problems are
> checked now.
>
> On 15.11.2014 1:50, Jim Graham wrote:
>> Hi Sergey,
>>
>> On 11/14/14 6:40 AM, Sergey Bylokhov wrote:
>>> 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.
>>
>> Since you no longer call appendSpan(), you no longer need the
>> additional byte in this new method.
>>
>>>   - "end < bands.length" was added to the loop for the reason of
>>> overall loop performance(+15%).
>>
>> That's odd that it helps since you then consume 5 entries after that
>> check, but I guess Hotspot works in mysterious ways.  I wonder if
>> "end+4" works as well or better since that would be a more accurate
>> test of what is happening.  Also, even for 15% performance difference
>> of this one method, it might be better to take the AAIOBE as a double
>> check against the caller feeding us bad data, such as edges[0,1] that
>> end up overflowing?  In either case, we generate valid data even in
>> the face of such anomalies.
>>
>> I'd still prefer to have the code confirm the ranges.  It would still
>> be much simpler than the old code that called appendSpan/endRow a lot
>> with just a few checks, like this:
>>
>>     static Region getInstance(final int lox, final int loy, final int
>> hix,
>>                               final int hiy, final int edges[]) {
>> // maybe?:        if (hiy <= loy || hix <= lox) return EMPTY_REGION;
>>         final int y1 = loy + edges[0];
>>         final int y2 = loy + edges[1];
>>         // rowsNum * (3 + 1 * 2)
>>         final int[] bands = new int[(y2 - y1) * 5];
>>         int end = 0;
>>         int index = 2;
>> // Note to Sergey: Does end+4 also generate faster code?
>>         for (int y = y1; end+4 < bands.length && y < y2; ++y) {
>>             final int x1 = lox + edges[index++];
>>             final int x2 = lox + edges[index++];
>> // Is the following comment too obvious?
>>             // Note: always consume both indices before rejecting span
>>             if (y < loy || y >= hiy) continue;
>>             if (x1 < lox) x1 = lox;
>>             if (x2 > hix) x2 = hix;
>>             if (x1 < x2) {
>>                 bands[end++] = y;     // spanloy
>>                 bands[end++] = y + 1; // spanhiy
>>                 bands[end++] = 1;     // 1 span per row
>>                 bands[end++] = x1;    // spanlox
>>                 bands[end++] = x2;    // spanhix
>>             }
>>         }
>>         return end != 0 ? new Region(lox, loy, hix, hiy, bands, end)
>>                         : EMPTY_REGION;
>>     }
>>
>> This code should ensure correctness/validity of the Region data
>> structure without much performance impact, hopefully?
>>
>>             ...jim
>
>