Refactor ImageFrame::setSizeAndColorSpace()

Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

No change in behavior, no new tests.

R=scroggo@chromium.org
BUG=701143
Review-Url: https://codereview.chromium.org/2749703002
Cr-Commit-Position: refs/heads/master@{#458437}
Committed: https://chromium.googlesource.com/chromium/src/+/b6b96c3de7c6db78762ce660b274514bd18a1c0b

[cblume, 2017-03-13 22:55:05]

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

[commit-bot: I haz the power, 2017-03-13 22:55:58]

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-13 22:56:07]

Description was changed from

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

R=scroggo@chromium.org
BUG=701143
==========

to

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

R=scroggo@chromium.org
BUG=701143
==========

[cblume, 2017-03-13 22:56:07]

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

[commit-bot: I haz the power, 2017-03-13 23:39:32]

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

[commit-bot: I haz the power, 2017-03-13 23:39:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[cblume, 2017-03-14 02:44:54]

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

[commit-bot: I haz the power, 2017-03-14 02:45:22]

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

[commit-bot: I haz the power, 2017-03-14 04:14:05]

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

[commit-bot: I haz the power, 2017-03-14 04:14:05]

Dry run: This issue passed the CQ dry run.

[cblume, 2017-03-14 04:22:45]

PTAL

[Peter Kasting, 2017-03-14 04:55:37]

LGTM

[Noel Gordon, 2017-03-14 06:30:42]

So I grok the idea:

> We want to be able to not zero fill the entire image. We will be filling the
whole image with pixel data anyway. 

Easy case (whole image decoded at paint-time) but shaves 2-5% off the cost of an
image decode by avoiding the zero fill.

> In the case of partial decodes we can manually zero fill only the portion that
does not have image data.

Manually fill the undecoded regions of the image, say when the Blink renderer
paints an image buffer before it is fully decoded (and it sure does do that). 
That's the tricky case, and this patch will help you explore that problem case I
assume?

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize() on
the image buffer well before we get here these days, right?  (Please check).

If that is true, that'd imply there is a width() and height(), so perhaps the
right DCHECK to use here now is

   DCHECK(isDecodedSizeAvailble());

or some such?

Second thing is: the original DCHECK was there to prevent this routine from
being called twice, since it did actually call setSize() back in the day, and
calling it twice did in fact leak memory.

Not sure if tryAllocPixel prevents such leaks, but that doesn't really matter
either if the rule is "only call the routine once".  No leaks then.  And I'd be
semi-suspicious on any code that tried calling this routine twice.  To enforce a
call-once rule, perhaps

   DCHECK(m_status == ImageFrame::FrameEmpty);

would get us part-way there.  That might bust MockImageDecoder mind, but we
could fix that, or find some other way to say with a DCHECK(),
call-me-once-only.

Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it as a
name.  The code appears to set|setup|create the backing store for the
ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
future.

So to begin the bike shed, maybe we should call it:

  createBackingStore(width, height, colorSpace);

or variations on that theme (createPixelStore or whatever).

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:944:
buffer.zeroFillPixelData();
Here and elsewhere you made this change, consider using

buffer.zeroFillPixelData();
DCHECK(buffer.hasAlpha());

to maybe remind everyone of that important contract?  (Since it so often comes
as a discussion point in image decoder changes).

[Peter Kasting, 2017-03-14 07:34:40]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:944:
buffer.zeroFillPixelData();
On 2017/03/14 06:30:41, noel gordon wrote:
> Here and elsewhere you made this change, consider using
> 
> buffer.zeroFillPixelData();
> DCHECK(buffer.hasAlpha());
> 
> to maybe remind everyone of that important contract?  (Since it so often comes
> as a discussion point in image decoder changes).

Hmm.

It worries me that zeroFillPixelData() sets m_hasAlpha = true, but setHasAlpha()
also calls m_bitmap.setAlphaType().  DCHECK(buffer.hasAlpha()) implies one
doesn't need to call setHasAlpha(true), as we currently do e.g. below.  But
unless the eraseARGB() call in zeroFillPixelData() does this automatically,
omitting that will subtly modify the effect of the code.

I wonder if zeroFillPixelData() should be calling setHasAlpha(true) instead of
just twiddling m_hasAlpha.

[cblume, 2017-03-14 07:59:36]

On 2017/03/14 06:30:42, noel gordon wrote:
> So I grok the idea:
> 
> > We want to be able to not zero fill the entire image. We will be filling the
> whole image with pixel data anyway. 
> 
> Easy case (whole image decoded at paint-time) but shaves 2-5% off the cost of
an
> image decode by avoiding the zero fill.
> 
> > In the case of partial decodes we can manually zero fill only the portion
that
> does not have image data.
> 
> Manually fill the undecoded regions of the image, say when the Blink renderer
> paints an image buffer before it is fully decoded (and it sure does do that). 
> That's the tricky case, and this patch will help you explore that problem case
I
> assume?

This is correct.
I don't expect much gain at all. I'm not even sure it will be noticeable.
However, while working on the SkGIFDecoder it fit so naturally that it would be
a shame not to take it.

[cblume, 2017-03-14 07:59:51]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 06:30:41, noel gordon wrote:
> Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize() on
> the image buffer well before we get here these days, right?  (Please check).
> 
> If that is true, that'd imply there is a width() and height(), so perhaps the
> right DCHECK to use here now is
> 
>    DCHECK(isDecodedSizeAvailble());
> 
> or some such?
> 
> Second thing is: the original DCHECK was there to prevent this routine from
> being called twice, since it did actually call setSize() back in the day, and
> calling it twice did in fact leak memory.
> 
> Not sure if tryAllocPixel prevents such leaks, but that doesn't really matter
> either if the rule is "only call the routine once".  No leaks then.  And I'd
be
> semi-suspicious on any code that tried calling this routine twice.  To enforce
a
> call-once rule, perhaps
> 
>    DCHECK(m_status == ImageFrame::FrameEmpty);
> 
> would get us part-way there.  That might bust MockImageDecoder mind, but we
> could fix that, or find some other way to say with a DCHECK(),
> call-me-once-only.
> 
> Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it as
a
> name.  The code appears to set|setup|create the backing store for the
> ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
> future.
> 
> So to begin the bike shed, maybe we should call it:
> 
>   createBackingStore(width, height, colorSpace);
> 
> or variations on that theme (createPixelStore or whatever).

I was talking to scroggo@ about the don't-leak-memory thing. I think you are
right, it will be backed by a SkBitmap and will not leak. That is, unless an
external allocator is supplied. Then I'm not sure if it could leak.

And I agree with renaming the function to something more appropriate now.

[f(malita), 2017-03-14 13:18:05]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 07:59:51, cblume wrote:
> On 2017/03/14 06:30:41, noel gordon wrote:
> > Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it
as
> a
> > name.  The code appears to set|setup|create the backing store for the
> > ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
> > future.
> > 
> > So to begin the bike shed, maybe we should call it:
> > 
> >   createBackingStore(width, height, colorSpace);
> > 
> > or variations on that theme (createPixelStore or whatever).
> 

> And I agree with renaming the function to something more appropriate now.

+1 to name change.  Since this is supposed to be one-time initialization, I
would also consider init() or such.

[Noel Gordon, 2017-03-14 14:26:26]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 13:18:05, f(malita) wrote:
> On 2017/03/14 07:59:51, cblume wrote:
> > On 2017/03/14 06:30:41, noel gordon wrote:
> > > Agree with the premise of the patch; setSizeAndColorSpace no longer cuts
it
> as
> > a
> > > name.  The code appears to set|setup|create the backing store for the
> > > ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
> > > future.
> > > 
> > > So to begin the bike shed, maybe we should call it:
> > > 
> > >   createBackingStore(width, height, colorSpace);
> > > 
> > > or variations on that theme (createPixelStore or whatever).
> > 
> 
> > And I agree with renaming the function to something more appropriate now.
> 
> +1 to name change.  Since this is supposed to be one-time initialization, I
> would also consider init() or such.

Agree with the one-time initialization part. However, init() might imply that
the pixels in the backing store were initialized also?  Most we do is allocate,
doing a malloc-like allocation ...

  allocateBackingStore, allocateStorage, and so on.

[scroggo_chromium, 2017-03-14 14:28:59]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 06:30:41, noel gordon wrote:
> Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize() on
> the image buffer well before we get here these days, right?  (Please check).
> 
> If that is true, that'd imply there is a width() and height(), so perhaps the
> right DCHECK to use here now is
> 
>    DCHECK(isDecodedSizeAvailble());
> 
> or some such?

I think you're confused - this is ImageFrame, not ImageDecoder. No way to check
isDecodedSizeAvailable() from here, and width() and height() are not yet set.
They are set on the ImageDecoder, but this is where they get set in ImageFrame.

> 
> Second thing is: the original DCHECK was there to prevent this routine from
> being called twice, since it did actually call setSize() back in the day, and
> calling it twice did in fact leak memory.
> 
> Not sure if tryAllocPixel prevents such leaks, but that doesn't really matter
> either if the rule is "only call the routine once".  No leaks then.  And I'd
be
> semi-suspicious on any code that tried calling this routine twice.  To enforce
a
> call-once rule, perhaps
> 
>    DCHECK(m_status == ImageFrame::FrameEmpty);
> 
> would get us part-way there.  That might bust MockImageDecoder mind, but we
> could fix that, or find some other way to say with a DCHECK(),
> call-me-once-only.

The comment is bogus, but width() and height() return the size of the bitmap,
which starts off as 0 x 0 and gets set down below, so I think the DCHECK itself
is fine. Checking m_status is also reasonable, except this method doesn't change
the status.

> 
> Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it as
a
> name.  The code appears to set|setup|create the backing store for the
> ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
> future.
> 
> So to begin the bike shed, maybe we should call it:
> 
>   createBackingStore(width, height, colorSpace);
> 
> or variations on that theme (createPixelStore or whatever).

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 07:59:51, cblume wrote:
> On 2017/03/14 06:30:41, noel gordon wrote:
> > Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize()
on
> > the image buffer well before we get here these days, right?  (Please check).
> > 
> > If that is true, that'd imply there is a width() and height(), so perhaps
the
> > right DCHECK to use here now is
> > 
> >    DCHECK(isDecodedSizeAvailble());
> > 
> > or some such?
> > 
> > Second thing is: the original DCHECK was there to prevent this routine from
> > being called twice, since it did actually call setSize() back in the day,
and
> > calling it twice did in fact leak memory.
> > 
> > Not sure if tryAllocPixel prevents such leaks, but that doesn't really
matter
> > either if the rule is "only call the routine once".  No leaks then.  And I'd
> be
> > semi-suspicious on any code that tried calling this routine twice.  To
enforce
> a
> > call-once rule, perhaps
> > 
> >    DCHECK(m_status == ImageFrame::FrameEmpty);
> > 
> > would get us part-way there.  That might bust MockImageDecoder mind, but we
> > could fix that, or find some other way to say with a DCHECK(),
> > call-me-once-only.
> > 
> > Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it
as
> a
> > name.  The code appears to set|setup|create the backing store for the
> > ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
> > future.
> > 
> > So to begin the bike shed, maybe we should call it:
> > 
> >   createBackingStore(width, height, colorSpace);
> > 
> > or variations on that theme (createPixelStore or whatever).
> 
> I was talking to scroggo@ about the don't-leak-memory thing. I think you are
> right, it will be backed by a SkBitmap and will not leak. That is, unless an
> external allocator is supplied. Then I'm not sure if it could leak.

The ExternalMemoryAllocator supplies an SkPixelRef which does not own its pixel
memory, because it is owned somewhere else. (You can trace through the function
calls to find the actual owner. In the software case at least, it is discardable
memory created by the SoftwareImageDecodeCache.) It would only leak memory if
someone used a custom allocator that does something stupid.

> 
> And I agree with renaming the function to something more appropriate now.

[Noel Gordon, 2017-03-14 14:49:10]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 14:28:59, scroggo_chromium wrote:
> On 2017/03/14 06:30:41, noel gordon wrote:
> > Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize()
on
> > the image buffer well before we get here these days, right?  (Please check).
> > 
> > If that is true, that'd imply there is a width() and height(), so perhaps
the
> > right DCHECK to use here now is
> > 
> >    DCHECK(isDecodedSizeAvailble());
> > 
> > or some such?
> 
> I think you're confused - this is ImageFrame, not ImageDecoder. No way to
check
> isDecodedSizeAvailable() from here, and 

Ah yes, I was using ImageDecoder thinking, not ImageFrame thinking.

width() and height() are not yet set.
> They are set on the ImageDecoder, but this is where they get set in
ImageFrame.

Right.

> > Second thing is: the original DCHECK was there to prevent this routine from
> > being called twice, since it did actually call setSize() back in the day,
and
> > calling it twice did in fact leak memory.
> > 
> > Not sure if tryAllocPixel prevents such leaks, but that doesn't really
matter
> > either if the rule is "only call the routine once".  No leaks then.  And I'd
> be
> > semi-suspicious on any code that tried calling this routine twice.  To
enforce
> a
> > call-once rule, perhaps
> > 
> >    DCHECK(m_status == ImageFrame::FrameEmpty);
> > 
> > would get us part-way there.  That might bust MockImageDecoder mind, but we
> > could fix that, or find some other way to say with a DCHECK(),
> > call-me-once-only.
> 
> The comment is bogus, but width() and height() return the size of the bitmap,
> which starts off as 0 x 0 and gets set down below, so I think the DCHECK
itself
> is fine. 

Agree, now I see that width() and height() are from the m_bitmap. The DCHECK is
fine.  The comment needs fixing, and the routine needs a new name.

[cblume, 2017-03-14 17:30:50]

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
DCHECK(!width() && !height());
On 2017/03/14 14:49:10, noel gordon wrote:
> On 2017/03/14 14:28:59, scroggo_chromium wrote:
> > On 2017/03/14 06:30:41, noel gordon wrote:
> > > Yeah, this DCHECK looks entirely bogus to me now.  We have called
setSize()
> on
> > > the image buffer well before we get here these days, right?  (Please
check).
> > > 
> > > If that is true, that'd imply there is a width() and height(), so perhaps
> the
> > > right DCHECK to use here now is
> > > 
> > >    DCHECK(isDecodedSizeAvailble());
> > > 
> > > or some such?
> > 
> > I think you're confused - this is ImageFrame, not ImageDecoder. No way to
> check
> > isDecodedSizeAvailable() from here, and 
> 
> Ah yes, I was using ImageDecoder thinking, not ImageFrame thinking.
> 
> width() and height() are not yet set.
> > They are set on the ImageDecoder, but this is where they get set in
> ImageFrame.
> 
> Right.
> 
> > > Second thing is: the original DCHECK was there to prevent this routine
from
> > > being called twice, since it did actually call setSize() back in the day,
> and
> > > calling it twice did in fact leak memory.
> > > 
> > > Not sure if tryAllocPixel prevents such leaks, but that doesn't really
> matter
> > > either if the rule is "only call the routine once".  No leaks then.  And
I'd
> > be
> > > semi-suspicious on any code that tried calling this routine twice.  To
> enforce
> > a
> > > call-once rule, perhaps
> > > 
> > >    DCHECK(m_status == ImageFrame::FrameEmpty);
> > > 
> > > would get us part-way there.  That might bust MockImageDecoder mind, but
we
> > > could fix that, or find some other way to say with a DCHECK(),
> > > call-me-once-only.
> > 
> > The comment is bogus, but width() and height() return the size of the
bitmap,
> > which starts off as 0 x 0 and gets set down below, so I think the DCHECK
> itself
> > is fine. 
> 
> Agree, now I see that width() and height() are from the m_bitmap. The DCHECK
is
> fine.  The comment needs fixing, and the routine needs a new name.

Name updated.
Removed the comment.

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:944:
buffer.zeroFillPixelData();
On 2017/03/14 07:34:40, Peter Kasting wrote:
> On 2017/03/14 06:30:41, noel gordon wrote:
> > Here and elsewhere you made this change, consider using
> > 
> > buffer.zeroFillPixelData();
> > DCHECK(buffer.hasAlpha());
> > 
> > to maybe remind everyone of that important contract?  (Since it so often
comes
> > as a discussion point in image decoder changes).
> 
> Hmm.
> 
> It worries me that zeroFillPixelData() sets m_hasAlpha = true, but
setHasAlpha()
> also calls m_bitmap.setAlphaType().  DCHECK(buffer.hasAlpha()) implies one
> doesn't need to call setHasAlpha(true), as we currently do e.g. below.  But
> unless the eraseARGB() call in zeroFillPixelData() does this automatically,
> omitting that will subtly modify the effect of the code.
> 
> I wonder if zeroFillPixelData() should be calling setHasAlpha(true) instead of
> just twiddling m_hasAlpha.

I think both of you had really good points.
I would be happy to remind people that zero filling means we're also talking
about alpha.
But I also don't want to mislead people into thinking they don't need to call a
function which is required.

I don't immediately see a reason why we set m_hasAlpha = true (inside
zeroFillPixelData()) and don't just call setHasAlpha(true). Is there a reason to
set m_hasAlpha = true without also calling m_bitmap.setAlphaType() ?

