Alpha premul pipeline cleanup

Alpha premul pipeline cleanup

This assumptions are now made:
- Internally float buffers are always linear alpha-premul colors
- Readers should worry about delivering float buffers with that
  assumptions.
- There's an input image setting to say whether it's stored with
  straight/premul alpha on the disk.
- Byte buffers are now assumed have straight alpha, readers should
  deliver straight alpha.

Some implementation details:

- Removed scene's color unpremultiply setting, which was very
  much confusing and was wrong for default settings.
  Now all renderers assumes to deliver premultiplied alpha.

- IMB_buffer_byte_from_float will now linearize alpha when
  converting from buffer.

- Sequencer's effects were changed to assume bytes have got
  straight alpha. Most of effects will work with bytes still,
  however for glow it was more tricky to avoid data loss, so
  there's a commented out glow implementation which converts
  byte buffer to floats first, operates on floats and returns
  bytes back. It's slower and not sure if it should actually
  be used -- who're using glow on alpha anyway?

- Sequencer modifiers should also be working nice with straight
  bytes now.

- GLSL preview will predivide float textures to make nice shading,
  shading with byte textures worked nice (GLSL was assuming straight
  alpha).

- Blender Internal will set alpha=1 to the whole sky. The same
  happens in Cycles and there's no way to avoid this -- sky is
  neither straight nor premul and doesn't fit color pipeline well.

- Straight alpha mode for render result was also eliminated.

[brechtvl, 2012-12-28 13:04:26]

https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/fallback_imp...
File intern/opencolorio/fallback_impl.cc (right):

https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/fallback_imp...
intern/opencolorio/fallback_impl.cc:266: inv_alpha = alpha != 0.0f ? 1.0f /
alpha : 1.0f;
This test is unnecessary given that it already checks for pixel[3] == 0.0f
above.

https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/ocio_impl.cc
File intern/opencolorio/ocio_impl.cc (right):

https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/ocio_impl.cc...
intern/opencolorio/ocio_impl.cc:408: inv_alpha = alpha != 0.0f ? 1.0f / alpha :
1.0f;
Same here

https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
File source/blender/makesrna/intern/rna_scene.c (left):

https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
source/blender/makesrna/intern/rna_scene.c:3336: -		{R_ALPHAKEY, "STRAIGHT", 0,
"Straight Alpha", "Transparent RGB and alpha pixels are unmodified"},
I think this option could become "Sky" and "Transparent". In the tooltip for
transparent it can perhaps mentioned that the alpha is premultiplied.

https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
File source/blender/makesrna/intern/rna_sequencer.c (right):

https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
source/blender/makesrna/intern/rna_sequencer.c:1545: {SEQ_ALPHA_STRAIGHT,
"STRAIGHT", 0, "Straight", "Transparent RGB and alpha pixels are unmodified"},
Suggestions to make the description more clear:
RGB channels in transparent pixels are unaffected by the alpha channel

