Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(32)

Issue 2762643004: Separate zeroing pixel data from setting alpha

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by cblume
Modified:
5 months ago
CC:
chromium-reviews, blink-reviews, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6

Patch Set 2 : Allocate pixels to their set alpha, remove out-dated comment #

Patch Set 3 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -9 lines) Patch
M third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp View 1 2 chunks +8 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp View 1 2 chunks +1 line, -4 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 18 (11 generated)
cblume
PTAL
5 months ago (2017-03-20 20:34:19 UTC) #4
Peter Kasting OOO thru Aug 22
LGTM
5 months ago (2017-03-20 20:43:53 UTC) #7
Noel Gordon
LGTM - with nits. https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp#newcode127 third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: // setSize() calls eraseARGB(), which ...
5 months ago (2017-03-20 21:09:03 UTC) #8
cblume
https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp#newcode113 third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:113: m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType, I wonder if this ...
5 months ago (2017-03-20 22:40:28 UTC) #11
Peter Kasting OOO thru Aug 22
https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp#newcode127 third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: // setSize() calls eraseARGB(), which resets the alpha flag, ...
5 months ago (2017-03-20 22:42:04 UTC) #12
cblume
https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2762643004/diff/1/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp#newcode127 third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:127: // setSize() calls eraseARGB(), which resets the alpha flag, ...
5 months ago (2017-03-20 23:14:01 UTC) #15
Noel Gordon
5 months ago (2017-03-22 23:44:55 UTC) #18
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b