I feel like it would be lovely if zeroFillPixelData() called
m_bitmap.setAlphaType() and we could get rid of the call to
buffer.setHasAlpha(true) below. But there might be a reason we cannot do that.

[cblume, 2017-03-14 17:57:27]

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

[commit-bot: I haz the power, 2017-03-14 17:57:58]

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

[commit-bot: I haz the power, 2017-03-14 18:04:29]

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

[commit-bot: I haz the power, 2017-03-14 18:04:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cblume, 2017-03-14 18:24:01]

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

[commit-bot: I haz the power, 2017-03-14 18:24:49]

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

[Peter Kasting, 2017-03-14 19:58:20]

On 2017/03/14 17:30:50, cblume wrote:
> I don't immediately see a reason why we set m_hasAlpha = true (inside
> zeroFillPixelData()) and don't just call setHasAlpha(true). Is there a reason
to
> set m_hasAlpha = true without also calling m_bitmap.setAlphaType() ?
> 
> I feel like it would be lovely if zeroFillPixelData() called
> m_bitmap.setAlphaType() and we could get rid of the call to
> buffer.setHasAlpha(true) below. But there might be a reason we cannot do that.

My suspicion without investigating is that setHasAlpha() originally just
twiddled m_hasAlpha, and when the setAlphaType() call was added to it later, no
one realized this meant zeroFillPixelData() should be updated to effectively
perform that operation as well.

