Hi Sergey,

That looks good to me.

Thanks,
Chris


On Jul 10, 2012, at 6:27 AM, Sergey Bylokhov wrote:
> Hi, Chris.
> Thanks for review. Can you please look into the new version:
> http://cr.openjdk.java.net/~serb/7181438/webrev.01/
> in this version OGLContext_SetExtraAlpha was removed and GL_ALPHA_BIAS 
> changed to 1.0f.
> 
> I'll create additional CR to track changes from 7125723&7181438.
> 
> 10.07.2012 03:16, Chris Campbell wrote:
>> Hi Sergey,
>> 
>>> Probably OGLBlitSwToTexture assumes that both of sw and texture should be 
>>> opaque or non opaque.
>> That is correct.  OGLBlitSwToTexture is only used to upload a system memory 
>> surface (a BufferedImage surface) into an OpenGL texture for the purposes of 
>> managed images.  So, prior to 7125723, the managed image code would check 
>> whether the source surface has an alpha channel.  If so, it would create a 
>> GL_RGBA texture and the alpha channel would be preserved.  If not, it would 
>> create a GL_RGB texture, so if you were to upload Int[X]Rgb data, the 
>> undefined alpha channel would effectively be ignored.
>> 
>> Also, since OGLBlitSwToTexture is only used for that special managed image 
>> upload-to-texture case, it should not take oglc->extraAlpha into account.  
>> (That value should only come into play when rendering to a destination 
>> surface, such as an FBO or Pbuffer.)  So, if you do end up needing to 
>> proceed with your fix, those calls to OGLContext_SetExtraAlpha() and 
>> j2d_glPixelTransferf(GL_ALPHA_BIAS) should be removed, I believe.
>> 
>> Thanks,
>> Chris
>> 
>> 
>> On Jul 9, 2012, at 3:35 PM, Sergey Bylokhov wrote:
>>> Hi, Chris.
>>> I am sure, that those changes should be reverted back or at least we should 
>>> rethink.
>>> 
>>> Note, that when I paint something from the buffered image to the surface, 
>>> I'll get correct image. Because OGLBlitSwToSurface, that is used in this 
>>> case, will block alpha for opaque surface. But when I enable managedimage 
>>> for the same code, I'll get incorrect image, because of incorrect alpha in 
>>> OGLBlitSwToTexture. Probably OGLBlitSwToTexture assumes that both of sw and 
>>> texture should be opaque or non opaque. Not sure that this is correct.
>>> 
>>> 
>>> 09.07.2012 21:40, Chris Campbell wrote:
>>>> Hi Sergey,
>>>> 
>>>> Do you happen to know the intent behind this original change from 
>>>> macosx-port?
>>>> http://hg.openjdk.java.net/macosx-port/macosx-port/jdk/rev/50b8fef5df7d
>>>> 
>>>> The commit message for that changeset didn't have much information; it 
>>>> just said:
>>>> "Fixing AWT components painted over in white"
>>>> 
>>>> Perhaps it was trying to work around a Mac-specific driver issue?
>>>> 
>>>> It seems unfortunate to create all textures (on all platforms) as RGBA 
>>>> even if the image is opaque.  At the very least, it invalidates the 
>>>> documentation for OGLSD_InitTextureObject():
>>>>   * If the isOpaque parameter is JNI_FALSE, then the texture will have a
>>>>   * full alpha channel; otherwise, the texture will be opaque (this can
>>>>   * help save VRAM when translucency is not needed).
>>>> 
>>>> Also, I remember a time when GL_ALPHA_SCALE and GL_ALPHA_BIAS weren't 
>>>> optimized by some drivers.  Maybe it is no longer an issue for modern 
>>>> drivers; not sure.  Maybe these changes (such as 7125723) should be #ifdef 
>>>> MACOSX to make it clear that it's working around Mac-specific bugs 
>>>> (assuming that was the case).
>>>> 
>>>> Just want to make sure we're not creating layers of workarounds here.  Or, 
>>>> if the workarounds are needed, it would be nice if they were explicitly 
>>>> documented and/or made conditional for specific platforms.
>>>> 
>>>> Thanks,
>>>> Chris
>>>> 
>>>> P.S. Good to see some activity on the OGL pipeline.  I don't get paid to 
>>>> worry about these things anymore though, so perhaps I should just crawl 
>>>> back in my hole now :)
>>>> 
>>>> 
>>>> On Jul 9, 2012, at 4:35 AM, Sergey Bylokhov wrote:
>>>>> Hi Everyone,
>>>>> Please review the fix for 7181438.
>>>>> This bug was introduced in 7u4b11, when macosx-port was integrated to 
>>>>> jdku7 by this changeset:
>>>>> http://hg.openjdk.java.net/jdk7u/jdk7u-dev/jdk/rev/9dfe50f456be
>>>>> 
>>>>> Note that now we always set format to GL_RGBA. So opaque surface has an 
>>>>> alpha channel.
>>>>> We handle this situation in: OGLBlitSurfaceToSurface, OGLBlitSwToSurface, 
>>>>> OGLBlitToSurfaceViaTexture,
>>>>> but there is no check in OGLBlitSwToTexture().
>>>>> 
>>>>> I assume that code for alpha verification should be the same.
>>>>> 
>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181438
>>>>> Webrev can be found at: http://cr.openjdk.java.net/~serb/7181438/webrev.00
>>>>> 
>>>>> Also note that CR 7177173 and 7124244 depends from this one.
>>>>> -- 
>>>>> Best regards, Sergey.
>>>>> 
>>> 
>>> -- 
>>> Best regards, Sergey.
>>> 
> 
> 
> -- 
> Best regards, Sergey.
> 

Reply via email to