https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
source/blender/makesrna/intern/rna_sequencer.c:1546: {SEQ_ALPHA_PREMUL,
"PREMUL", 0, "Premultiplied", "Transparent RGB pixels are multiplied by the
alpha channel"},
RGB channels in transparent pixels are multiplied by the alpha channel

[brechtvl, 2012-12-28 13:31:00]

With the extra premultiplication in ibuf_get_color texture filtering works
mostly correct, however mipmapping (IMB_makemipmap) is still wrong. There it
should also premul/unpremul as it scales down the image, to get correct
interpolation.

A compatibility issue here exists also when users have set up an image as a
texture but don't want to use its alpha channel (Use alpha disabled). In this
case premultiplying in texture filtering will make some areas black. Maybe this
is acceptable, or maybe the texture filtering should automatically skip the
premul in that case? For mipmaps it's a bit weak if the image is used by
multiple textures with different settings, but usually would go ok to skip it.

In compositing and the sequencer, color correction type nodes should have an
option to unpremul the color. Not necessarily something that should be done as
part of this patch, just making a note.

[brechtvl, 2012-12-28 14:22:34]

https://codereview.appspot.com/7018048/diff/1/source/blender/render/intern/so...
File source/blender/render/intern/source/imagetexture.c (right):

https://codereview.appspot.com/7018048/diff/1/source/blender/render/intern/so...
source/blender/render/intern/source/imagetexture.c:106: /* bytes are internally
straight, however render pipeline seems to expect premul */
Texture filtering indeed must be premul, it's not just that the render pipeline
expects it, it's the only way to do correct interpolation of RGBA pixels.

ibuf_get_color_clip should get the same modification.

[brechtvl, 2012-12-28 14:25:43]

* Are we handling 16 bit PNG images correctly, are they convert from straight to
premultiplied on load and vice versa for loading?
* For TIFF we should also read/write the metadata about the alpha type?
* Is the image painting code working correct for premul float and straight byte?

[sergey.vfx, 2012-12-29 20:30:27]

Corrections to most of comments

[sergey.vfx, 2012-12-29 20:31:42]

On 2012/12/28 13:04:26, brechtvl wrote:
>
https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/fallback_imp...
> File intern/opencolorio/fallback_impl.cc (right):
> 
>
https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/fallback_imp...
> intern/opencolorio/fallback_impl.cc:266: inv_alpha = alpha != 0.0f ? 1.0f /
> alpha : 1.0f;
> This test is unnecessary given that it already checks for pixel[3] == 0.0f
> above.
> 
> https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/ocio_impl.cc
> File intern/opencolorio/ocio_impl.cc (right):
> 
>
https://codereview.appspot.com/7018048/diff/1/intern/opencolorio/ocio_impl.cc...
> intern/opencolorio/ocio_impl.cc:408: inv_alpha = alpha != 0.0f ? 1.0f / alpha
:
> 1.0f;
> Same here
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
> File source/blender/makesrna/intern/rna_scene.c (left):
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
> source/blender/makesrna/intern/rna_scene.c:3336: -		{R_ALPHAKEY, "STRAIGHT",
0,
> "Straight Alpha", "Transparent RGB and alpha pixels are unmodified"},
> I think this option could become "Sky" and "Transparent". In the tooltip for
> transparent it can perhaps mentioned that the alpha is premultiplied.
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
> File source/blender/makesrna/intern/rna_sequencer.c (right):
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
> source/blender/makesrna/intern/rna_sequencer.c:1545: {SEQ_ALPHA_STRAIGHT,
> "STRAIGHT", 0, "Straight", "Transparent RGB and alpha pixels are unmodified"},
> Suggestions to make the description more clear:
> RGB channels in transparent pixels are unaffected by the alpha channel
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/makesrna/intern/...
> source/blender/makesrna/intern/rna_sequencer.c:1546: {SEQ_ALPHA_PREMUL,
> "PREMUL", 0, "Premultiplied", "Transparent RGB pixels are multiplied by the
> alpha channel"},
> RGB channels in transparent pixels are multiplied by the alpha channel

Corrected all of this comments.

[sergey.vfx, 2012-12-29 20:33:47]

On 2012/12/28 13:31:00, brechtvl wrote:
> With the extra premultiplication in ibuf_get_color texture filtering works
> mostly correct, however mipmapping (IMB_makemipmap) is still wrong. There it
> should also premul/unpremul as it scales down the image, to get correct
> interpolation.
> 
> A compatibility issue here exists also when users have set up an image as a
> texture but don't want to use its alpha channel (Use alpha disabled). In this
> case premultiplying in texture filtering will make some areas black. Maybe
this
> is acceptable, or maybe the texture filtering should automatically skip the
> premul in that case? For mipmaps it's a bit weak if the image is used by
> multiple textures with different settings, but usually would go ok to skip it.

Hrm. Passing Use Alpha flag here could be really tricky i'm afraid. Also, didn't
we want to add option to force alpha=1 on load, or it should be just additional
option?

[sergey.vfx, 2012-12-29 20:34:01]

On 2012/12/28 14:22:34, brechtvl wrote:
>
https://codereview.appspot.com/7018048/diff/1/source/blender/render/intern/so...
> File source/blender/render/intern/source/imagetexture.c (right):
> 
>
https://codereview.appspot.com/7018048/diff/1/source/blender/render/intern/so...
> source/blender/render/intern/source/imagetexture.c:106: /* bytes are
internally
> straight, however render pipeline seems to expect premul */
> Texture filtering indeed must be premul, it's not just that the render
pipeline
> expects it, it's the only way to do correct interpolation of RGBA pixels.
> 
> ibuf_get_color_clip should get the same modification.

This is done.

[sergey.vfx, 2012-12-29 20:45:57]

On 2012/12/28 14:25:43, brechtvl wrote:
> * Are we handling 16 bit PNG images correctly, are they convert from straight
to
> premultiplied on load and vice versa for loading?

16bit png should work now just fine.

Also, i do have pretty finished patch to support saving 16bit PNG from blender,
but that's another story.

> * For TIFF we should also read/write the metadata about the alpha type?

Eeeh.. Looked into this and ended up with complete misunderstanding of TIFF
logic. With current code if we'll save 8 bit TIFF and load it back, we'll need
to use Straight alpha mode, and for 16 bit TIFF rendered and opened in blender
we'll set alpha to Premul, which correlates with our assumptions. However, 8 bit
TIFF would be opened incorrect in Gimp, but will show correct in Gwenview
(default KDE image browser).

Now, if at save time we'll use unassociated flag for 8 bit tiff, it'll be
displayed correct in Gimp. But we'll need to use Premul alpha mode when opening
it back in blender. And it will also look incorrect in Gwenview.

So there's something crappy going on here and i can't figure this out yet.

> * Is the image painting code working correct for premul float and straight
byte?

Float painting seems to behave properly. At least it seems it's behaving the
same way as krita/gimp.

However, byte buffers behaves quite fuzzy. When i do a stroke with white brush
on complete transparent image, i should see the whole stroke being white in RGB
display mode, but it fades up to darkish colors to the sides of brush.
Converting pixels from straight-byte to premul-float in IMB_rectblend and using
IMB_blend_color_float doesn't give correct result here.

Afraid that's the part of patch where i do need some help because i've currently
run out of ideas what's going on there.

[xavier.thomas.1980_gmail.com, 2012-12-29 22:41:44]

> However, byte buffers behaves quite fuzzy. When i do a stroke with white
> brush on complete transparent image, i should see the whole stroke being
> white in RGB display mode, but it fades up to darkish colors to the
> sides of brush. Converting pixels from straight-byte to premul-float in
> IMB_rectblend and using IMB_blend_color_float doesn't give correct
> result here.


Darkish colors in the semitransparent areas is often a sign of having
premultiplied colors by alpha twice or more.

[sergey.vfx, 2012-12-29 23:06:16]

On 2012/12/29 22:41:44, xavier.thomas.1980_gmail.com wrote:
> > However, byte buffers behaves quite fuzzy. When i do a stroke with white
> > brush on complete transparent image, i should see the whole stroke being
> > white in RGB display mode, but it fades up to darkish colors to the
> > sides of brush. Converting pixels from straight-byte to premul-float in
> > IMB_rectblend and using IMB_blend_color_float doesn't give correct
> > result here.
> 
> 
> Darkish colors in the semitransparent areas is often a sign of having
> premultiplied colors by alpha twice or more.

I know this and it doesn't seem to be this case. If you'll try the patch you'll
notice that blending is much more "steppy" than just multiplying alpha twice. It
could be extra premul, but there seems to be some other issue involved here.

[xavier.thomas.1980_gmail.com, 2012-12-29 23:06:56]

The double can come from the painting too.

If you paint on an fully transparant image, what would be the background
without taking alpha into consideration?
Black I suppose (If white than the problem would be present when painting
black)
Now you paint white on it with a brush with soft-edges.
If the painting code mix the white (the color you are painting with) with
the black (the supposed background color that we cannot see) on the edges
of the brush.
Than you premultiply for display -> blak border.

The painting code should paint full white even on the border of the brush
and should not blend the painting color with the background, but this only
when painting on area that where fully transparant befor painting. For
areas that where opaque already before painting the  blend should occur.
But for it to work properly in any case -> the painting code should be
modifed and take the "old" alpha value into account for modifying colors.

That's where I think the problem comes from anyway.


2012/12/29 Xavier Thomas <xavier.thomas.1980@gmail.com>

>
> However, byte buffers behaves quite fuzzy. When i do a stroke with white
>> brush on complete transparent image, i should see the whole stroke being
>> white in RGB display mode, but it fades up to darkish colors to the
>> sides of brush. Converting pixels from straight-byte to premul-float in
>> IMB_rectblend and using IMB_blend_color_float doesn't give correct
>> result here.
>
>
> Darkish colors in the semitransparent areas is often a sign of having
> premultiplied colors by alpha twice or more.
>

[xavier.thomas.1980_gmail.com, 2012-12-29 23:11:39]

I am patching now, but I cannot promess I will have time to debug :(


2012/12/29 <sergey.vfx@gmail.com>

> On 2012/12/29 22:41:44, xavier.thomas.1980_gmail.com wrote:
>
>> > However, byte buffers behaves quite fuzzy. When i do a stroke with
>>
> white
>
>> > brush on complete transparent image, i should see the whole stroke
>>
> being
>
>> > white in RGB display mode, but it fades up to darkish colors to the
>> > sides of brush. Converting pixels from straight-byte to premul-float
>>
> in
>
>> > IMB_rectblend and using IMB_blend_color_float doesn't give correct
>> > result here.
>>
>
>
>  Darkish colors in the semitransparent areas is often a sign of having
>> premultiplied colors by alpha twice or more.
>>
>
> I know this and it doesn't seem to be this case. If you'll try the patch
> you'll notice that blending is much more "steppy" than just multiplying
> alpha twice. It could be extra premul, but there seems to be some other
> issue involved here.
>
>
https://codereview.appspot.**com/7018048/<https://codereview.appspot.com/7018...
>

[xavier.thomas.1980_gmail.com, 2012-12-30 03:24:07]

Hi,

I made some tests and read most of the painting code but did not had time
to code a fix yet.

Here are my finding so far.

Create an new image in the uv/image editor (opaque black)
Paint the lower half of the image in white (still no alpha)
Use the "Erase Alpha" paint mode to make the whole image transparent.

--> In the view (RGB whit no alpha) it displays all black, the white and
transparant part of the image is lost.
This makes me think that even the image is byte with straight colors, not
colormanged  and need to be displayed with straight colors it is somehow
modified.

Then painting white or black stroke over the image works ok, or not
depending if you paint white on white or white on black or the other way
around.

Images are better than a 1000 words:

http://www.pasteall.org/pic/42616
http://www.pasteall.org/pic/42617


2012/12/29 Xavier Thomas <xavier.thomas.1980@gmail.com>

> I am patching now, but I cannot promess I will have time to debug :(
>
>
> 2012/12/29 <sergey.vfx@gmail.com>
>
> On 2012/12/29 22:41:44, xavier.thomas.1980_gmail.com wrote:
>>
>>> > However, byte buffers behaves quite fuzzy. When i do a stroke with
>>>
>> white
>>
>>> > brush on complete transparent image, i should see the whole stroke
>>>
>> being
>>
>>> > white in RGB display mode, but it fades up to darkish colors to the
>>> > sides of brush. Converting pixels from straight-byte to premul-float
>>>
>> in
>>
>>> > IMB_rectblend and using IMB_blend_color_float doesn't give correct
>>> > result here.
>>>
>>
>>
>>  Darkish colors in the semitransparent areas is often a sign of having
>>> premultiplied colors by alpha twice or more.
>>>
>>
>> I know this and it doesn't seem to be this case. If you'll try the patch
>> you'll notice that blending is much more "steppy" than just multiplying
>> alpha twice. It could be extra premul, but there seems to be some other
>> issue involved here.
>>
>>
https://codereview.appspot.**com/7018048/<https://codereview.appspot.com/7018...
>>
>
>

[brechtvl, 2012-12-30 10:20:44]

On 2012/12/29 20:33:47, sergey.vfx wrote:
> On 2012/12/28 13:31:00, brechtvl wrote:
> > With the extra premultiplication in ibuf_get_color texture filtering works
> > mostly correct, however mipmapping (IMB_makemipmap) is still wrong. There it
> > should also premul/unpremul as it scales down the image, to get correct
> > interpolation.
> > 
> > A compatibility issue here exists also when users have set up an image as a
> > texture but don't want to use its alpha channel (Use alpha disabled). In
this
> > case premultiplying in texture filtering will make some areas black. Maybe
> this
> > is acceptable, or maybe the texture filtering should automatically skip the
> > premul in that case? For mipmaps it's a bit weak if the image is used by
> > multiple textures with different settings, but usually would go ok to skip
it.
> 
> Hrm. Passing Use Alpha flag here could be really tricky i'm afraid. Also,
didn't
> we want to add option to force alpha=1 on load, or it should be just
additional
> option?

I think force alpha 1.0 on load is ok, which could replace the "Use Alpha"
option on the texture. You can't get perfect compatibility then but in most
cases it would be fine if the image is only used for the texture.

[brechtvl, 2012-12-30 10:32:10]

On 2012/12/29 20:45:57, sergey.vfx wrote:
> > * For TIFF we should also read/write the metadata about the alpha type?
> 
> Eeeh.. Looked into this and ended up with complete misunderstanding of TIFF
> logic. With current code if we'll save 8 bit TIFF and load it back, we'll need
> to use Straight alpha mode, and for 16 bit TIFF rendered and opened in blender
> we'll set alpha to Premul, which correlates with our assumptions. However, 8
bit
> TIFF would be opened incorrect in Gimp, but will show correct in Gwenview
> (default KDE image browser).
> 
> Now, if at save time we'll use unassociated flag for 8 bit tiff, it'll be
> displayed correct in Gimp. But we'll need to use Premul alpha mode when
opening
> it back in blender. And it will also look incorrect in Gwenview.
> 
> So there's something crappy going on here and i can't figure this out yet.

I think Gimp might not be reading the metadata then, since it seems we are
currently writing that the image as associated alpha (= premultiplied alpha):
int extraSampleTypes[1] = { EXTRASAMPLE_ASSOCALPHA };

We should change that to EXTRASAMPLE_UNASSALPHA for byte images I think.

> However, byte buffers behaves quite fuzzy. When i do a stroke with white brush
> on complete transparent image, i should see the whole stroke being white in
RGB
> display mode, but it fades up to darkish colors to the sides of brush.
> Converting pixels from straight-byte to premul-float in IMB_rectblend and
using
> IMB_blend_color_float doesn't give correct result here.
> 
> Afraid that's the part of patch where i do need some help because i've
currently
> run out of ideas what's going on there.

This might actually just be a precision issue that we can't fix, thought it
depends what you are testing exactly.  2D painting in the image editor currently
always works as if using an Airbrush in Gimp. When using a low strength this
will give very clear rounding issues that you will also find in Gimp using the
same tool.

There might be another issue here as well though, I didn't test yet.

[sergey.vfx, 2012-12-30 10:50:26]

> I think Gimp might not be reading the metadata then, since it seems we are
> currently writing that the image as associated alpha (= premultiplied alpha):
> int extraSampleTypes[1] = { EXTRASAMPLE_ASSOCALPHA };
> 
> We should change that to EXTRASAMPLE_UNASSALPHA for byte images I think.

Gimp does read metadata, but could be bug there yes. Not sure if Gwenview reads
metadata tho. Strangest thing last night was that it seems setting alpha mode to
TIFF didn't just wrote metadata, but also affected on pixels. At least that was
an expression of mine lats night.

I'm currently working on a slightly different task (cleaning PNG 16bit write
support), will look into TIFF again after that. Don't spend time on your own
with this. I only mentioned current issues with TIFF, just to show what else
remained to be solved before commit and if you'll have time would rather ask for
help with painting since i'm not so much up to this area.

> This might actually just be a precision issue that we can't fix, thought it
> depends what you are testing exactly.  2D painting in the image editor
currently
> always works as if using an Airbrush in Gimp. When using a low strength this
> will give very clear rounding issues that you will also find in Gimp using the
> same tool.
> 
> There might be another issue here as well though, I didn't test yet.

I'm just doing stroke of white brush with strength=1 over a transparent byte
image with black color.

[sergey.vfx, 2012-12-30 15:55:46]

Updated patch, detailed comments later

[sergey.vfx, 2012-12-30 15:57:51]

In updated patch there is:

- Alpha mode is set properly to TIFF files. There's one hack in loading files to
prevent color premultiplication in the core of libtiff (which seems always
output premul color, even for bytes)

- Detect alpha mode from file when first time opening it.

- Added compatibility code for sequencer

[sergey.vfx, 2012-12-31 13:08:31]

Updates for Use Alpha texture settings. Basically replaces by per-image
datablock use Alpha.

[sergey.vfx, 2012-12-31 13:54:08]

Commited to svn.