In which case, yes, we should just change the current code.

Note that I didn't blame-trawl to verify that hypothesis.  That's just based on
my memory of what the code used to look like years ago.

[commit-bot: I haz the power, 2017-03-14 20:19:48]

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

[commit-bot: I haz the power, 2017-03-14 20:19:49]

Dry run: This issue passed the CQ dry run.

[cblume, 2017-03-14 20:49:45]

On 2017/03/14 19:58:20, Peter Kasting wrote:
> On 2017/03/14 17:30:50, cblume wrote:
> > I don't immediately see a reason why we set m_hasAlpha = true (inside
> > zeroFillPixelData()) and don't just call setHasAlpha(true). Is there a
reason
> to
> > set m_hasAlpha = true without also calling m_bitmap.setAlphaType() ?
> > 
> > I feel like it would be lovely if zeroFillPixelData() called
> > m_bitmap.setAlphaType() and we could get rid of the call to
> > buffer.setHasAlpha(true) below. But there might be a reason we cannot do
that.
> 
> My suspicion without investigating is that setHasAlpha() originally just
> twiddled m_hasAlpha, and when the setAlphaType() call was added to it later,
no
> one realized this meant zeroFillPixelData() should be updated to effectively
> perform that operation as well.
> 
> In which case, yes, we should just change the current code.
> 
> Note that I didn't blame-trawl to verify that hypothesis.  That's just based
on
> my memory of what the code used to look like years ago.

I took a look to see what changes would need to happen in order to have
zeroFillPixelData() call m_bitmap.setAlphaType().
It looks like maybe there is a special case in the bmp decoder:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

If I understand correctly:
1. BMPs are sometimes incorrectly encoded to have an alpha channel that is
completely filled with 0s. In this case, we ignore the alpha channel.
2. But maybe the BMP was encoded correctly -- maybe we will encounter a non-zero
alpha value.
3. BUT maybe the alpha channel is filled with 255 and no pixel has any alpha at
all.

scroggo@ has told me about #3, where we detect if the image was encoded poorly
and it actually has no alpha. This can give us a speedup. However, it speeds up
poorly encoded images at the cost of detecting, which punishes all images
(including correctly encoded ones).

Since it will appear correct in either case, I think I favor not making a
special case for poorly encoded images. That will also simplify the code and
allow me to call m_bitmap.setAlphaType() inside zeroFillPixelData().

[Peter Kasting, 2017-03-14 23:04:23]

On 2017/03/14 20:49:45, cblume wrote:
> I took a look to see what changes would need to happen in order to have
> zeroFillPixelData() call m_bitmap.setAlphaType().
> It looks like maybe there is a special case in the bmp decoder:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> 
> If I understand correctly:
> 1. BMPs are sometimes incorrectly encoded to have an alpha channel that is
> completely filled with 0s. In this case, we ignore the alpha channel.
> 2. But maybe the BMP was encoded correctly -- maybe we will encounter a
non-zero
> alpha value.
> 3. BUT maybe the alpha channel is filled with 255 and no pixel has any alpha
at
> all.
> 
> scroggo@ has told me about #3, where we detect if the image was encoded poorly
> and it actually has no alpha. This can give us a speedup. However, it speeds
up
> poorly encoded images at the cost of detecting, which punishes all images
> (including correctly encoded ones).
> 
> Since it will appear correct in either case, I think I favor not making a
> special case for poorly encoded images. That will also simplify the code and
> allow me to call m_bitmap.setAlphaType() inside zeroFillPixelData().

I'm a bit confused.  I don't think your characterizations of "poorly encoded" or
"correctly encoded" are accurate, but rather than debate those, I'd rather
understand how you would perceive this code to need a special case to begin
with.  Assuming we wanted to not change the functionality of this code today,
why would it be a blocker to calling setAlphaType() from zeroFillPixelData()? 
Calling that seems correct for the code as written.

[cblume, 2017-03-14 23:53:53]

