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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Review request for JDK-8147413 : api/java_awt/Image/MultiResolutionImage/index.
From:       Alexander Scherbatiy <alexandr.scherbatiy () oracle ! com>
Date:       2016-01-22 11:35:03
Message-ID: 56A213E7.8090903 () oracle ! com
[Download RAW message or body]


I have created two separated issues on it:
   8148045 Handle null resolution variant in SunGraphics2D
     https://bugs.openjdk.java.net/browse/JDK-8148045

   8148046 Investigate the case where MRI.getResolutionVariant() returns 
MultiResolutionImage
     https://bugs.openjdk.java.net/browse/JDK-8148046

Thanks,
Alexandr.

On 22/01/16 00:34, Jim Graham wrote:
> BASE essentially just means to draw the 1:1 image.  The problem is 
> that returning null is not the way to achieve that.  Returning null 
> means "MRI doesn't apply here, just use the original image", but in 
> the case of BASE we have to make sure it isn't an MRI first otherwise 
> the caller will get an exception.
> 
> The change does achieve that since BASE handling is built in to the 
> helper getResolutionVariant() method in SG2D and so it should ask for 
> the 1:1 image, but if the getRV() helper method returns null then we 
> still return null from the drawHiDPIImage() method and that causes the 
> caller to use the original MRI in a normal-image pipeline.  There 
> should perhaps be an else clause for the "if (rV != img && rV != null) 
> { } else { ???; }". What that "???" is depends on why the helper 
> method returned the original image and/or a null, but it should not be 
> "return null;". (Also, there is no check if it returned another 
> different MRI. That should probably not be allowed, but we never state 
> that in the interface description...)
> 
> The remaining question is why the helper method would return either 
> the original image or null.
> 
> I don't think the helper method ever returns the original MRI 
> knowingly, but the MRI itself might do that.  That would then fall 
> under the "what to do if the MRI returns any sort of non-standard 
> image" case, and perhaps the exception is appropriate there?  We 
> should probably detect that, though, and throw a more obvious 
> exception (or recurse?). Recursion can get ugly, so perhaps a very 
> explicit error with a reasonable description would be better than 
> letting the DrawImage pipeline fail in various ways.
> 
> The helper function can return null if the operation is a NOP (sxy1 == 
> sxy2 - i.e. an empty subimage rectangle), but that case should be 
> handled here and end up with returning ?true? which matches what many 
> of the drawImage() variants return when they detect a trivial NOP case.
> 
> The helper function can also return null if the srcWidth/Height are 
> negative which would imply an uninitialized image.  I'm not sure what 
> to do there because normally an uninitialized image is tracked by the 
> DrawImage pipe and the observer is notified as the image data is 
> loaded, but we have no mechanism to do that with an MRI.  This case 
> may require more work, or something explicit in the MRI spec (what 
> happens when someone lazily loads an image with an @x2 variant now?).
> 
> It also returns null if the return value from the MRI is a 
> ToolkitImage with an error - but I think that is wrong, it should just 
> return the ToolkitImage and let the caller send it to the DrawImage 
> pipeline to detect the error and respond in the appropriate manner.  
> Or perhaps it should cause the drawHiDPIImage() method to return 
> whatever boolean is appropriate for an error to save time, but there 
> is no way to orchestrate that from the helper method since it can only 
> return a (non-MRI) image.  It's probably best to just return the 
> errored TKImage in that case...?
> 
> The last way it can return null is if the MRI returned null from its 
> getRV() method, but the interface doesn't discuss what that means.  I 
> suppose it might mean that all of the resolution variants are being 
> loaded asynchronously and none of them are available yet, but we are 
> then missing a mechanism to inform the observer of when the variants 
> are loaded.  (Is there a bugid for that?  I seem to recall having a 
> discussion about that missing mechanism in the past...?)
> 
> So, I think the change made here is along the right track, but we have 
> a couple more holes to plug before we can consider this fixed...
> 
> ...jim
> 
> On 1/20/2016 12:57 PM, Phil Race wrote:
> > It seems like the expectation was that BASE could be drawn by the old
> > imaging path.
> > which would be lower overhead.
> > I think we should ask Alexandr what the intention was here and 
> > whether the
> > code that handles the base image needs to be taught how to extract 
> > data from
> > a MultiResolutionImage.
> > 
> > -phil.
> > 
> > On 01/20/2016 08:20 AM, Jayathirth D V wrote:
> > > 
> > > Hi,
> > > 
> > > __
> > > 
> > > _Please review the following fix in JDK9:_
> > > 
> > > Bug : https://bugs.openjdk.java.net/browse/JDK-8147413
> > > 
> > > Webrev : http://cr.openjdk.java.net/~jdv/8147413/webrev.00/
> > > <http://cr.openjdk.java.net/%7Ejdv/8147413/webrev.00/>
> > > 
> > > Issue : JCK testcase
> > > api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE] \
> > >  
> > > is failing from b96 JDK9 build.
> > > 
> > > Root cause : In getManager API of SurfaceManager.java we are trying to
> > > typecast BaseMultiResolutionImage to BufferedImage and it is causing
> > > ClassCastException and in turn IllegalArgumentException seen in result
> > > of test case. It is happening because of change made in JDK-8073320
> > > <https://bugs.openjdk.java.net/browse/JDK-8073320> in
> > > SunGraphics2D.java. In case of VALUE_RESOLUTION_VARIANT_BASE type we
> > > are not trying to convert MultiResolutionImage to BufferedImage.
> > > 
> > > Solution : Modify the condition present in drawHiDPIImage API to
> > > convert all MultiResolutionImage to BufferedImage irrespective of
> > > KEY_RESOLUTION_VARIANT type.
> > > 
> > > Thanks,
> > > 
> > > Jay
> > > 
> > 


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