[Bf-committers] REVIEW: Predivide for premultiplied images patch. ATTN: Brecht

Troy Sobotka troy.sobotka at gmail.com
Fri Sep 30 18:16:50 CEST 2011


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
<brechtvanlommel at pandora.be> 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


More information about the Bf-committers mailing list