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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8140530, , Creating a VolatileImage with size 0,
From:       Jim Graham <james.graham () oracle ! com>
Date:       2015-12-04 22:52:08
Message-ID: 56621918.5060308 () oracle ! com
[Download RAW message or body]

On 12/4/15 1:00 AM, Sergey Bylokhov wrote:
> Looks fine to me to.
> I guess that the native check still necessary in case we create a native
> surface as a cache for a BufferedImage.

Or in case we want to relax the general API restriction later, it keeps 
us from ending up with a hard-to-diagnose bug on the X11 pipeline. We'll 
find out right away that we need an alternate solution for X11...

			...jim

> On 04.12.15 12:39, Jim Graham wrote:
>> +1
>>
>>          ...jim
>>
>> On 12/3/15 10:17 PM, prasanta sadhukhan wrote:
>>> ok. Thanks Jim .
>>> Please review the modified webrev
>>> http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.02/
>>>
>>> Regards
>>> Prasanta
>>> On 12/4/2015 6:45 AM, Jim Graham wrote:
>>>> I think it makes sense to catch it at a higher level, but also to
>>>> throw some type of exception from the X11 code as you do now because
>>>> regardless of our higher level policy, the X11 implementation function
>>>> can never succeed there...
>>>>
>>>> So, my preference would be to keep the existing pieces of this fix
>>>> that you already have and to add an explicit check and throw IAE to
>>>> SunVolatileImage for completeness and guaranteed consistency...
>>>>
>>>>             ...jim
>>>>
>>>> On 12/2/15 11:18 PM, prasanta sadhukhan wrote:
>>>>> Hi Jim,
>>>>>
>>>>> On 12/3/2015 12:38 AM, Jim Graham wrote:
>>>>>> Hi Prasanta,
>>>>>>
>>>>>> (On a practical note, in the HTML version of your message, the text
>>>>>> said "webrev.01", but the link in the href pointed to "webrev.00"
>>>>>> so I
>>>>>> sat there wondering why the changes you noted weren't there until I
>>>>>> realized that I was still looking at webrev.00 and had to manually
>>>>>> enter webrev.01 in the browser to see the new code...)
>>>>>>
>>>>>> Have you run your new test on all platforms to make sure that it
>>>>>> succeeds (by throwing IAE) on all currently supported/tested
>>>>>> platorms?
>>>>>>
>>>>> IAE was already been thrown for windows and mac. It was not working
>>>>> for
>>>>> linux only.
>>>>>> It seems, from the comment, that one issue is that X11 has a special
>>>>>> need in that if we make it through to the native code with 0,0
>>>>>> arguments and attempt to create a pixmap of 0,0 then we get an X11
>>>>>> error so I'm OK with the native code having its own check for
>>>>>> protection against the X11 error. But, for consistency, shouldn't the
>>>>>> 0,0 be detected and and IAE thrown at a much higher level shared by
>>>>>> all platforms so that no platform can accidentally allow 0,0?
>>>>>> Otherwise we have to make sure that each and every current platform
>>>>>> and each and every future platform port contains these checks to
>>>>>> satisfy the new behavior expectation.
>>>>>>
>>>>>> Apparently, somewhere above the native method there is a check that
>>>>>> converts OOME to IAE - is that in shared code or in the X11-specific
>>>>>> code?
>>>>> It is not a direct conversion from OOME to IAE. Basically, the Java
>>>>> will
>>>>> try to create a accelerated surface and if it cannot, it creates a
>>>>> backup BufferedImage via createCompatibleImage() which calls
>>>>> createCompatibleWritableRaster() and that function has a check for 0
>>>>> width,height which throws IAE.
>>>>>
>>>>> Code wise flow:
>>>>> VolatileSurfaceManager.initialize() will check for accelerated surface
>>>>> via initAcceleratedSurface() which goes to different pipeline for
>>>>> different platforms.
>>>>>
>>>>> In windows, D3DVolatileSurfaceManager.java#initAcceleratedSurface()
>>>>> will
>>>>> call D3DSurfaceData.createData() which calls initSurface() which
>>>>> initializes surface by calling "native" initSurfaceNow() which returns
>>>>> false for 0 width.height. D3DSurfaceData throws
>>>>> InvalidPipeException to
>>>>> D3DVolatileSurfaceManager#initAcceleratedSurface()  which marks
>>>>> accelerated surface as null in that case so Java will fall back to
>>>>> BufferedImage as explained above.
>>>>>
>>>>> In mac, CGLVolatileSurfaceManager.jaav#initAcceleratedSurface() will
>>>>> get
>>>>> OOM from CGlSurfaceData.createData() which calls native initFBObject()
>>>>> which returns false
>>>>>
>>>>> In linux, native was not throwing any issue even though it does not
>>>>> utilise 0 width,height so Java still assumes it is working with
>>>>> accelerated surface so will not try to create backup BufferedImage
>>>>> surface so cannot throw IAE.
>>>>> My fix will let native throw OOM to
>>>>> XRVolatileSurfaceManager.java#initAcceleratedSurface() so that it
>>>>> knows
>>>>> it fails to create accelerated surface and has to fall back to
>>>>> BufferedImage surface and then the IAE will be thrown from
>>>>> createCompatibleWritableRaster()
>>>>>
>>>>>
>>>>> If you think it's ok, we can catch 0 width,height in SunVolatileImage
>>>>> constructor before it calls VolatileSurfacemaanger. initialize() so
>>>>> that
>>>>> it gets trapped  at higher level? Please advise.
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>>
>>>>>>
>>>>>>             ...jim
>>>>>>
>>>>>> On 11/30/15 9:58 PM, prasanta sadhukhan wrote:
>>>>>>> Modified the testcase to "fail" if IAE is not thrown
>>>>>>> http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.01
>>>>>>>
>>>>>>> Regards
>>>>>>> Prasanta
>>>>>>> On 11/30/2015 2:13 PM, prasanta sadhukhan wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Please review a fix for jdk9
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8140530
>>>>>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.00/
>>>>>>>>
>>>>>>>> The issue was creating a volatileImage with 0 width, height does
>>>>>>>> not
>>>>>>>> result in IllegalArgumentException.
>>>>>>>> But, when we try to create a non-volatile Image via
>>>>>>>> GraphicsConfiguration.createCompatibleImage(0,0) or a
>>>>>>>> BufferedImage(0,0,imagetype) it results in IAE.
>>>>>>>> So, to maintain consistency across all image w.r.t 0 width,height,
>>>>>>>> createVolatileImage() should also throw IAE.
>>>>>>>>
>>>>>>>> In windows, creating a volatileImage with 0 width,height
>>>>>>>> resulted in
>>>>>>>> IAE but in linux it does not.
>>>>>>>>
>>>>>>>> This is because XCreatePixmap() generate BadValue unless
>>>>>>>> width,height
>>>>>>>> is nonzero but the error handler does not catch it.
>>>>>>>> https://tronche.com/gui/x/xlib/pixmap-and-cursor/XCreatePixmap.html
>>>>>>>> [The width and height arguments must be nonzero, or a *BadValue*
>>>>>>>> error
>>>>>>>> results.]
>>>>>>>>
>>>>>>>> I have added a check to prevent zero width,height to be used for
>>>>>>>> XCreatePixmap() and also throw OOME so to ask Java to throw IAE.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Prasanta
>>>>>>>
>>>>>
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]