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

[Sergey Bylokhov]

Thanks, Jim!
Can anybody make the second review of this fix?

Also I need a review of this change as well:
http://mail.openjdk.java.net/pipermail/2d-dev/2014-November/004937.html

On 20.11.2014 7:01, Jim Graham wrote:
> 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
>>
>>


-- 
Best regards, Sergey.