Thanks for looking Brecht. Some clarifications as discussed via IRC but put here for posterity and searching:
On Fri, Sep 30, 2011 at 7:37 AM, Brecht Van Lommel <[email protected]> wrote: > * The BLI_CM_ should not be in blenlib, there should be separate flags > in DNA_image_types.h and IMB_imbuf_types.h. This adds a bit more code, > but these things should be decoupled. Not entirely clear here. At some point relevant color management flags should migrate to the colormanagement.c/h file when we end up with one. I dropped the flags into math_color.h as it seemed the most appropriate location for color management related flags. I can easily move them somewhere else. > * I'd also be inclined to not add a separate cm_flags but use the > existing flag members in ImBuf and Image. At a bare minimum we will likely require two additional elements for a color management system. An int value is sub-optimal as it is currently implemented, and probably isn't terribly future friendly. I'd suggest we will probably require a color management struct or, alternatively, a 240ish char string and an int flag. This should initially cover OCIO implementation. A struct may make even more sense here in the event that the CM system widens in the future, requiring ICC strings or like details. > * Predivide should check if the alpha is zero and avoid dividing in such > cases. Check. > * It would help performance to compute invalpha = 1.0f/alpha; and then > multiply by that instead of doing 3 divisions. Check. > * I think the predivide flag from the scene should be set in a few > more places, like for compositing out nodes, viewer nodes, .. . Now > it's only being done on rendering an animation to disk. It seems to take there when set in the Shading panel according to tests. > * It's also not clear to me yet that the predivide happens in all > places it's needed, will need to check this more closely. Not sure we will ever know that with certainty within our current system. Luckily with the solid test images, relevant developers and artists can test the impact, and perhaps even tag code paths with comments. Unfortunately there are two layers of complexity here, and they both look extremely similar to the eye: 1) Blender not properly handling premultiplied alpha in all instances via GL calls (GL_ONE versus GL_SRC_ALPHA in glBlendFunc as an example) and Image / ImBuf flags. 2) Blender not handling linearization with premultiplied alpha images correctly. Coupled with above. > I think of this as a temporary solution that we have to leave off by > default for compatibility now, but should be turned on by default once > we have a better premul/key system, where we actually know which > buffers are in which alpha format. Agree. If the flags are available and cover 80% use case, it should be sufficient for the time being. With respect, TJS _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
