Separate zeroing pixel data from setting alpha

Separate zeroing pixel data from setting alpha

Zeroing the pixel data should only affect values.
Setting the alpha on or off should only effect behavior.

For example, if I have an opaque image and I set the pixel values to 0
my image should appear black. But we currently also enable the alpha in
this case which will make my image appear transparent. That forces me to
reset the alpha back to disabled after zeroing the pixel data.

BUG=703304

[cblume, 2017-03-20 20:29:07]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 20:29:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2017-03-20 20:34:18]

cblume@chromium.org changed reviewers:
+ noel@chromium.org, pkasting@chromium.org, scroggo@chromium.org

[cblume, 2017-03-20 20:34:19]

PTAL

[Peter Kasting, 2017-03-20 20:43:01]

Description was changed from

==========
Separate zeroing pixel data from setting alpha

Zeroing the pixel data should only effect values.
Setting the alpha on or off should only effect behavior.

For example, if I have an opaque image and I set the pixel values to 0
my image should appear black. But we currently also enable the alpha in
this case which will amek my image appear transparent. That forces me to
reset the alpha back to disabled after zeroing the pixel data.

BUG=703304
==========

to

==========
Separate zeroing pixel data from setting alpha

Zeroing the pixel data should only affect values.
Setting the alpha on or off should only effect behavior.

For example, if I have an opaque image and I set the pixel values to 0
my image should appear black. But we currently also enable the alpha in
this case which will amek my image appear transparent. That forces me to
reset the alpha back to disabled after zeroing the pixel data.

BUG=703304
==========

[Peter Kasting, 2017-03-20 20:43:13]

Description was changed from

==========
Separate zeroing pixel data from setting alpha

Zeroing the pixel data should only affect values.
Setting the alpha on or off should only effect behavior.

For example, if I have an opaque image and I set the pixel values to 0
my image should appear black. But we currently also enable the alpha in
this case which will amek my image appear transparent. That forces me to
reset the alpha back to disabled after zeroing the pixel data.

BUG=703304
==========

to

==========
Separate zeroing pixel data from setting alpha

Zeroing the pixel data should only affect values.
Setting the alpha on or off should only effect behavior.

For example, if I have an opaque image and I set the pixel values to 0
my image should appear black. But we currently also enable the alpha in
this case which will make my image appear transparent. That forces me to
reset the alpha back to disabled after zeroing the pixel data.

BUG=703304
==========

[Peter Kasting, 2017-03-20 20:43:53]

LGTM

[Noel Gordon, 2017-03-20 21:09:03]

LGTM - with nits.

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: //
setSize() calls eraseARGB(), which resets the alpha flag, so we force
Comment seems out-of-date.

[commit-bot: I haz the power, 2017-03-20 22:11:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 22:11:10]

Dry run: This issue passed the CQ dry run.

[cblume, 2017-03-20 22:40:28]

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:113:
m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,
I wonder if this line should change too.

Before changes in other CLs, this piece of code would be followed by a call to
zeroFillPixelData().

Really, the SkImageInfo could be kPremul, kUnpremul, or kOpaque.

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: //
setSize() calls eraseARGB(), which resets the alpha flag, so we force
On 2017/03/20 21:09:03, noel gordon wrote:
> Comment seems out-of-date.

Oh, you're right.
In fact, the comment indicates that the following m_buffer->setHasAlpha(false);
is no longer required.

It sounds like it was only there because the old setSizeAndColorSpace() would
reset alpha to true. And this line had to set it back.

I think the comment only exists to explain that line. I think we can get rid of
both.

[Peter Kasting, 2017-03-20 22:42:04]

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: //
setSize() calls eraseARGB(), which resets the alpha flag, so we force
On 2017/03/20 22:40:28, cblume wrote:
> On 2017/03/20 21:09:03, noel gordon wrote:
> > Comment seems out-of-date.
> 
> Oh, you're right.
> In fact, the comment indicates that the following
m_buffer->setHasAlpha(false);
> is no longer required.
> 
> It sounds like it was only there because the old setSizeAndColorSpace() would
> reset alpha to true. And this line had to set it back.
> 
> I think the comment only exists to explain that line. I think we can get rid
of
> both.

Just be careful that at that point the code has an undocumented implicit
assumption that frames are opaque by default.  If we wanted to change that
assumption globally, nothing would make it obvious that the code here would be
impacted.

