[OpenJDK 2D-Dev] More incompatibilities

[Jim Graham]

On a footnote, we should consider coming up with naming conventions for 
when ranges refer to "first and last pixels touched" vs. when they refer 
to "start and end coordinates between pixels (i.e. when the end 
coordinate is the number of the first pixel outside the region)".

Personally, in code that I write I tend to always use the latter 
convention so that you can figure out how many pixels are being modified 
using "x1-x0".  The math just seems easier in that convention, but I 
think it's a matter of taste.  The engineer who wrote Pisces seems to 
have done a lot of it using the other convention.

Mixing and matching tends to lead to bugs like this - it would be better 
to convert to one convention or the other, but if we don't have a single 
convention then naming becomes more important...

			...jim

Jim Graham wrote:
> This just came through internally for a code review and I looked into it 
> in more depth.  Sorry to take so long to get back to everyone, but I 
> don't think this is the answer.
> 
> I think the problem is in the startRow function.  In the "alphaRows == 
> 0" case it sets the bbox to x1+1, but in the else part it only expands 
> the bbox to x1.  So, startRow is inconsistent with respect to whether it 
> expects x1 to be the last pixel in the box, or the first one to the 
> right of the box.  It needs to be made consistent and then this code, 
> and any other code that calls that function, need to then adhere to that 
> convention.  A quick search looks like this is the only place that calls 
> it so it shouldn't be too hard to fix by just modifying PiscesCache.
> 
> Until startRow is consistent internally, we can't fix this bug by 
> changing our convention of how it is called from Renderer...
> 
>             ...jim
> 
> Hiroshi Yamauchi wrote:
>> Hi Roman,
>>
>> First off, thanks for taking a look at the issues. I confirmed that
>> all the tests that I have pass with your patches applied.
>>
>> On Tue, Mar 3, 2009 at 2:09 AM, Roman Kennke <roman.kennke at aicas.com> 
>> wrote:
>>> Hi there,
>>>
>>>> I believe that the minX and maxX parameters to that function are
>>>> relative to the origin of the raster coordinates which means you would
>>>> add each of them to the raster's origin (rasterMinX) to get an absolute
>>>> min/maxX.  So they look correct as written.
>>>>
>>>> If this change fixes the problem, then it is a side effect of moving 
>>>> the
>>>> right edge of the "startRow" call out to the right some more.  That
>>>> could indicate that perhaps maxX was not calculated correctly in the
>>>> first place and so expanding it out "covers up" the problem.
>>> I had a look at that code. It seems like minX and maxX are calculated
>>> correctly. However, in emitRow, we calculate x0 and x1 from that, and
>>> then call the cache.startRow() with that to start a new row in the RLE
>>> cache. It seems like the cache (or the rendering code that reads the
>>> cache) interprets x1 to be the first pixel right of the bounding box,
>>> while the emitRow() function needs that pixel _inside_ the bounding box
>>> (it does while (srcIdx <= maxX) { .. } ). The following patch fixes the
>>> problem for me:
>>
>> Aha.
>>
>>> diff --git a/src/share/classes/sun/java2d/pisces/Renderer.java
>>> b/src/share/classes/sun/java2d/pisces/Renderer.java
>>> --- a/src/share/classes/sun/java2d/pisces/Renderer.java
>>> +++ b/src/share/classes/sun/java2d/pisces/Renderer.java
>>> @@ -634,7 +634,7 @@
>>>                 int x0 = minX + (rasterMinX >>
>>> SUBPIXEL_LG_POSITIONS_X);
>>>                 int x1 = maxX + (rasterMinX >>
>>> SUBPIXEL_LG_POSITIONS_X);
>>>
>>> -                cache.startRow(y, x0, x1);
>>> +                cache.startRow(y, x0, x1 + 1);
>>>                 int srcIdx = minX;
>>>
>>>                 // Perform run-length encoding
>>> (END)
>>>
>>> /Roman
>>>
>>> -- 
>>> Dipl.-Inform. (FH) Roman Kennke, Software Engineer, http://kennke.org
>>> aicas Allerton Interworks Computer Automated Systems GmbH
>>> Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
>>> http://www.aicas.com   * Tel: +49-721-663 968-48
>>> USt-Id: DE216375633, Handelsregister HRB 109481, AG Karlsruhe
>>> Geschäftsführer: Dr. James J. Hunt
>>>
>>>
>>>
>>
>> Hiroshi