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