On 2017/03/14 23:04:23, Peter Kasting wrote:
> On 2017/03/14 20:49:45, cblume wrote:
> > I took a look to see what changes would need to happen in order to have
> > zeroFillPixelData() call m_bitmap.setAlphaType().
> > It looks like maybe there is a special case in the bmp decoder:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> > 
> > If I understand correctly:
> > 1. BMPs are sometimes incorrectly encoded to have an alpha channel that is
> > completely filled with 0s. In this case, we ignore the alpha channel.
> > 2. But maybe the BMP was encoded correctly -- maybe we will encounter a
> non-zero
> > alpha value.
> > 3. BUT maybe the alpha channel is filled with 255 and no pixel has any alpha
> at
> > all.
> > 
> > scroggo@ has told me about #3, where we detect if the image was encoded
poorly
> > and it actually has no alpha. This can give us a speedup. However, it speeds
> up
> > poorly encoded images at the cost of detecting, which punishes all images
> > (including correctly encoded ones).
> > 
> > Since it will appear correct in either case, I think I favor not making a
> > special case for poorly encoded images. That will also simplify the code and
> > allow me to call m_bitmap.setAlphaType() inside zeroFillPixelData().
> 
> I'm a bit confused.  I don't think your characterizations of "poorly encoded"
or
> "correctly encoded" are accurate, but rather than debate those, I'd rather
> understand how you would perceive this code to need a special case to begin
> with.  Assuming we wanted to not change the functionality of this code today,
> why would it be a blocker to calling setAlphaType() from zeroFillPixelData()? 
> Calling that seems correct for the code as written.

When I say poorly encoded, I mean when an alpha channel is provided but not used
(and thus, wasteful).
For example, a gif that says it has a transparent palette entry but no pixel
uses that palette entry. Or a bmp with an alpha channel that is filled with 255.

I suppose I shouldn't contrast it to "correctly encoded" since both are correct
here. :)
They'll display the same image. Just one is more wasteful.



What I wanted to point out is the performance could change.
Functionally, they should continue to display the same.

Right now, we try to detect these not-actually-using-alpha images. Detecting it
could slow us down.
The reason we detect is because when the image has an alpha but doesn't use it,
we can treat the image as not having alpha.
The comment above the link I gave says "opaque images are faster to draw".

If we want to continue to treat not-actually-using-alpha images as opaque
(despite the image telling us it has alpha), we cannot call setAlphaType()
inside zeroFillPixelData().

Alternatively, if we trust the image when it says it has alpha, we can call
setAlphaType() inside zeroFillPixelData() and get rid of that extra code which
detects false alpha.

Perhaps all of this should be in a separate CL.

[Peter Kasting, 2017-03-14 23:58:40]

On 2017/03/14 23:53:53, cblume wrote:
> If we want to continue to treat not-actually-using-alpha images as opaque
> (despite the image telling us it has alpha), we cannot call setAlphaType()
> inside zeroFillPixelData().

This is the statement I think is untrue.  The call to zeroFillPixelData() you
highlighted is a call to explicitly say the image _does_ have alpha, and we're
going to take all the bits we previously decoded and mark them as
fully-transparent, because it turns out we shouldn't have ignored their
zero-valued alpha channel after all.

When decoding BMPs with an alpha channel value of zero, we will never wind up
with zero-alpha pixels in the ImageFrame except when we actually want them to be
treated as transparent.  The case where we "treat zero alpha as opaque" is
handled by calling setHasAlpha(false) after creating the frame, then throwing
away the alpha value (of zero) while decoding, before writing into the
framebuffer.  If we've done that and then see a non-zero-alpha pixel, we reach
the code you highlighted, at which point we have to throw away our previous
decoding and treat it as fully-transparent.

[cblume, 2017-03-15 00:23:11]

On 2017/03/14 23:58:40, Peter Kasting wrote:
> On 2017/03/14 23:53:53, cblume wrote:
> > If we want to continue to treat not-actually-using-alpha images as opaque
> > (despite the image telling us it has alpha), we cannot call setAlphaType()
> > inside zeroFillPixelData().
> 
> This is the statement I think is untrue.  The call to zeroFillPixelData() you
> highlighted is a call to explicitly say the image _does_ have alpha, and we're
> going to take all the bits we previously decoded and mark them as
> fully-transparent, because it turns out we shouldn't have ignored their
> zero-valued alpha channel after all.
> 
> When decoding BMPs with an alpha channel value of zero, we will never wind up
> with zero-alpha pixels in the ImageFrame except when we actually want them to
be
> treated as transparent.  The case where we "treat zero alpha as opaque" is
> handled by calling setHasAlpha(false) after creating the frame, then throwing
> away the alpha value (of zero) while decoding, before writing into the
> framebuffer.  If we've done that and then see a non-zero-alpha pixel, we reach
> the code you highlighted, at which point we have to throw away our previous
> decoding and treat it as fully-transparent.

Right. I agree with what you are saying.
I believe you and I are talking about different cases.

Earlier, I described case #1 where the BMP has only 0s in the alpha. And there
was case #2 where it turns out there were values other than 0.
I believe that is what you are talking about.
And I agree that in that case, we want to set the alpha to true.


You'll notice in the code, it doesn't call m_buffer->setHasAlpha(true); at the
same place it calls m_buffer->zeroFillPixelData(). Instead, it sets alpha in the
else branch.
This is because of case #3 -- where all alpha values were 255.
This is the case I'm talking about.


Right now, the code will not enable alpha if the alpha channel was fully opaque.
The current code detects if the alpha was all 255 and then doesn't call
m_buffer->setHasAlpha(true);.
That is what I want to change.

[Peter Kasting, 2017-03-15 06:56:24]

On 2017/03/15 00:23:11, cblume wrote:
> On 2017/03/14 23:58:40, Peter Kasting wrote:
> > On 2017/03/14 23:53:53, cblume wrote:
> > > If we want to continue to treat not-actually-using-alpha images as opaque
> > > (despite the image telling us it has alpha), we cannot call setAlphaType()
> > > inside zeroFillPixelData().
> > 
> > This is the statement I think is untrue.  The call to zeroFillPixelData()
you
> > highlighted is a call to explicitly say the image _does_ have alpha, and
we're
> > going to take all the bits we previously decoded and mark them as
> > fully-transparent, because it turns out we shouldn't have ignored their
> > zero-valued alpha channel after all.
> > 
> > When decoding BMPs with an alpha channel value of zero, we will never wind
up
> > with zero-alpha pixels in the ImageFrame except when we actually want them
to
> be
> > treated as transparent.  The case where we "treat zero alpha as opaque" is
> > handled by calling setHasAlpha(false) after creating the frame, then
throwing
> > away the alpha value (of zero) while decoding, before writing into the
> > framebuffer.  If we've done that and then see a non-zero-alpha pixel, we
reach
> > the code you highlighted, at which point we have to throw away our previous
> > decoding and treat it as fully-transparent.
> 
> Right. I agree with what you are saying.
> I believe you and I are talking about different cases.
> 
> Earlier, I described case #1 where the BMP has only 0s in the alpha. And there
> was case #2 where it turns out there were values other than 0.
> I believe that is what you are talking about.
> And I agree that in that case, we want to set the alpha to true.
> 
> 
> You'll notice in the code, it doesn't call m_buffer->setHasAlpha(true); at the
> same place it calls m_buffer->zeroFillPixelData(). Instead, it sets alpha in
the
> else branch.
> This is because of case #3 -- where all alpha values were 255.
> This is the case I'm talking about.
> 
> 
> Right now, the code will not enable alpha if the alpha channel was fully
opaque.
> The current code detects if the alpha was all 255 and then doesn't call
> m_buffer->setHasAlpha(true);.
> That is what I want to change.

Are you trying to say you want to effectively add an explicit setHasAlpha(true)
call right after the zeroFillPixelData() call, in the first branch of the
conditional?  The code already assumed that was what was happening, and was not
written with the intent of omitting it for performance reasons.  I wrote this
code originally, and when I wrote it, this idea of the underlying Skia type
having its own alpha tracking didn't exist.  It's not an optimization that we're
omitting it.

