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