[OpenJDK 2D-Dev] [8] request for review: 7124347: [macosx] "java.lang.InternalError: not implemented yet" on call Graphics2D.drawRend

Wed Oct 1 18:29:37 UTC 2014

Hi, Jim, Phil, Andrew.
I have a question about this fix:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/3c5bf5ed45a9

This fix was implemented on top of the Blit loop(new OGLAnyCompositeBlit 
class extends Blit). This approach has one drawback - it does not cover 
ScaledBlit/TransformBlit loops(same issue exist in OGLGeneralBlit as well)
So if we try to blit something in DrawImage using 
ScaledBlit/TransformBlit we fail to find a necessary loop and we moves 
to the TransformHelper, which is extremely slow(it tries to draw 
scanline by scanline to opengl)
My first solution was to create the new classes like 
OGLAnyCompositeScaledBlit/OGLAnyCompositeTransformBlit/OGLGeneralScaledBlit/OGLGeneralTransformBli 
, extend them from ScaledBlit/TransformBlit, and implement necessary 
functionality, but code became noisy with a lot of code duplication.

Probably it will be better to rework initial fix and move the code from 
OGLAnyCompositeBlit to OGLDrawImage.renderImageXform()? It will be last 
chance to blit something to oglSurface instead of using TransformHelper. 
Because if we came to renderImageXform means that all possible loops 
where dest is oglsurface were checked. So we can try to blit oglsurface 
to argbPreTempBuffer, then blit current surface to this buffer, and then 
blit buffer back to oglsurface.
What do you think?

Also I think that this loop 
OGLAnyCompositeBlit(OGLSurfaceData.OpenGLTexture), is not necessary, 
because we have no OGLSurfaceData.OpenGLTexture->SurfaceType.IntArgbPre 
blit anyway.

Related bugs:
https://javafx-jira.kenai.com/browse/RT-37740
https://bugs.openjdk.java.net/browse/JDK-8029253

On 01.12.2012 4:40, Jim Graham wrote:
> I'm OK with the removal of the test, but I didn't look in more depth. 
> Phil already approved from his end, though, so I think you can take 
> this as a concurrence...
>
>             ...jim
>
> On 11/30/12 1:37 AM, Andrew Brygin wrote:
>> Hi Jim,
>>
>>   I have verified that we use either the imageComp field from 
>> SunGraphics2D
>>   (which can not be null), or some explicit composite type in this 
>> method.
>>   So, according to  your suggestion, I have removed  the check for null.
>>   Please take a look to updated webrev:
>>
>>   http://cr.openjdk.java.net/~bae/7124347/8/webrev.01/
>>
>>   Phil, could you please take a look the the updated fix also?
>>
>> Thanks,
>> Andrew
>>
>>
>> On 30.11.2012 4:48, Jim Graham wrote:
>>> Hi Andrew,
>>>
>>> My belief is that comp should never be null.  A simple grep should
>>> confirm that belief.  While I do agree that it can be safer to check
>>> for null if you aren't sure about the code base, once one piece of
>>> code does it then it spreads and weakens the API contract so it would
>>> be better to check and keep the API contract stronger if we can...
>>>
>>>             ...jim
>>>
>>> On 11/29/12 12:18 AM, Andrew Brygin wrote:
>>>> Hello Jim,
>>>>
>>>>   I do not think that suggested change allows a null composite per se:
>>>>   the check for null was introduced just to make sure that we can 
>>>> safely
>>>>   call isDerivedFrom method. However, it does not change the method
>>>>   behavior for the case of null composite.
>>>>
>>>>   If it is impossible to get null composite here for some reason
>>>> (unfortunately
>>>>   I can not say this for sure), then this check is not required.
>>>> However, I do not
>>>>   think that the extra check for null may cause any harm.
>>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>>
>>>> On 29.11.2012 4:07, Jim Graham wrote:
>>>>> Is there a reason for allowing a null comp in the isSupported method?
>>>>>
>>>>>             ...jim
>>>>>
>>>>> On 11/26/2012 4:38 AM, Andrew Brygin wrote:
>>>>>> Hello,
>>>>>>
>>>>>>   could you please review a fix for 7124347?
>>>>>>
>>>>>>   This fix does not implement getRater() in ogl surfaces.
>>>>>>   Instead, it provides a blit for custom composite, which
>>>>>>   prepares a snapshot of the destination surface (which
>>>>>>   is one of ogl surfaces) and delegates the work to the
>>>>>>   general blit, which now extracts raster from the snapshot.
>>>>>>   A result of general blit's work is transferred to the
>>>>>>   original destination.
>>>>>>
>>>>>>   Changes in OGLSurfaceDataProcy are required in order to prevent
>>>>>>   getting an accelerated copy of original source image as an
>>>>>>   operand of the blit. Now we first check for composite type,
>>>>>>   and only then (if composite is not specified, or is a kind
>>>>>>   of alpha composite) we take into account other conditions.
>>>>>>
>>>>>>   So this fix does not change anything for the case of alpha
>>>>>> composites
>>>>>>   but affects only XOR and custom composites case.
>>>>>>
>>>>>>   Note that the fix solves the problem only for the case of blits
>>>>>>   (i.e. drawImage()). Other operations may result into the same
>>>>>>   problem if custom composite is used. However, in case of XOR,
>>>>>>   this change solves the problem completely, because other rendering
>>>>>>   operations in the ogl pipeline are ready for XOR composite.
>>>>>>
>>>>>>   Supplied regression test verifies that the fix makes the problem
>>>>>> gone.
>>>>>>   I have verified both windows and macosx platforms.
>>>>>>
>>>>>>   Please take a look and share your comments.
>>>>>>
>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7124347
>>>>>> Webrev: http://cr.openjdk.java.net/~bae/7124347/8/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Andrew
>>>>
>>

-- 
Best regards, Sergey.