[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] [8] Review request for 7181438: [OGL] Incorrect alpha used, during blit from SW to
From:       campbell () plausible ! coop (Chris Campbell)
Date:       2012-07-10 16:41:21
Message-ID: 104556D2-1096-4747-A1A6-1AC181390582 () plausible ! coop
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]