Or are you trying to say you want to remove the setHasAlpha(false) call in
decodeBMP()?  That would be orthogonal to touching the code here or changing the
behavior of zeroFillPixelData().  I don't understand why that would be useful or
desirable for you.  This is more what it sounds like you're saying when you
refer to the case where all previous alpha values were 255.  That case would not
be deoptimized by making zeroFillPixelData() set the underlying alpha correctly,
because decodeBMP() is going to set it right back to false again on return,
unless you remove that call.

If you feel like we're still talking past each other, let's hop on VC tomorrow
and get this squared away.

[cblume, 2017-03-15 07:08:36]

On 2017/03/15 06:56:24, Peter Kasting wrote:
> On 2017/03/15 00:23:11, cblume wrote:
> > On 2017/03/14 23:58:40, Peter Kasting wrote:
> > > On 2017/03/14 23:53:53, cblume wrote:
> > > > If we want to continue to treat not-actually-using-alpha images as
opaque
> > > > (despite the image telling us it has alpha), we cannot call
setAlphaType()
> > > > inside zeroFillPixelData().
> > > 
> > > This is the statement I think is untrue.  The call to zeroFillPixelData()
> you
> > > highlighted is a call to explicitly say the image _does_ have alpha, and
> we're
> > > going to take all the bits we previously decoded and mark them as
> > > fully-transparent, because it turns out we shouldn't have ignored their
> > > zero-valued alpha channel after all.
> > > 
> > > When decoding BMPs with an alpha channel value of zero, we will never wind
> up
> > > with zero-alpha pixels in the ImageFrame except when we actually want them
> to
> > be
> > > treated as transparent.  The case where we "treat zero alpha as opaque" is
> > > handled by calling setHasAlpha(false) after creating the frame, then
> throwing
> > > away the alpha value (of zero) while decoding, before writing into the
> > > framebuffer.  If we've done that and then see a non-zero-alpha pixel, we
> reach
> > > the code you highlighted, at which point we have to throw away our
previous
> > > decoding and treat it as fully-transparent.
> > 
> > Right. I agree with what you are saying.
> > I believe you and I are talking about different cases.
> > 
> > Earlier, I described case #1 where the BMP has only 0s in the alpha. And
there
> > was case #2 where it turns out there were values other than 0.
> > I believe that is what you are talking about.
> > And I agree that in that case, we want to set the alpha to true.
> > 
> > 
> > You'll notice in the code, it doesn't call m_buffer->setHasAlpha(true); at
the
> > same place it calls m_buffer->zeroFillPixelData(). Instead, it sets alpha in
> the
> > else branch.
> > This is because of case #3 -- where all alpha values were 255.
> > This is the case I'm talking about.
> > 
> > 
> > Right now, the code will not enable alpha if the alpha channel was fully
> opaque.
> > The current code detects if the alpha was all 255 and then doesn't call
> > m_buffer->setHasAlpha(true);.
> > That is what I want to change.
> 
> Are you trying to say you want to effectively add an explicit
setHasAlpha(true)
> call right after the zeroFillPixelData() call, in the first branch of the
> conditional?  The code already assumed that was what was happening, and was
not
> written with the intent of omitting it for performance reasons.  I wrote this
> code originally, and when I wrote it, this idea of the underlying Skia type
> having its own alpha tracking didn't exist.  It's not an optimization that
we're
> omitting it.
> 
> Or are you trying to say you want to remove the setHasAlpha(false) call in
> decodeBMP()?  That would be orthogonal to touching the code here or changing
the
> behavior of zeroFillPixelData().  I don't understand why that would be useful
or
> desirable for you.  This is more what it sounds like you're saying when you
> refer to the case where all previous alpha values were 255.  That case would
not
> be deoptimized by making zeroFillPixelData() set the underlying alpha
correctly,
> because decodeBMP() is going to set it right back to false again on return,
> unless you remove that call.
> 
> If you feel like we're still talking past each other, let's hop on VC tomorrow
> and get this squared away.

VC might be a good idea. :)

I'm not the one saying it.
The comment here is:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
"As an optimization, avoid setting "hasAlpha" to true for images where all alpha
values are 255; opaque images are faster to draw."

That is why the code has:
} else if (alpha != 255)
  m_buffer->setHasAlpha(true);
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

It isn't relying on an implicit call to m_buffer->setHasAlpha(true);
Instead, it only calls it if one of the values isn't 255.

Said another way, if all of the pixels have the alpha value 255 then the image
is fully opaque -- it is not transparent. So it never calls
m_buffer->setHasAlpha(true); because effectively it *doesn't* have alpha.

[Peter Kasting, 2017-03-15 10:09:12]

On 2017/03/15 07:08:36, cblume wrote:
> On 2017/03/15 06:56:24, Peter Kasting wrote:
> > On 2017/03/15 00:23:11, cblume wrote:
> > > On 2017/03/14 23:58:40, Peter Kasting wrote:
> > > > On 2017/03/14 23:53:53, cblume wrote:
> > > > > If we want to continue to treat not-actually-using-alpha images as
> opaque
> > > > > (despite the image telling us it has alpha), we cannot call
> setAlphaType()
> > > > > inside zeroFillPixelData().
> > > > 
> > > > This is the statement I think is untrue.  The call to
zeroFillPixelData()
> > you
> > > > highlighted is a call to explicitly say the image _does_ have alpha, and
> > we're
> > > > going to take all the bits we previously decoded and mark them as
> > > > fully-transparent, because it turns out we shouldn't have ignored their
> > > > zero-valued alpha channel after all.
> > > > 
> > > > When decoding BMPs with an alpha channel value of zero, we will never
wind
> > up
> > > > with zero-alpha pixels in the ImageFrame except when we actually want
them
> > to
> > > be
> > > > treated as transparent.  The case where we "treat zero alpha as opaque"
is
> > > > handled by calling setHasAlpha(false) after creating the frame, then
> > throwing
> > > > away the alpha value (of zero) while decoding, before writing into the
> > > > framebuffer.  If we've done that and then see a non-zero-alpha pixel, we
> > reach
> > > > the code you highlighted, at which point we have to throw away our
> previous
> > > > decoding and treat it as fully-transparent.
> > > 
> > > Right. I agree with what you are saying.
> > > I believe you and I are talking about different cases.
> > > 
> > > Earlier, I described case #1 where the BMP has only 0s in the alpha. And
> there
> > > was case #2 where it turns out there were values other than 0.
> > > I believe that is what you are talking about.
> > > And I agree that in that case, we want to set the alpha to true.
> > > 
> > > 
> > > You'll notice in the code, it doesn't call m_buffer->setHasAlpha(true); at
> the
> > > same place it calls m_buffer->zeroFillPixelData(). Instead, it sets alpha
in
> > the
> > > else branch.
> > > This is because of case #3 -- where all alpha values were 255.
> > > This is the case I'm talking about.
> > > 
> > > 
> > > Right now, the code will not enable alpha if the alpha channel was fully
> > opaque.
> > > The current code detects if the alpha was all 255 and then doesn't call
> > > m_buffer->setHasAlpha(true);.
> > > That is what I want to change.
> > 
> > Are you trying to say you want to effectively add an explicit
> setHasAlpha(true)
> > call right after the zeroFillPixelData() call, in the first branch of the
> > conditional?  The code already assumed that was what was happening, and was
> not
> > written with the intent of omitting it for performance reasons.  I wrote
this
> > code originally, and when I wrote it, this idea of the underlying Skia type
> > having its own alpha tracking didn't exist.  It's not an optimization that
> we're
> > omitting it.
> > 
> > Or are you trying to say you want to remove the setHasAlpha(false) call in
> > decodeBMP()?  That would be orthogonal to touching the code here or changing
> the
> > behavior of zeroFillPixelData().  I don't understand why that would be
useful
> or
> > desirable for you.  This is more what it sounds like you're saying when you
> > refer to the case where all previous alpha values were 255.  That case would
> not
> > be deoptimized by making zeroFillPixelData() set the underlying alpha
> correctly,
> > because decodeBMP() is going to set it right back to false again on return,
> > unless you remove that call.
> > 
> > If you feel like we're still talking past each other, let's hop on VC
tomorrow
> > and get this squared away.
> 
> VC might be a good idea. :)
> 
> I'm not the one saying it.
> The comment here is:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> "As an optimization, avoid setting "hasAlpha" to true for images where all
alpha
> values are 255; opaque images are faster to draw."
> 
> That is why the code has:
> } else if (alpha != 255)
>   m_buffer->setHasAlpha(true);
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> 
> It isn't relying on an implicit call to m_buffer->setHasAlpha(true);
> Instead, it only calls it if one of the values isn't 255.
> 
> Said another way, if all of the pixels have the alpha value 255 then the image
> is fully opaque -- it is not transparent. So it never calls
> m_buffer->setHasAlpha(true); because effectively it *doesn't* have alpha.