[cblume, 2017-03-20 23:13:07]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 23:13:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2017-03-20 23:14:01]

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: //
setSize() calls eraseARGB(), which resets the alpha flag, so we force
On 2017/03/20 22:42:04, Peter Kasting wrote:
> On 2017/03/20 22:40:28, cblume wrote:
> > On 2017/03/20 21:09:03, noel gordon wrote:
> > > Comment seems out-of-date.
> > 
> > Oh, you're right.
> > In fact, the comment indicates that the following
> m_buffer->setHasAlpha(false);
> > is no longer required.
> > 
> > It sounds like it was only there because the old setSizeAndColorSpace()
would
> > reset alpha to true. And this line had to set it back.
> > 
> > I think the comment only exists to explain that line. I think we can get rid
> of
> > both.
> 
> Just be careful that at that point the code has an undocumented implicit
> assumption that frames are opaque by default.  If we wanted to change that
> assumption globally, nothing would make it obvious that the code here would be
> impacted.

You are right.
I think it would be better for the decoder to explicitly set the frames to what
they want.

I'll look at locations to do that. Maybe each decoder knows what it wants its
default to be when initializing the frame.

[commit-bot: I haz the power, 2017-03-21 00:56:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 00:56:36]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-03-22 23:44:55]

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:113:
m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,
On 2017/03/20 22:40:28, cblume wrote:
> I wonder if this line should change too.

This came after my reply #8.  Not sure why you'd want to change this line ...

> Before changes in other CLs, this piece of code would be followed by a call to
> zeroFillPixelData().

allocatePixelData() sets the backing store bitmpap to

  m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,

so the backing store states it "has alpha" at the Skia level.  But the backing
store pixels are not yet initialized, we're in an inconsistent state.

Enter zeroFillPixelData() ... it is the step that makes that "has alpha" at Skia
level true.  It was done in this code before, then split out as a separate call
as we know in another CL, and done straight after.

The effect must be the same nonetheless.  The zero fill, would fill the buffer
with zero, and would set m_alpha to true.  The zero fill aspect is key.  It
fills the backing store pixel buffer with RGBA(0,0,0,0).  This value is referred
to in the web specs as "transparent black" and it certainly does have alpha.

Once zeroFillPixelData() is done, the backing store "has alpha" Skia state, due
to this earlier line

  m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,

actually becomes true, and consistent, since the backing store is filled with
transparent black (thus, the backing store does in fact have alpha).

> Really, the SkImageInfo could be kPremul, kUnpremul, or kOpaque.

One must be careful setting it kOpaque though.  While there are undecoded
regions in an ImageFrame, the backing store must have kPremul || kUnpremul,
viz., alpha.  The renderer can paint the ImageFrame frame while it's undecoded. 
If we twiddle the backing store of a partially decoded ImageFrame to kOpaque,
the result is foo-bar rendering: examples ...

https://trac.webkit.org/changeset/150340/trunk/Source/WebCore/platform/graphi...
 see the last paragraph of the change description.
https://bugs.webkit.org/show_bug.cgi?id=97170
https://bugs.webkit.org/show_bug.cgi?id=78239
https://bugs.chromium.org/p/chromium/issues/detail?id=113171 for some visuals.

The rule is: the undecoded areas of an ImageFrame have alpha for all images
types, that alpha being the backing store value kPremul || kUnpremul. Rendering
breaks if we do not do this.

Some decoders need to do their own alpha tracking during decoding to work out if
an image frame does in fact have alpha.  They only actually know the result of
their alpha tracking when the ImageFrame is fully decoded, and all our decoders
logically do something like this at that point:

   buffer.setHasAlpha(m_alpha_tracking);
   buffer.setStatus(ImageFrame::FrameComplete);

Follow the code and you will see that computeAlphaType() gets invoked as a
result of these calls:

   if (!m_hasAlpha && m_status == FrameComplete)
      return kOpaque_SkAlphaType;   <-- frame must be complete to become kOpaque

   return m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType;

which makes sense to me: skia level backing store alpha can be set kOpaque only
if the frame is complete (fully decoded).  This to prevent said rendering bugs.

^^^^ If you already understand all of the above, my apologies for preaching to
the converted, ignore me :)

> Really, the SkImageInfo could be kPremul, kUnpremul, or kOpaque.

ImageFrame has undecoded regions
   only kPremul or kUnpremul are permitted
ImageFrame is fully decoded (aka m_status == FrameComplete)
   can have kPremul, kUnpremul, or kOpaque

The latest patch I'm reading on this issue now, is doing something with kOpaque
during buffer init (viz., while the image is undecoded). Given the notes above,
that makes me nervous.

Since the code changed after my review #8, maybe I'll need to review again once
you've finalized the code?

[Peter Kasting, 2017-08-29 04:12:17]

Ping.  Rietveld is deprecated.  I want to make sure my queue here is empty. 
What's the status here?

[cblume, 2017-08-29 07:19:58]

On 2017/08/29 04:12:17, Peter Kasting wrote:
> Ping.  Rietveld is deprecated.  I want to make sure my queue here is empty. 
> What's the status here?

Marked as closed.
The purpose of this patch was really to be a step towards SkCodec acceptance,
which we agreed to adopt. So this patch isn't really needed.