|
|
Created:
3 years, 9 months ago by cblume Modified:
3 years, 9 months ago CC:
chromium-reviews, jzern, krit, drott+blinkwatch_chromium.org, urvang, blink-reviews-platform-graphics_chromium.org, skal, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, fmalita+watch_chromium.org, Rik, Stephen Chennney, Justin Novosad, blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor 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
Patch Set 1 #Patch Set 2 : Fix alloc returned value flipped #
Total comments: 11
Patch Set 3 : Rename setSizeAndColorSpace() to allocateBackingStore() #Patch Set 4 : Forgot to update the call sites #
Total comments: 4
Patch Set 5 : Rename allocateBackingStore() to allocatePixelData() #Patch Set 6 : Rebase #Patch Set 7 : Call setHasAlpha(true); from within zeroFillPixelData() #
Total comments: 9
Patch Set 8 : Not calling setHasAlpha() inside zeroFillPixelData(). Updating comments and DCHECK #Patch Set 9 : BMP reader should be setting hasAlpha to true, not false #Patch Set 10 : Remove comments that focus on transparency, which is no longer explicitly set #
Total comments: 2
Patch Set 11 : Add trailing period to comment #Patch Set 12 : Moving to another CR the separation of zeroing pixel data from setting alpha #Dependent Patchsets: Messages
Total messages: 91 (51 generated)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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@chromium.org changed reviewers: + fmalita@chromium.org, noel@chromium.org, pkasting@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
LGTM
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).
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.
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.
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.
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.
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.
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.
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.
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.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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().
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.
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.
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.
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.
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.
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.
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 :)
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).
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think this might now satisfy everybody's comments. If I missed anything let me know.
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.
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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. :)
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.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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 ==========
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(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
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
LGTM
LGTM
The CQ bit was checked by cblume@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490114073040730, "parent_rev": "f4ce05617437fb9944ac3d00d88154c51fc02795", "commit_rev": "b6b96c3de7c6db78762ce660b274514bd18a1c0b"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/b6b96c3de7c6db78762ce660b274... |