Correct, and none of that is relevant to making zeroFillPixelData() call
setHasAlpha(true) -- the only zeroFillPixelData() calls in the code are one
that's already followed by an explicit setHasAlpha(false), and one that assumes
the zeroFillPixelData() is already doing the equivalent of setHasAlpha(true).

One reason it's hard for me to understand what you're proposing is that you've
never given an actual diff of the change you're saying you want to make.  That's
why I asked which of two different changes you were saying you wanted to make --
which you still didn't answer above.

Give me a diff rather than prose, and I'll comment in more detail :)

[Noel Gordon, 2017-03-15 13:42:51]

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:124: bool
allocateBackingStore(int newWidth, int newHeight, sk_sp<SkColorSpace>);
Better, and re-reading the comment here for this API suggests another possible
name: allocatePixelData(). Maybe if  

 - we allocateBackingStore() we should zeroFillBackingStore()
 - we allocatePixelData() we should zeroFillPixelData()

since their use is usually adjacent in the code with this CL.  The reader of
that code should maybe not have to think about: "What's the diff b/w a
BackingStore and a PixelData?" :)  Thoughts?

Comment "Must only be called once." is removed compared to diff-left.  Should
this API really be promoting multiple calls?  The DCHECK in its body seems to
say otherwise: DCHECK(!width() && !height()) [1].

[1] width() and height() here being the SkBitmap width() as height() as Leon
pointed out (to me).

[cblume, 2017-03-15 18:22:35]

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

[commit-bot: I haz the power, 2017-03-15 18:23:20]

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

[commit-bot: I haz the power, 2017-03-15 18:26:22]

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

[commit-bot: I haz the power, 2017-03-15 18:26:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[cblume, 2017-03-15 19:17:47]

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

[commit-bot: I haz the power, 2017-03-15 19:18:26]

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

[commit-bot: I haz the power, 2017-03-15 20:42:13]

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

[commit-bot: I haz the power, 2017-03-15 20:42:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cblume, 2017-03-16 02:29:25]

I've made a separate issue to handle opaque images with an unused alpha channel:
https://codereview.chromium.org/2756463003

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:124: bool
allocateBackingStore(int newWidth, int newHeight, sk_sp<SkColorSpace>);
On 2017/03/15 13:42:51, noel gordon wrote:
> Better, and re-reading the comment here for this API suggests another possible
> name: allocatePixelData(). Maybe if  
> 
>  - we allocateBackingStore() we should zeroFillBackingStore()
>  - we allocatePixelData() we should zeroFillPixelData()
> 
> since their use is usually adjacent in the code with this CL.  The reader of
> that code should maybe not have to think about: "What's the diff b/w a
> BackingStore and a PixelData?" :)  Thoughts?
> 
> Comment "Must only be called once." is removed compared to diff-left.  Should
> this API really be promoting multiple calls?  The DCHECK in its body seems to
> say otherwise: DCHECK(!width() && !height()) [1].
> 
> [1] width() and height() here being the SkBitmap width() as height() as Leon
> pointed out (to me).

I think allocatePixelData() is a much better name. I'll go with that. Good call.


Also, you're right about the DCHECK. I removed the comment because it was not
correct -- it would not leak memory if called multiple times.

We still shouldn't call it multiple times per frame because that would just
deallocate and allocate again. However, I think the DCHECK is now a bit
paranoid. Elsewhere in the codebase we don't have DCHECKs to prevent this sort
of reallocation. So even though protection is good, I think this might be a bit
heavy on the defensive programming. I think we should perhaps get rid of the
DCHECKs now.

[cblume, 2017-03-16 10:22:28]

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

[commit-bot: I haz the power, 2017-03-16 10:22:45]

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-16 10:23:23]

I think this might now satisfy everybody's comments.
If I missed anything let me know.

[Noel Gordon, 2017-03-16 11:32:07]

On 2017/03/16 02:29:25, cblume wrote:
> I've made a separate issue to handle opaque images with an unused alpha
channel:
> https://codereview.chromium.org/2756463003

Yes, it is indeed a separate question in my mind, so ok to deal with it
elsewhere.  The current change does not change existing behavior, IIUC.  Is that
correct?

If so, then your change description (which now needs updating btw), should say
"No change in behavior, no new tests".

This because the Blink (formerly WebKit) rule is "all patches should come with a
test or be covered by exiting tests, and all patches should have a bug".  Since
that rule is assumed / enforced by Blink reviewers, then if a patch does not
change behavior, then your patch is in fact covered by existing tests.

IN this case, if your patch did accidentally change alpha handling behavior,
then the _partial_ load tests in

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType...

for bmp, png, jpeg, and webp would point out the problem.

We have a bug on file for adding a partial load test for animated APNG image now
that the APNG patch has landed: crbug.com/700892

We should add a partial load test for an animated GIF, filed crbug.com/702148
about that.

Animated WEBP does not currently support partial rendering.

[Noel Gordon, 2017-03-16 11:34:04]

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:124: bool
allocateBackingStore(int newWidth, int newHeight, sk_sp<SkColorSpace>);
On 2017/03/16 02:29:25, cblume wrote:
> On 2017/03/15 13:42:51, noel gordon wrote:
> > Better, and re-reading the comment here for this API suggests another
possible
> > name: allocatePixelData(). Maybe if  
> > 
> >  - we allocateBackingStore() we should zeroFillBackingStore()
> >  - we allocatePixelData() we should zeroFillPixelData()
> > 
> > since their use is usually adjacent in the code with this CL.  The reader of
> > that code should maybe not have to think about: "What's the diff b/w a
> > BackingStore and a PixelData?" :)  Thoughts?
> > 
> > Comment "Must only be called once." is removed compared to diff-left. 
Should
> > this API really be promoting multiple calls?  The DCHECK in its body seems
to
> > say otherwise: DCHECK(!width() && !height()) [1].
> > 
> > [1] width() and height() here being the SkBitmap width() as height() as Leon
> > pointed out (to me).
> 
> I think allocatePixelData() is a much better name. I'll go with that. Good
call.

I'm good with this part of the change: allocatePixelData() seems fine and fits
well with ZeroFillPixelData(), however, ...

> Also, you're right about the DCHECK. I removed the comment because it was not
> correct -- it would not leak memory if called multiple times.

The comment was to be fixed, not removed.  It was wrong about the leaking memory
part and that should go for sure ...
 
> We still shouldn't call it multiple times per frame because that would just
> deallocate and allocate again. 

