[OpenJDK 2D-Dev] [9] RFR: JDK-8140530, , Creating a VolatileImage with size 0, 0 results in no longer working g2d.drawStri

[Sergey Bylokhov]

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.

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
>>>>>>
>>>>
>>


-- 
Best regards, Sergey.