[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [9]: RFR JDK-8023213 [macosx] closed/java/awt/FontClass/NaNTransform.java fails
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2015-11-19 14:16:45
Message-ID: 564DD9CD.8050906 () oracle ! com
[Download RAW message or body]

I still think that possibly it will be better to block such transforms 
on the upper level, somewhere in the java probably in CStrike.java.


On 17.11.15 9:12, prasanta sadhukhan wrote:
> Hi Phil,
> 
> I have updated webrev to explicitly set glyph image width/height to be 0
> in case of NaN transform. Could you please review it?
> http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.02/
> 
> Regards
> Prasanta
> On 11/17/2015 3:23 AM, Phil Race wrote:
> > Would it not be better to be explicit in this so as not to leave it to
> > chance ?
> > 
> > -phil.
> > 
> > On 11/05/2015 11:49 PM, prasanta sadhukhan wrote:
> > > Hi Phil,
> > > 
> > > On 11/5/2015 4:54 AM, Philip Race wrote:
> > > > This does not look right to me. Who knows what data is on the canvas ?
> > > > it is not clear that it is 'blank'. It could have data from a
> > > > previous glyph .. I am not
> > > > sure how you know for sure. I can see that canvas is re-used. There
> > > > is reference
> > > > to a "global shared canvas".
> > > > And the actual function you invoke is one of two : one for grey and
> > > > one for lcd and
> > > > looking at the grey one CGGI_CopyImageFromCanvasToAlphaInfo)
> > > > it utilises info->width and info->height which can't be NaN because
> > > > they
> > > > are uint16 but I don't know if they are valid .. and is the "image"
> > > > field
> > > > allocated to be 0 length ?
> > > info->width & height is coming out to be 0 in case of NaN transform
> > > therefore "image" field will be of 0 length so "empty" glyph will be
> > > copied when we copy the "glyph image" from canvas (no matter what is
> > > there in canvas) to info->image via this code
> > > http://hg.openjdk.java.net/jdk9/client/jdk/file/298d3fe64572/src/java.desktop/macosx/native/libawt_lwawt/font/CGGlyphImages.m#l294
> > >  
> > > 
> > > 
> > > size_t destRow = y * destRowWidth;
> > > size_t srcRow = y * srcRowWidth;
> > > size_t x;
> > > for (x = 0; x < destRowWidth; x++) {
> > > UInt32 p = src[srcRow + x];
> > > dest[destRow + x] = CGGI_ConvertBWPixelToByteGray(p);
> > > 
> > > where destRowWidth = destRow = info->width will be 0 so dest =
> > > info->image will be of 0 length
> > > 
> > > Regards
> > > Prasanta
> > > > Could you step through how this is all guaranteed
> > > > to be safe/correct ?
> > > > 
> > > > -phil.
> > > > 
> > > > On 11/2/15, 12:41 AM, prasanta sadhukhan wrote:
> > > > > Hi Phil,
> > > > > 
> > > > > I have modified as per your review to populate GlyphInfo with
> > > > > "empty" glyph
> > > > > and also moved your existing test to "open"
> > > > > I also added a Infinite Transform test along with your NaN
> > > > > transform just incase (in fact Sergey informally asked me to check)
> > > > > 
> > > > > http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.01/
> > > > > 
> > > > > Regards
> > > > > Prasanta
> > > > > On 10/30/2015 1:03 AM, Phil Race wrote:
> > > > > > Should this new check go before this :
> > > > > > 
> > > > > > CGGI_ClearCanvas(canvas, info);
> > > > > > 
> > > > > > since it is using info which is where you get NaN from.
> > > > > > 
> > > > > > 
> > > > > > And should we then populate the returned canvas and info to
> > > > > > ensure that we return an "empty" glyph rather than garbage values ?
> > > > > > It is not clear to me that this is occurring.
> > > > > > 
> > > > > > Why does the bug report not contain the evaluation below ?
> > > > > > Also why is there a new test ? I would expect SQE would
> > > > > > want to run the existing test to verify this fix.
> > > > > > Should we not just open the existing test ?
> > > > > > 
> > > > > > 
> > > > > > -phil.
> > > > > > 
> > > > > > On 10/13/2015 04:49 AM, prasanta sadhukhan wrote:
> > > > > > > Gentle reminder for review
> > > > > > > 
> > > > > > > Regards
> > > > > > > Prasanta
> > > > > > > On 10/6/2015 3:25 PM, prasanta sadhukhan wrote:
> > > > > > > > Hello All,
> > > > > > > > 
> > > > > > > > Please review a fix for jdk9
> > > > > > > > 
> > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8023213
> > > > > > > > webrev: http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.00/
> > > > > > > > 
> > > > > > > > drawString takes long time in mac native call
> > > > > > > > CGContextShowGlyphsAtPoint() if NaN transform is used which
> > > > > > > > translated to x & y being NaN.
> > > > > > > > Fix is to prevent calling mac api for such invalid usage.
> > > > > > > > 
> > > > > > > > Regards
> > > > > > > > Prasanta
> > > > > > > 
> > > > > > 
> > > > > 
> > > 
> > 
> 


-- 
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]