So "deallocate and allocate again" is not what we want, so let's document that.
I think the comment from diff-left should be retained in some form, something
like 

// Allocates space for the pixel data. Must be called before any pixels are
// written, and should be only be called once. The specified color space may
// be null if and only if color correct rendering is enabled. Returns true
// if the allocation succeeded.

> However, I think the DCHECK is now a bit
> paranoid. Elsewhere in the codebase we don't have DCHECKs to prevent this sort
> of reallocation. 

What other areas of the codebase do is interesting, but the DCHECK in the
ImageFrame .cpp file regarding the width() and height() of the m_bitmap should
be retained to document the code contract.

That is not paranoid imho, it simply states what the .cpp code expects (don't
call me twice, which we seem to agree on as the goal, since any re-allocation
would suggest misuse of this API to me).

> So even though protection is good, I think this might be a bit
> heavy on the defensive programming. I think we should perhaps get rid of the
> DCHECKs now.

Don't agree, see above.

[commit-bot: I haz the power, 2017-03-16 11:42:01]

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

[commit-bot: I haz the power, 2017-03-16 11:42:02]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-03-16 12:01:50]

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
setHasAlpha(true);
Harmless, but also redundant.  It sets the m_bitmap alpha type to 

m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType

aka the bitmap backing store has alpha.  But allocatePixelData already did that
@line 111 below.  No real need to do it twice.

[Noel Gordon, 2017-03-16 23:54:59]

On 2017/03/16 11:32:07, noel gordon wrote:
> On 2017/03/16 02:29:25, cblume wrote:
> > I've made a separate issue to handle opaque images with an unused alpha
> channel:
> > https://codereview.chromium.org/2756463003
> 
> Yes, it is indeed a separate question in my mind, so ok to deal with it
> elsewhere.  The current change does not change existing behavior, IIUC.  Is
that
> correct?
> 
> If so, then your change description (which now needs updating btw), should say
> "No change in behavior, no new tests".
> 
> This because the Blink (formerly WebKit) rule is "all patches should come with
a
> test or be covered by exiting tests, and all patches should have a bug". 
Since
> that rule is assumed / enforced by Blink reviewers, then if a patch does not
> change behavior, then your patch is in fact covered by existing tests.
> 
> IN this case, if your patch did accidentally change alpha handling behavior,
> then the _partial_ load tests in
> 
>
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType...
> 
> for bmp, png, jpeg, and webp would point out the problem.
> 
> We have a bug on file for adding a partial load test for animated APNG image
now
> that the APNG patch has landed: crbug.com/700892
> 
> We should add a partial load test for an animated GIF, filed crbug.com/702148
> about that.

Tests landed for animated APNG and animated GIF on these bugs.

[cblume, 2017-03-17 05:34:22]

On 2017/03/16 23:54:59, noel gordon wrote:
> The current change does not change existing behavior, IIUC.  Is
> that correct?

Correct.
It was going to change an unused code path: If we called zeroFillPixelData()
before copyBitmapData() and it was set to unpremultiply, it would have not
copied.
However, copyBitmapData() is always called before zeroFillPixelData().
But now that we don't call setHasAlpha() inside zeroFillPixelData(), we don't
need to worry about that unused path.
So there is no change in behavior.

> If so, then your change description (which now needs updating btw), should say
> "No change in behavior, no new tests".

Will do.

> Tests landed for animated APNG and animated GIF on these bugs.

I'll rerun the bots to make sure it passes the tests. :)

[cblume, 2017-03-17 05:35:14]

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/2749703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:124: bool
allocateBackingStore(int newWidth, int newHeight, sk_sp<SkColorSpace>);
On 2017/03/16 11:34:04, noel gordon wrote:
> something like 
> 
> // Allocates space for the pixel data. Must be called before any pixels are
> // written, and should be only be called once. The specified color space may
> // be null if and only if color correct rendering is enabled. Returns true
> // if the allocation succeeded.

Done.

> the DCHECK in the
> ImageFrame .cpp file regarding the width() and height() of the m_bitmap should
> be retained to document the code contract.

Done.

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
setHasAlpha(true);
On 2017/03/16 12:01:50, noel gordon wrote:
> Harmless, but also redundant.  It sets the m_bitmap alpha type to 
> 
> m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType
> 
> aka the bitmap backing store has alpha.  But allocatePixelData already did
that
> @line 111 below.  No real need to do it twice.
> 
> 
> 

Wait....you're right.
I could have sworn it made perfect sense just yesterday.

All the existing call sites call allocatePixelData() before calling
zeroFillPixelData().

The other call site I care about which isn't shown here is the reason I created
this CR: So I can call setSizeAndColorSpace() without zero-filling. In that
case, I am not calling zeroFillPixelData().


I don't think there is a case where we need to do anything with the alpha inside
zeroFillPixelData(). I think we could have just removed the m_hasAlpha = true;
line. And I think we saw it and thought "Probably should be calling the
function."

I'll go ahead and remove it.

[cblume, 2017-03-17 05:35:35]

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

[commit-bot: I haz the power, 2017-03-17 05:36:00]

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

[Peter Kasting, 2017-03-17 06:35:50]

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
setHasAlpha(true);
On 2017/03/17 05:35:14, cblume wrote:
> On 2017/03/16 12:01:50, noel gordon wrote:
> > Harmless, but also redundant.  It sets the m_bitmap alpha type to 
> > 
> > m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType
> > 
> > aka the bitmap backing store has alpha.  But allocatePixelData already did
> that
> > @line 111 below.  No real need to do it twice.
> > 
> > 
> > 
> 
> Wait....you're right.
> I could have sworn it made perfect sense just yesterday.

Because the BMP decoder needs it, no?

It allocates and zeros the pixel data and then explicitly calls
setHasAlpha(false).

Later when it calls zeroFillPixelData() again it expects that to mark the image
as having alpha.

I don't think this can be removed, because zeroFillPixelData() is not called
only at image creation time.

[commit-bot: I haz the power, 2017-03-17 08:08:19]

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

[commit-bot: I haz the power, 2017-03-17 08:08:20]

Dry run: This issue passed the CQ dry run.

[scroggo_chromium, 2017-03-17 14:36:38]

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:946:
// The buffer is transparent outside the decoded area while the image is
This comment seems to apply to the line that you have removed down below. Remove
it?

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:464:
// The buffer is transparent outside the decoded area while the image is
Similarly, this comment seems to apply to the line you've removed.

[cblume, 2017-03-17 17:52:55]

Description was changed from

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

R=scroggo@chromium.org
BUG=701143
==========

to

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

No change in behavior, no new tests.

R=scroggo@chromium.org
BUG=701143
==========

[cblume, 2017-03-17 18:05:30]

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

[cblume, 2017-03-17 18:05:46]

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
setHasAlpha(true);
On 2017/03/17 06:35:50, Peter Kasting wrote:
> On 2017/03/17 05:35:14, cblume wrote:
> > On 2017/03/16 12:01:50, noel gordon wrote:
> > > Harmless, but also redundant.  It sets the m_bitmap alpha type to 
> > > 
> > > m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType
> > > 
> > > aka the bitmap backing store has alpha.  But allocatePixelData already did
> > that
> > > @line 111 below.  No real need to do it twice.
> > > 
> > > 
> > > 
> > 
> > Wait....you're right.
> > I could have sworn it made perfect sense just yesterday.
> 
> Because the BMP decoder needs it, no?
> 
> It allocates and zeros the pixel data and then explicitly calls
> setHasAlpha(false).
> 
> Later when it calls zeroFillPixelData() again it expects that to mark the
image
> as having alpha.
> 
> I don't think this can be removed, because zeroFillPixelData() is not called
> only at image creation time.

It is now being explicitly set in BMPImageReader.cpp:827 with

m_buffer->zeroFillPixelData();
m_buffer->setHasAlpha(true);

since this was the only location that needed both and the concepts are separate.

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:946:
// The buffer is transparent outside the decoded area while the image is
On 2017/03/17 14:36:38, scroggo_chromium wrote:
> This comment seems to apply to the line that you have removed down below.
Remove
> it?

Done.

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:464:
// The buffer is transparent outside the decoded area while the image is
On 2017/03/17 14:36:38, scroggo_chromium wrote:
> Similarly, this comment seems to apply to the line you've removed.

Done.

[commit-bot: I haz the power, 2017-03-17 18:06:10]

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

[commit-bot: I haz the power, 2017-03-17 19:50:42]

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

[commit-bot: I haz the power, 2017-03-17 19:50:43]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-03-17 21:18:40]

(For clarity, still LGTM)

https://codereview.chromium.org/2749703002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:108: //
allocatePixelData() should only be called once
Nit: Trailing period

[cblume, 2017-03-17 21:33:04]

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

[commit-bot: I haz the power, 2017-03-17 21:33:57]

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-17 21:41:16]

I need f(malita)'s approval for
	third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h

But I'll also wait for approvals from the others who have commented.

https://codereview.chromium.org/2749703002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:108: //
allocatePixelData() should only be called once
On 2017/03/17 21:18:40, Peter Kasting wrote:
> Nit: Trailing period

Done.

[commit-bot: I haz the power, 2017-03-17 23:36:47]

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

[commit-bot: I haz the power, 2017-03-17 23:36:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Noel Gordon, 2017-03-20 13:24:52]

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
setHasAlpha(true);
On 2017/03/17 18:05:45, cblume wrote:
> On 2017/03/17 06:35:50, Peter Kasting wrote:
> > On 2017/03/17 05:35:14, cblume wrote:
> > > On 2017/03/16 12:01:50, noel gordon wrote:
> > > > Harmless, but also redundant.  It sets the m_bitmap alpha type to 
> > > > 
> > > > m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType
> > > > 
> > > > aka the bitmap backing store has alpha.  But allocatePixelData already
did
> > > that
> > > > @line 111 below.  No real need to do it twice.
> > > > 
> > > > 
> > > > 
> > > 
> > > Wait....you're right.
> > > I could have sworn it made perfect sense just yesterday.
> > 
> > Because the BMP decoder needs it, no?
> > 
> > It allocates and zeros the pixel data and then explicitly calls
> > setHasAlpha(false).
> > 
> > Later when it calls zeroFillPixelData() again it expects that to mark the
> image
> > as having alpha.
> > 
> > I don't think this can be removed, because zeroFillPixelData() is not called
> > only at image creation time.
> 
> It is now being explicitly set in BMPImageReader.cpp:827 with
> 
> m_buffer->zeroFillPixelData();
> m_buffer->setHasAlpha(true);
> 
> since this was the only location that needed both and the concepts are
separate.

Not sure they're separate concepts.  Somewhat odd to me to zero the data and
then not set m_alpha = true.  (And then Peter spotted the BMP bug that would
have resulted).  I think the body should read:

m_bitmap.eraseARGB(0, 0, 0, 0);
m_hasAlpha = true;

as it was in diff-left, since your goal here was to just split the calling of
this function out, without change behavior :)

That way you wouldn't need to touch the BMP decoder, and the comments therein
(near line@127) seem to describe what is expected pretty well?

[cblume, 2017-03-20 19:42:29]

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

[commit-bot: I haz the power, 2017-03-20 19:42:55]

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:30:14]

On 2017/03/20 13:24:52, noel gordon wrote:
>
https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):
> 
>
https://codereview.chromium.org/2749703002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81:
> setHasAlpha(true);
> On 2017/03/17 18:05:45, cblume wrote:
> > On 2017/03/17 06:35:50, Peter Kasting wrote:
> > > On 2017/03/17 05:35:14, cblume wrote:
> > > > On 2017/03/16 12:01:50, noel gordon wrote:
> > > > > Harmless, but also redundant.  It sets the m_bitmap alpha type to 
> > > > > 
> > > > > m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType
> > > > > 
> > > > > aka the bitmap backing store has alpha.  But allocatePixelData already
> did
> > > > that
> > > > > @line 111 below.  No real need to do it twice.
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > Wait....you're right.
> > > > I could have sworn it made perfect sense just yesterday.
> > > 
> > > Because the BMP decoder needs it, no?
> > > 
> > > It allocates and zeros the pixel data and then explicitly calls
> > > setHasAlpha(false).
> > > 
> > > Later when it calls zeroFillPixelData() again it expects that to mark the
> > image
> > > as having alpha.
> > > 
> > > I don't think this can be removed, because zeroFillPixelData() is not
called
> > > only at image creation time.
> > 
> > It is now being explicitly set in BMPImageReader.cpp:827 with
> > 
> > m_buffer->zeroFillPixelData();
> > m_buffer->setHasAlpha(true);
> > 
> > since this was the only location that needed both and the concepts are
> separate.
> 
> Not sure they're separate concepts.  Somewhat odd to me to zero the data and
> then not set m_alpha = true.  (And then Peter spotted the BMP bug that would
> have resulted).  I think the body should read:
> 
> m_bitmap.eraseARGB(0, 0, 0, 0);
> m_hasAlpha = true;
> 
> as it was in diff-left, since your goal here was to just split the calling of
> this function out, without change behavior :)
> 
> That way you wouldn't need to touch the BMP decoder, and the comments therein
> (near line@127) seem to describe what is expected pretty well?

Hrmmm I think they may still be separate concepts in general (although maybe not
in these use cases here).

But you are definitely right that it should go in a separate CL.
I moved that over to https://codereview.chromium.org/2762643004

[commit-bot: I haz the power, 2017-03-20 20:37:01]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Noel Gordon, 2017-03-20 21:02:47]

LGTM

[f(malita), 2017-03-21 12:27:59]

LGTM

[cblume, 2017-03-21 16:34:33]

The CQ bit was checked by cblume@chromium.org

[cblume, 2017-03-21 16:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2749703002/#ps220001
(title: "Moving to another CR the separation of zeroing pixel data from setting
alpha")

[commit-bot: I haz the power, 2017-03-21 16:34:56]

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

[commit-bot: I haz the power, 2017-03-21 16:43:11]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490114073040730,
"parent_rev": "f4ce05617437fb9944ac3d00d88154c51fc02795", "commit_rev":
"b6b96c3de7c6db78762ce660b274514bd18a1c0b"}

[commit-bot: I haz the power, 2017-03-21 16:43:58]

Description was changed from

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

No change in behavior, no new tests.

R=scroggo@chromium.org
BUG=701143
==========

to

==========
Refactor ImageFrame::setSizeAndColorSpace()

The function ImageFrame::setSizeAndColorSpace does multiple things.
It sets the size and color space & zero fills the data.

We want to be able to not zero fill the entire image. The image decoder
can fill the entire image frame with valid pixel data. And in the case
of partial images, the decoder knows which pixels do not contain image
data, allowing them to zero fill a smaller area.

But for now, just separate the concept of setSizeAndColorSpace() and
zeroFillPixelData().

No change in behavior, no new tests.

R=scroggo@chromium.org
BUG=701143

Review-Url: https://codereview.chromium.org/2749703002
Cr-Commit-Position: refs/heads/master@{#458437}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6b96c3de7c6db78762ce660b274...
==========

[commit-bot: I haz the power, 2017-03-21 16:44:00]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/b6b96c3de7c6db78762ce660b274...