Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2756463003:
Remove opaque alpha channel special case (Closed)
DescriptionRemove opaque alpha channel special case
The image decoders which support alpha have a special case to handle
situations where the alpha channel is unused.
This can come up from a handful of cases but the net effect is we are
detecting if the image was actually opaque, despite the image being
encoded to say that it wasn't.
Some scenarios where this could happen are:
1.) a gif frame's palette includes transparency but no pixels use that
transparent palette entry,
2.) an animation frame completely covering the previous frame & having
no transparent pixels, and
3.) an animation frame partially covering a previous frame, but the
previous frame containing no visible transparent pixels.
Scenario #1 is O(n) over the frame's pixel data.
After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory.
Raster times were slower for small images and faster for large images.
[1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz9QxpaI_GQEziQgryJc0/edit?usp=sharing
R=scroggo@chromium.org
BUG=702059
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixing first-pixel alpha case #
Total comments: 2
Patch Set 3 : Rebasing #Patch Set 4 : Updating tests to not assume opaque detection #Patch Set 5 : Rebasing #Patch Set 6 : Hopefully this rebase works #Patch Set 7 : Fix gif detecting if a frame claims to have transparency #
Total comments: 9
Patch Set 8 : Rebasing #Patch Set 9 : Code review comments #
Total comments: 10
Patch Set 10 : Correct comments #Patch Set 11 : Check that the transparent pixel index is within range #Patch Set 12 : Fix build #Patch Set 13 : Fix IsAlphaSupported returning the inverse #
Total comments: 16
Patch Set 14 : Fix mistake where variables were incorrectly pulled out of a branch #Patch Set 15 : Clarify comment for test_gif #Patch Set 16 : Correct inverted conditional. Add DCHECK for assumed condition #Patch Set 17 : Remove redundant conditional #
Total comments: 14
Patch Set 18 : Clarify comment #Patch Set 19 : Only set bmp alpha when we detect actual alpha #
Total comments: 1
Patch Set 20 : Checking if JPEGImageDecoder::OutputScanlines()'s SetHasAlpha(true) is under test #Messages
Total messages: 99 (59 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 ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The reason we detected when the image was actually opaque is because it was faster to draw opaque images. This might not be true any more with things like GPU raster. It might not even be true for CPU raster now-a-days. R=scroggo@chromium.org BUG=702059 ========== to ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The reason we detected when the image was actually opaque is because it was faster to draw opaque images. This might not be true any more with things like GPU raster. It might not even be true for CPU raster now-a-days. R=scroggo@chromium.org BUG=702059 ==========
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This CL mixes three separate changes: (1) zeroFillPixelData() should call setHasAlpha(true), and WebP/JPEG do not need to do so redundantly. This is uncontroversial. (2) A BMP change that I think is incorrect (see comments below). (3) The change covered in the description, which removes the code to mark the image as not having alpha when it doesn't have alpha. It's not clear to me why removing this is desirable. Surely it's not harmful to performance to mark the frame as fully-opaque when it is indeed fully-opaque, so at worst, this is simply (a tiny amount of) wasted effort. (And in that case, I would argue that the right fix is to remove this bit from Skia's APIs.) But at best it actually does help perf in at least some scenario. Have you tested extensively to figure this out? I don't think (3) is worth taking the time to figure out if it can possibly help. Just leave the code. So I would limit this CL to only doing (1). https://codereview.chromium.org/2756463003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (left): https://codereview.chromium.org/2756463003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:829: m_buffer->setHasAlpha(true); Removing this is not correct. It will mean any BMP that does not begin with zero-alpha pixels will be marked fully-opaque. Consider a one-pixel RGB BMP, where the one pixel has alpha 128. decodeBMP() will call zeroFillPixelData(), which will call setHasAlpha(true). It will then immediately call setHasAlpha(false). Then we'll come here. Since alpha != 0 and m_seenZeroAlphaPixel == false, we would have taken this branch and marked this image as having alpha. Now we won't. Then decoding will finish.
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...
+Matt -- If I remember correctly, we saw there is no longer a speed benefit in the opaque drawing case? I might be wrong about this. On 2017/03/16 04:37:14, Peter Kasting wrote: > This CL mixes three separate changes: > > (1) zeroFillPixelData() should call setHasAlpha(true), and WebP/JPEG do not need > to do so redundantly. This is uncontroversial. > (2) A BMP change that I think is incorrect (see comments below). This one was a mistake. Hopefully it is fixed now. > (3) The change covered in the description, which removes the code to mark the > image as not having alpha when it doesn't have alpha. It's not clear to me why > removing this is desirable. Surely it's not harmful to performance to mark the > frame as fully-opaque when it is indeed fully-opaque, so at worst, this is > simply (a tiny amount of) wasted effort. (And in that case, I would argue that > the right fix is to remove this bit from Skia's APIs.) But at best it actually > does help perf in at least some scenario. Have you tested extensively to figure > this out? > > I don't think (3) is worth taking the time to figure out if it can possibly > help. Just leave the code. So I would limit this CL to only doing (1). The reason I was combining (1) and (3) is because (1) changes the behavior. The old code did not update the SkImageInfo inside zeroFillPixelData(). To Skia, it was still opaque. It wouldn't become transparent until a non-255 value was seen. But maybe I was being too close-minded. I figured by setting it to transparent inside zeroFillPixelData(), we would suddenly be ignoring the all-255 case. We would be setting it to transparent when we would have left it opaque. But you are right, (1) and (3) are be separate concepts. I just need to call m_buffer->setHasTransparency(false); after the call to m_buffer->zeroFillPixelData(); if we want to separate these concepts. Because that makes (1) simple again, I can go ahead and add it back in the other code review. But I will leave this code review around because removing the 255 special case may be valuable. I don't have any measurements yet because I only just wrote the code. Now that I have the code, I can measure it. You described a scenario where maybe it costs little-to-nothing and if there is a case where there is benefit, it would be worth it. I agree with that. But this hinges on there still being a benefit. If there isn't a case where there is a benefit then it's added complexity and maybe a small performance loss. https://codereview.chromium.org/2756463003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (left): https://codereview.chromium.org/2756463003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:829: m_buffer->setHasAlpha(true); On 2017/03/16 04:37:14, Peter Kasting wrote: > Removing this is not correct. It will mean any BMP that does not begin with > zero-alpha pixels will be marked fully-opaque. > > Consider a one-pixel RGB BMP, where the one pixel has alpha 128. decodeBMP() > will call zeroFillPixelData(), which will call setHasAlpha(true). It will then > immediately call setHasAlpha(false). Then we'll come here. Since alpha != 0 > and m_seenZeroAlphaPixel == false, we would have taken this branch and marked > this image as having alpha. Now we won't. Then decoding will finish. You are right that the call to zeroFillPixelData() is inside if (m_seenZeroAlphaPixel);. So that means the scenario you described -- a 1x1 BMP with 128 alpha will not get m_buffer->setHasAlpha(true); called. I meant to change this to: } else m_buffer->setHasAlpha(true); which would have also covered case (3) you mentioned.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> +Matt -- If I remember correctly, we saw there is no longer a speed benefit in > the opaque drawing case? I might be wrong about this. > I investigated something similar a while ago with RGBA PNGs where all of the alphas are 255. In that case: ***There was a real decode performance win to drop the per pixel alpha checking ***Decode+draw benchmarks were dominated by decode time, draw time was essentially negligible ***These PNGs are somewhat rare ***There was a code complexity win ***It felt good not to cater to "badly encoded" PNGs I don't have real data in this case, but I would guess: ***Decode performance win may not be as significant. Since GIFs are index8, we are already forced to do per pixel table lookups. I don't think alpha checking adds too much to this. ***There is a code complexity win - I think this is even more true and important in the gif case. ***I believe gif frames with an unused transparent index are rare. And I'd guess gif frames that don't cover the whole canvas are rare. ***I don't expect draw time to be significantly different. To paraphrase, I'm definitely in favor of making this simplification. Concept looks good to me. I understand Peter's suggestion that we should have data to back-up these instincts. If it comes to that, I do think it would be worth the time.
https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: } else I'm still confused. If you really want to do this, why not mark all BMPs as having alpha all the time, i.e. rip out this conditional and rip out the setHasAlpha(false) call earlier in the file? More generally, why expose setHasAlpha() at all? If there's no benefit, then no callers should ever call it, and the API shouldn't exist. This is the same comment I had for the Skia code. I don't really want to go halfway on this one. If we believe this isn't a win, let's rip the entire API out as far down as we can. Otherwise let's leave everything.
https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: } else On 2017/03/16 19:43:27, Peter Kasting wrote: > I'm still confused. If you really want to do this, why not mark all BMPs as > having alpha all the time, i.e. rip out this conditional and rip out the > setHasAlpha(false) call earlier in the file? > > More generally, why expose setHasAlpha() at all? If there's no benefit, then no > callers should ever call it, and the API shouldn't exist. This is the same > comment I had for the Skia code. > > I don't really want to go halfway on this one. If we believe this isn't a win, > let's rip the entire API out as far down as we can. Otherwise let's leave > everything. I think those are different. An image may or may not have alpha. That is on the concept level. If the code paths for painting are the same speed, that's on the implementation level. Just because the implementation has no speed difference doesn't mean we should get rid of the concept. The image decoders had this as an optimization. And that optimization may no longer be valid. It doesn't mean the concept of alpha is bad. It means the times have changed, hardware has changed, code has changed, and this optimization has finished serving its purpose.
On 2017/03/16 20:39:45, cblume wrote: > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp > (right): > > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: } > else > On 2017/03/16 19:43:27, Peter Kasting wrote: > > I'm still confused. If you really want to do this, why not mark all BMPs as > > having alpha all the time, i.e. rip out this conditional and rip out the > > setHasAlpha(false) call earlier in the file? > > > > More generally, why expose setHasAlpha() at all? If there's no benefit, then > no > > callers should ever call it, and the API shouldn't exist. This is the same > > comment I had for the Skia code. > > > > I don't really want to go halfway on this one. If we believe this isn't a > win, > > let's rip the entire API out as far down as we can. Otherwise let's leave > > everything. > > I think those are different. > An image may or may not have alpha. That is on the concept level. > If the code paths for painting are the same speed, that's on the implementation > level. > > Just because the implementation has no speed difference doesn't mean we should > get rid of the concept. If there's no speed difference, why are we keeping the concept? I'm not opposed to keeping it, if there's actually a good reason to have it. But I don't see why we'd keep it around without some other explicit purpose. The answer to this will help answer my next question. > The image decoders had this as an optimization. And that optimization may no > longer be valid. It doesn't mean the concept of alpha is bad. It means the times > have changed, hardware has changed, code has changed, and this optimization has > finished serving its purpose. Well, the image decoders had this to try and mark things accurately. The fact that it provided a speed optimization was motivation to keep that accuracy as high as possible. In this CL, you're explicitly making things less accurate; we're now marking a bunch of images that we know do not have alpha as having alpha. The question is, what's the ramification of that? How much do we care about this concept of something having alpha?
On 2017/03/16 22:36:35, Peter Kasting wrote: > On 2017/03/16 20:39:45, cblume wrote: > > > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp > > (right): > > > > > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: > } > > else > > On 2017/03/16 19:43:27, Peter Kasting wrote: > > > I'm still confused. If you really want to do this, why not mark all BMPs as > > > having alpha all the time, i.e. rip out this conditional and rip out the > > > setHasAlpha(false) call earlier in the file? > > > > > > More generally, why expose setHasAlpha() at all? If there's no benefit, > then > > no > > > callers should ever call it, and the API shouldn't exist. This is the same > > > comment I had for the Skia code. > > > > > > I don't really want to go halfway on this one. If we believe this isn't a > > win, > > > let's rip the entire API out as far down as we can. Otherwise let's leave > > > everything. > > > > I think those are different. > > An image may or may not have alpha. That is on the concept level. > > If the code paths for painting are the same speed, that's on the > implementation > > level. > > > > Just because the implementation has no speed difference doesn't mean we should > > get rid of the concept. > > If there's no speed difference, why are we keeping the concept? I'm not opposed > to keeping it, if there's actually a good reason to have it. But I don't see > why we'd keep it around without some other explicit purpose. > > The answer to this will help answer my next question. > > > The image decoders had this as an optimization. And that optimization may no > > longer be valid. It doesn't mean the concept of alpha is bad. It means the > times > > have changed, hardware has changed, code has changed, and this optimization > has > > finished serving its purpose. > > Well, the image decoders had this to try and mark things accurately. The fact > that it provided a speed optimization was motivation to keep that accuracy as > high as possible. In this CL, you're explicitly making things less accurate; > we're now marking a bunch of images that we know do not have alpha as having > alpha. The question is, what's the ramification of that? How much do we care > about this concept of something having alpha? Ehhh sort of. An alternative interpretation is this patch is *more* accurate about what the file told us. If the file said this frame has alpha we can report that. There are a few different cases where we do and don't reinterpret what the file said. For example, the file might claim it is 100x100 but only provide the first 50 rows. In this case we honor what the file said and display a partial decode. We don't reinterpret the file to make it only 50 pixels tall. Another example (this time where we do reinterpret) is in gif's logical screen descriptor. We end up ignoring the canvas width / height. We set it to the size of the first frame: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... So I don't know that I feel strongly about accuracy. Since the image displays the same either way, I see it more as an optimization issue.
On 2017/03/16 23:07:50, cblume wrote: > On 2017/03/16 22:36:35, Peter Kasting wrote: > > On 2017/03/16 20:39:45, cblume wrote: > > > > > > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > > > File > third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2756463003/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: > > } > > > else > > > On 2017/03/16 19:43:27, Peter Kasting wrote: > > > > I'm still confused. If you really want to do this, why not mark all BMPs > as > > > > having alpha all the time, i.e. rip out this conditional and rip out the > > > > setHasAlpha(false) call earlier in the file? > > > > > > > > More generally, why expose setHasAlpha() at all? If there's no benefit, > > then > > > no > > > > callers should ever call it, and the API shouldn't exist. This is the > same > > > > comment I had for the Skia code. > > > > > > > > I don't really want to go halfway on this one. If we believe this isn't a > > > win, > > > > let's rip the entire API out as far down as we can. Otherwise let's leave > > > > everything. > > > > > > I think those are different. > > > An image may or may not have alpha. That is on the concept level. > > > If the code paths for painting are the same speed, that's on the > > implementation > > > level. > > > > > > Just because the implementation has no speed difference doesn't mean we > should > > > get rid of the concept. > > > > If there's no speed difference, why are we keeping the concept? I'm not > opposed > > to keeping it, if there's actually a good reason to have it. But I don't see > > why we'd keep it around without some other explicit purpose. > > > > The answer to this will help answer my next question. > > > > > The image decoders had this as an optimization. And that optimization may no > > > longer be valid. It doesn't mean the concept of alpha is bad. It means the > > times > > > have changed, hardware has changed, code has changed, and this optimization > > has > > > finished serving its purpose. > > > > Well, the image decoders had this to try and mark things accurately. The fact > > that it provided a speed optimization was motivation to keep that accuracy as > > high as possible. In this CL, you're explicitly making things less accurate; > > we're now marking a bunch of images that we know do not have alpha as having > > alpha. The question is, what's the ramification of that? How much do we care > > about this concept of something having alpha? > > Ehhh sort of. > An alternative interpretation is this patch is *more* accurate about what the > file told us. > If the file said this frame has alpha we can report that. Well, that's certainly not true of the BMP changes here :). If you wanted to report whether the file has the potential for alpha, it'd be done by checking for 32bpp format. More critically, the distinction between whether an image actually has alpha and whether the file says the image is allowed to have alpha is relevant. One could imagine reasons to track the latter in an image metadata viewer or something, but in the case of Chrome, I can't imagine that we would have reason to care about anything but the former. > So I don't know that I feel strongly about accuracy. Yes, I don't think one should answer that question on principle. It should be answered based on practice: what does tracking this get us? This was the key question I was trying to ask. I see a couple paths forward: * Collect data and find that speed matters (or don't collect data, in which case I'm very reluctant to assume speed doesn't matter), and leave things as they are. * Decide that code simplicity is the driving factor here and be convinced that speed doesn't matter, and remove alpha tracking from image decoders entirely, probably from ImageFrame, possibly from layers further down. * Discuss some other set of factors for which alpha tracking is relevant besides painting optimizations, and base what the code does on the needs of those other factors.
On 2017/03/17 00:47:49, Peter Kasting wrote: > > An alternative interpretation is this patch is *more* accurate about what the > > file told us. > > If the file said this frame has alpha we can report that. > > Well, that's certainly not true of the BMP changes here :). If you wanted to > report whether the file has the potential for alpha, it'd be done by checking > for 32bpp format. Oh. Maybe I misunderstood something. I think maybe I get what you are saying... The section of code I was looking at seemed to already in the case of when the bitmap is 32bpp. But we can't only use "32bpp" as the sign of whether the image actually has alpha or not because some image encoders would fill the alpha channel with 0s. So if we only reported "Yeah, it is 32bpp and thus has alpha" we would cause problems. This is a case where we need to reinterpret the image data because what was sent is not what was intended. I think this is what you mean. In this case we still have to reinterpret and not be accurate about what the image sent us. So we need to keep a special case for an alpha channel with all 0s. I don't care whether that means we report it as having no alpha or if we report it as having alpha but change all the 0s to 255. The end results will display the image correctly in either of these cases. So at that point I care more about what is easier to maintain & faster to run. > More critically, the distinction between whether an image actually has alpha and > whether the file says the image is allowed to have alpha is relevant. One could > imagine reasons to track the latter in an image metadata viewer or something, > but in the case of Chrome, I can't imagine that we would have reason to care > about anything but the former. I agree with you that we don't care in Chrome whether the image *could* have alpha. I'm not trying to propose that. I'm trying to propose not making a special case for an alpha channel filled with 255s. In that case, there *IS* an alpha and it was useless. Again, we can paint it either way (without alpha or with alpha all 255). The result is the same. So we should go with what is easier to maintain & faster to run. > * Collect data and find that speed matters (or don't collect data, in which case > I'm very reluctant to assume speed doesn't matter), and leave things as they > are. I agree with this. Making random changes based on a guess is not a good way to go. We already have some data, which indicates a benefit to removing the special case for 255. I'm happy to collect as much data as is warranted. We can't test every image ever made. But we can get a handful of images of various sizes / encoding params that have alpha filled with 255. > * Decide that code simplicity is the driving factor here and be convinced that > speed doesn't matter, and remove alpha tracking from image decoders entirely, > probably from ImageFrame, possibly from layers further down. I think I follow. Like if we don't care whether it is faster or slower, there is no need to test the speed. I feel the 255 tracking is not THAT much complexity. I vote in favor of speed.
On 2017/03/17 20:53:15, cblume wrote: > On 2017/03/17 00:47:49, Peter Kasting wrote: > > > An alternative interpretation is this patch is *more* accurate about what > the > > > file told us. > > > If the file said this frame has alpha we can report that. > > > > Well, that's certainly not true of the BMP changes here :). If you wanted to > > report whether the file has the potential for alpha, it'd be done by checking > > for 32bpp format. > > Oh. Maybe I misunderstood something. > I think maybe I get what you are saying... > > The section of code I was looking at seemed to already in the case of when the > bitmap is 32bpp. > But we can't only use "32bpp" as the sign of whether the image actually has > alpha or not because some image encoders would fill the alpha channel with 0s. Correct. And we can't just ignore 0s with nonzero RGB data either because sometimes the image has some 0s and some non-0s. So we have this horrible hack where as long as we've only seen 0s we ignore them, but as soon as we see a non-0 we retroactively decide the previous 0s mean "fully transparent". And even that still doesn't display all BMPs "correctly" because sometimes the file literally does not contain enough data to figure out that you should have just ignored alpha in it. The core of the problem is that BMP is just a disk serialization of a memory buffer, and programs can put crap in the memory buffer that doesn't make sense but that they know how to interpret. If that gets written out for display, good luck. Trying to fix these cases was fun. Lots of test images from Chinese message boards. > Again, we can paint it either way (without alpha or with alpha all 255). The > result is the same. So we should go with what is easier to maintain & faster to > run. Well, since we're decoding to a 32bpp memory buffer, in terms of the memory buffer, it always has alpha. And we always set the alpha to 255 on pixels that we want to be fully opaque. It's just that we also call sethasAlpha(true/false) to mark whether there is any non-255 alpha in the whole image, and assume that maybe the callee cares. > > * Collect data and find that speed matters (or don't collect data, in which > case > > I'm very reluctant to assume speed doesn't matter), and leave things as they > > are. > > I agree with this. Making random changes based on a guess is not a good way to > go. > We already have some data, which indicates a benefit to removing the special > case for 255. We do? Based on msarett's reply I thought we had that data for "we should remove per-pixel alpha checking for PNG decode because it is much more expensive than any paint optimization we get". > I'm happy to collect as much data as is warranted. > We can't test every image ever made. > But we can get a handful of images of various sizes / encoding params that have > alpha filled with 255. I would expect 32bpp, all alpha values == 255 to be the common case, actually, since BMPs tend to originate with Windows, and Windows generally dumps 32 bpp opaque BMPs. I believe using other bit formats or actually having transparency is the unusual case. I could be wrong on this since I haven't looked at the data since I wrote the BMP decoder. The speed thing that I care about is simply whether marking setHasAlpha(false) on known-opaque images buys us paint perf in any configuration (e.g. software rendering on old machines with no memory and bad video cards, mobile, etc.) and how much. If we find it does nothing, then we can just rip all this out. If it does a lot, we should probably leave it all in. If it does a little, then we might have to think more, e.g. by looking at whether we get any perf win from not tracking these cases in the decoders. (TBH, I kind of doubt it, based on how the code is structured.)
On 2017/03/16 14:04:14, msarett wrote: > > +Matt -- If I remember correctly, we saw there is no longer a speed benefit in > > the opaque drawing case? I might be wrong about this. > > > > I investigated something similar a while ago with RGBA PNGs where all of the > alphas are 255. > In that case: > ***There was a real decode performance win to drop the per pixel alpha checking > ***Decode+draw benchmarks were dominated by decode time, draw time was > essentially negligible Drive-by comment: this analysis assumes a single decode and a single draw, no? If the image is drawn thousands of times for a single decode (e.g., animation), even a small per-draw perf win may outweigh the extra cost at decode time. Perhaps the decode+draw benchmarks don't exercise this use case?
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-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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...
There were some tests that were failing because they expected the special case of opaque to be found. Or so I thought. When I went to update them it looks like some might not have been the special case. What I do NOT want to get rid of is reporting when the frame itself says it is opaque. For a single-frame image like boston.gif which has no transparency, I would expect us to report transparency only when doing a partial decode. But once the image is fully decoded, it should be marked as opaque. This patch seems to break that. I do not want that. I'll look into it. I updated the tests to remove what looked like it might be detecting the opaque special case but I'm going to put it back if it seems like that was not the special case and was instead normal "this frame reports opaque." On 2017/03/17 21:12:15, Peter Kasting wrote: > On 2017/03/17 20:53:15, cblume wrote: > > I agree with this. Making random changes based on a guess is not a good way to > > go. > > We already have some data, which indicates a benefit to removing the special > > case for 255. > > We do? Based on msarett's reply I thought we had that data for "we should > remove per-pixel alpha checking for PNG decode because it is much more expensive > than any paint optimization we get". Right. That's what I mean by some. I'll definitely test all the cases I reasonably can. On 2017/03/17 21:12:15, Peter Kasting wrote: > The speed thing that I care about is simply whether marking setHasAlpha(false) > on known-opaque images buys us paint perf in any configuration (e.g. software > rendering on old machines with no memory and bad video cards, mobile, etc.) and > how much. If we find it does nothing, then we can just rip all this out. If it > does a lot, we should probably leave it all in. If it does a little, then we > might have to think more, e.g. by looking at whether we get any perf win from > not tracking these cases in the decoders. (TBH, I kind of doubt it, based on > how the code is structured.) I completely agree. There are some VERY limited Android devices which I care about deeply. So even if it makes no difference on desktops running GPU raster, I want to look for weak-but-supported devices. On 2017/03/21 14:00:40, Stephen White wrote: > On 2017/03/16 14:04:14, msarett wrote: > > > +Matt -- If I remember correctly, we saw there is no longer a speed benefit > in > > > the opaque drawing case? I might be wrong about this. > > > > > > > I investigated something similar a while ago with RGBA PNGs where all of the > > alphas are 255. > > In that case: > > ***There was a real decode performance win to drop the per pixel alpha > checking > > ***Decode+draw benchmarks were dominated by decode time, draw time was > > essentially negligible > > Drive-by comment: this analysis assumes a single decode and a single draw, no? > If the image is drawn thousands of times for a single decode (e.g., animation), > even a small per-draw perf win may outweigh the extra cost at decode time. > Perhaps the decode+draw benchmarks don't exercise this use case? That's a good point. I'll be sure to separately measure decode and draw times. That will help us make wise decisions. For example, if it takes 200 paints to regain the perf hit on decode, maybe we don't normally display 200 frames of an animation (maybe the user scrolls away too quickly).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/03/21 19:27:34, cblume wrote: > What I do NOT want to get rid of is reporting when the frame itself says it is > opaque. > > For a single-frame image like boston.gif which has no transparency, I would > expect us to report transparency only when doing a partial decode. But once the > image is fully decoded, it should be marked as opaque. > > This patch seems to break that. I do not want that. I had only been looking closely at the BMP decoder. Regarding the GIF decoder, yes, you are removing the only route it currently uses to mark any frame as opaque. Unless I am misremembering the spec, GIFs don't have an "alpha channel" per se (they have a signal color which is "transparent") and thus this is basically the only way they can detect if transparency is used. There's no way ahead of time to say "this frame is fully opaque" that would catch most cases, the way there theoretically is with BMP.
On 2017/03/21 19:36:40, Peter Kasting wrote: > On 2017/03/21 19:27:34, cblume wrote: > > What I do NOT want to get rid of is reporting when the frame itself says it is > > opaque. > > > > For a single-frame image like boston.gif which has no transparency, I would > > expect us to report transparency only when doing a partial decode. But once > the > > image is fully decoded, it should be marked as opaque. > > > > This patch seems to break that. I do not want that. > > I had only been looking closely at the BMP decoder. Regarding the GIF decoder, > yes, you are removing the only route it currently uses to mark any frame as > opaque. > > Unless I am misremembering the spec, GIFs don't have an "alpha channel" per se > (they have a signal color which is "transparent") and thus this is basically the > only way they can detect if transparency is used. There's no way ahead of time > to say "this frame is fully opaque" that would catch most cases, the way there > theoretically is with BMP. Right. GIF frames may have a graphic control extension, which specifies 1.) if a color index should be treated as 100% transparent, and 2.) which index in the color table it should be. It is possible to mark an index as being transparent but for a frame to never use that color index. In that way, a frame could specify that it has alpha but never use it. The current code compares a given color index against transparentPixel, which is initialized to SkGIFColorMap::kNotFound. I will check if transparentPixel is SkGIFColorMap::kNotFound to discover whether or not the image encoder claimed there would be a transparent pixel. This lets us remove the per-pixel check.
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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.
I've fixed the gif decoder to set the transparency to what is claimed in the optional graphic control extension before each frame. The graphic control extension has a bit indicating whether there is transparency in this frame or not. And if so, the next byte indicates which index is to be treated as transparent. I not use that as my indicator whether or not this frame will have transparency. As a consequence, I got to put the tests back to where they were. I had to modify one test. It now defaults a frame to transparent (which makes sense for progressive decoding) and updates to non-transparent when the image indicates so.
On 2017/03/22 16:57:34, cblume wrote: > I've fixed the gif decoder to set the transparency to what is claimed in the > optional graphic control extension before each frame. > > The graphic control extension has a bit indicating whether there is transparency > in this frame or not. And if so, the next byte indicates which index is to be > treated as transparent. I not use that as my indicator whether or not this frame > will have transparency. > > As a consequence, I got to put the tests back to where they were. > I had to modify one test. It now defaults a frame to transparent (which makes > sense for progressive decoding) and updates to non-transparent when the image > indicates so. That behavior sgtm
Looks like this probably needs a rebase. Main issue in terms of doing what you intended is that I think your BMP change is broader than you wanted. (It's not clear to me if you still wanted _any_ BMP change after previous discussion.) Main issue in terms of landing this is still just determining the actual decode and paint time impacts. https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81: setHasAlpha(true); Seems like this shouldn't be here given patches elsewhere that separate these concepts. https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: } else I think removing this will cause all non-RLE BMPs to be marked as having alpha, even if they don't have an alpha channel. https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:948: buffer.setHasAlpha(true); I think this already got removed in another CL? https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:464: buffer.setStatus(ImageFrame::FramePartial); Again, I think this got removed in another CL
I'm working on measuring the timing for this. My measurements right now are surprising. When measuring only the decode speeds (not including any raster) it seems slower when not making a special case for alpha correction. This is odd because it should be strictly less work. If the point was to use a faster raster pipe, we shouldn't have gained any benefit when not using the raster pipe. I'm pretty sure what is happening is by setting the alpha we're effectively prefetching a piece of memory into cache we're about to use (the image frame). The alpha write can be reordered without blocking anything. The other work probably has to stall on the cache miss. I've only tested gifs so far.
scroggo@chromium.org changed reviewers: + msarett@chromium.com
On 2017/05/04 09:41:38, cblume wrote: > I'm working on measuring the timing for this. > > My measurements right now are surprising. When measuring only the decode speeds > (not including any raster) it seems slower when not making a special case for > alpha correction. This is odd because it should be strictly less work. > > If the point was to use a faster raster pipe, we shouldn't have gained any > benefit when not using the raster pipe. > > I'm pretty sure what is happening is by setting the alpha we're effectively > prefetching a piece of memory into cache we're about to use (the image frame). > The alpha write can be reordered without blocking anything. The other work > probably has to stall on the cache miss. > > I've only tested gifs so far. Interesting. I wonder if you'd see the same result with SkCodec's SIMD-optimized functions.
https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:172: m_currentBufferSawAlpha = true; I confirmed this line is making a difference and it is 100% because of it acting like a prefetch for the cpu. I stepped through each piece and this had the largest timing impact. I then replaced it with a __builtin_prefetch() and got the same results. This is because after this function returns, execution resumes in GIFLZWContext::outputRow(), which starts to use memory around that location. Since no where else in the code bases uses CPU prefetching (other than V8) and we don't even do low-hanging cpu cache improvements like carefully laying out structs, I'm going to go ahead and say now is a bad time to worry about the effects of CPU caching. If we care, we should do that in a separate bug/CL. But that means this is now within the noise floor.
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.
https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:81: setHasAlpha(true); On 2017/03/22 23:37:46, Peter Kasting wrote: > Seems like this shouldn't be here given patches elsewhere that separate these > concepts. Done. https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:825: } else On 2017/03/22 23:37:46, Peter Kasting wrote: > I think removing this will cause all non-RLE BMPs to be marked as having alpha, > even if they don't have an alpha channel. Hrmmm I think you might be right. GetAlpha() uses the bitmask's default to return 0xff. The call to GetAlpha() is effectively If (FileSupportsAlpha()) { alpha = GetAlpha(); } else { alpha = 0xff; } I think I'll separate that out. That will let us be more explicit anyway. https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:948: buffer.setHasAlpha(true); On 2017/03/22 23:37:47, Peter Kasting wrote: > I think this already got removed in another CL? I don't see it in any of my other CLs after a quick look. The one that I think might have been closest is https://codereview.chromium.org/2762643004/ https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:464: buffer.setStatus(ImageFrame::FramePartial); On 2017/03/22 23:37:47, Peter Kasting wrote: > Again, I think this got removed in another CL I did a quick look around my CLs but didn't see it elsewhere.
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:355: // After decoding, the frame is known to be opaque. This line looks to be no longer true. https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:359: // Without the file, we remain at the default: transparent When test_gif is false, it means that data_ is a PNG, so this really means that we're testing PNG, not testing without a file. (Maybe this for loop should be made more clear?) https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:821: seen_zero_alpha_pixel_ = true; Why did this get moved out here? Now it will always be true when we execute the code below. And it is not used anywhere else. https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:247: return GetComponent(pixel, 3); Previously, this was only called if (bit_masks_[3]), which, in the new code is !IsAlphaSupported(). But now, this method is only called if (IsAlphaSupported()). It seems like this got flipped? https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:195: buffer.SetHasAlpha(transparent_pixel != kNotFound); I think we should also consider the case where the transparent pixel is outside the range of its ColorMap (which may be either the local or global one).
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
Shoot. I combined a rebase and changes. Sorry. https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:355: // After decoding, the frame is known to be opaque. On 2017/05/08 14:56:36, scroggo_chromium wrote: > This line looks to be no longer true. Done. https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:359: // Without the file, we remain at the default: transparent On 2017/05/08 14:56:36, scroggo_chromium wrote: > When test_gif is false, it means that data_ is a PNG, so this really means that > we're testing PNG, not testing without a file. (Maybe this for loop should be > made more clear?) Done.
https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:195: buffer.SetHasAlpha(transparent_pixel != kNotFound); On 2017/05/08 14:56:36, scroggo_chromium wrote: > I think we should also consider the case where the transparent pixel is outside > the range of its ColorMap (which may be either the local or global one). Done.
https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:247: return GetComponent(pixel, 3); On 2017/05/08 14:56:36, scroggo_chromium wrote: > Previously, this was only called if (bit_masks_[3]), which, in the new code is > !IsAlphaSupported(). But now, this method is only called if > (IsAlphaSupported()). It seems like this got flipped? Huh. Yeah. IIUC, bit_masks_[3] is initialized at either * if IsWindowsV4Plus [1] with the value read, or * otherwise [2] with the value 0xff000000 or 0. It seems like the value 0 means we don't yet know that there is alpha. This is when it would begin checking for an alpha value other than 255. So if no-alpha means bit_masks_[3] is 0, !bit_masks_[3] should return true if there is no alpha. That's the opposite of what I wanted. You're right. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... [3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:362: // When the file is a PNG (test_gif is false), we remain at The comment seems to imply that all PNG files will remain transparent. But I don't think we want that to be true. If the PNG file is known to be opaque (e.g. if it's paletted, it does not have transparent indices, or its type is PNG_COLOR_TYPE_RGB), it should be marked opaque. This image appears to use PNG_COLOR_TYPE_RGB, so we should've marked it as opaque. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:149: ((info_header_.bi_bit_count < 16) || IsAlphaSupported() || Now that you flipped IsAlphaSupported() back, don't you want !IsAlphaSupported() here? https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:821: seen_zero_alpha_pixel_ = true; Now this always gets set to true before we read it. I don't see how this could be the intended behavior. (I already brought this up in https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou...) https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:827: if (seen_zero_alpha_pixel_) { This will always be true, since we always set it to true on line 821. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:829: seen_zero_alpha_pixel_ = false; No one ever reads this value, since we set it to true again on line 821 before we ever read it. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:830: } else { We will never hit this branch. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:247: return GetComponent(pixel, 3); Should this DCHECK(IsAlphaSupported()); ? https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:199: buffer.SetHasAlpha(transparent_pixel != kNotFound && The first check is redundant - kNotFound is max size_t, so it will be greater than color_table.size(). So this could just be buffer.SetHasAlpha(transparentPixel <= color_table.size());
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:821: seen_zero_alpha_pixel_ = true; On 2017/05/08 14:56:36, scroggo_chromium wrote: > Why did this get moved out here? Now it will always be true when we execute the > code below. And it is not used anywhere else. I think this was moved as a mistake. If you look on the left, I removed two "= true;" lines. But they are setting different variables that have somewhat similar names. If I mistakenly thought they were the same variable, I may have thought "Both the if and else branch set this to true. I'll move it out of the branch." I'm trying to think of a situation where this may not have been a mistake. But then things like the "if (seen_zero_alpha_pixel_)" line make no sense. I'm convinced this was a mistake. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:821: seen_zero_alpha_pixel_ = true; On 2017/05/30 19:45:17, scroggo_chromium wrote: > Now this always gets set to true before we read it. I don't see how this could > be the intended behavior. > > (I already brought this up in > https://codereview.chromium.org/2756463003/diff/160001/third_party/WebKit/Sou...) Done. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:827: if (seen_zero_alpha_pixel_) { On 2017/05/30 19:45:17, scroggo_chromium wrote: > This will always be true, since we always set it to true on line 821. Done. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:829: seen_zero_alpha_pixel_ = false; On 2017/05/30 19:45:17, scroggo_chromium wrote: > No one ever reads this value, since we set it to true again on line 821 before > we ever read it. Done. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:830: } else { On 2017/05/30 19:45:17, scroggo_chromium wrote: > We will never hit this branch. Done.
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/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:362: // When the file is a PNG (test_gif is false), we remain at On 2017/05/30 19:45:17, scroggo_chromium wrote: > The comment seems to imply that all PNG files will remain transparent. But I > don't think we want that to be true. > > If the PNG file is known to be opaque (e.g. if it's paletted, it does not have > transparent indices, or its type is PNG_COLOR_TYPE_RGB), it should be marked > opaque. This image appears to use PNG_COLOR_TYPE_RGB, so we should've marked it > as opaque. I will try to word it better. I meant to say that the png file used by this test is transparent.
https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:149: ((info_header_.bi_bit_count < 16) || IsAlphaSupported() || On 2017/05/30 19:45:17, scroggo_chromium wrote: > Now that you flipped IsAlphaSupported() back, don't you want !IsAlphaSupported() > here? Done. https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:247: return GetComponent(pixel, 3); On 2017/05/30 19:45:17, scroggo_chromium wrote: > Should this > > DCHECK(IsAlphaSupported()); > > ? Good idea.
https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:199: buffer.SetHasAlpha(transparent_pixel != kNotFound && On 2017/05/30 19:45:17, scroggo_chromium wrote: > The first check is redundant - kNotFound is max size_t, so it will be greater > than color_table.size(). > > So this could just be > > buffer.SetHasAlpha(transparentPixel <= color_table.size()); Done.
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.
> Remove opaque alpha channel special case > > The image decoders which support alpha have a special case to handle > situations where the alpha channel is unused. > > This can come up from a handful of cases but the net effect is we are > detecting if the image was actually opaque, despite the image being > encoded to say that it wasn't. > > Some scenarios where this could happen are: > - a gif frame's palette includes transparency but no pixels use that > transparent palette entry, > - an animation frame completely covering the previous frame & having no > transparent pixels, and > - an animation frame partially covering a previous frame, but the > previous frame containing no visible transparent pixels. > > The reason we detected when the image was actually opaque is because it > was faster to draw opaque images. This might not be true any more with > things like GPU raster. It might not even be true for CPU raster > now-a-days. Better than speculation would be data here. If it's *not* true, I agree with Peter - no reason to report alpha at all. If the goal is to avoid O(n) work, as was the goal in SkCodec when we removed alpha detection, this should say so. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:362: // The PNG file used by this test (when test_gif is false) is transparent. The PNG file is actually an opaque white 1x1 image. We are just not currently recognizing that it was PNG_COLOR_TYPE_RGB and has no tRNS chunk, so it must be opaque. But it seems like we should - just how in GIF we're considering a frame that does not have a transparent pixel to be opaque, we should consider this PNG to be opaque. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:831: } else { Why did you remove "if (alpha != 255)" here? I think the overall goal of this change is the following: - if we can do O(1) work to determine an image is opaque, mark it opaque - if we have to do O(n) work to determine that the image is opaque, we skip that work Following that logic, it seems like we should really be calling buffer_->SetHasAlpha(true) once if IsAlphaSupported(). Instead, IIUC, you're now calling SetHasAlpha(true) even if alpha is 255. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:186: // Initialize the frame if necessary. Some GIFs insert do-nothing frames, Should we consider the case of a do-nothing frame? In that case, I think the alpha should be the same as the prior frame. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:946: buffer.SetHasAlpha(true); This removal is related, but isn't called out in the change description. It won't change anything (has_alpha_ is initialized to false, and the ImageFrame is considered to have alpha until it's FrameComplete, regardless of has_alpha). Do we already have a test that verifies this? https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:272: has_alpha_channel_ = (channels == 4); if channels == 3, we know that the image is opaque. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:260: buffer.SetHasAlpha(true); If we're going to remove the other call to SetHasAlpha (which is used for single frame images), shouldn't we also remove this one (which is used for multi-frame images)?
https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:362: // The PNG file used by this test (when test_gif is false) is transparent. On 2017/05/31 14:27:50, scroggo_chromium wrote: > The PNG file is actually an opaque white 1x1 image. We are just not currently > recognizing that it was PNG_COLOR_TYPE_RGB and has no tRNS chunk, so it must be > opaque. > > But it seems like we should - just how in GIF we're considering a frame that > does not have a transparent pixel to be opaque, we should consider this PNG to > be opaque. Ooooh THAT is what you were saying. Okay, I'll update the comment. I think I'll also create a separate bug for adding that detection.
Description was changed from ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The reason we detected when the image was actually opaque is because it was faster to draw opaque images. This might not be true any more with things like GPU raster. It might not even be true for CPU raster now-a-days. R=scroggo@chromium.org BUG=702059 ========== to ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) for the size of the color palette. After timing, we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. R=scroggo@chromium.org BUG=702059 ==========
Description was changed from ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) for the size of the color palette. After timing, we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. R=scroggo@chromium.org BUG=702059 ========== to ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) for the size of the color palette. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. [1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz... R=scroggo@chromium.org BUG=702059 ==========
https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:831: } else { On 2017/05/31 14:27:50, scroggo_chromium wrote: > Why did you remove "if (alpha != 255)" here? I think the overall goal of this > change is the following: > - if we can do O(1) work to determine an image is opaque, mark it opaque > - if we have to do O(n) work to determine that the image is opaque, we skip that > work > > Following that logic, it seems like we should really be calling > buffer_->SetHasAlpha(true) once if IsAlphaSupported(). > > Instead, IIUC, you're now calling SetHasAlpha(true) even if alpha is 255. You are right that we should ideally call buffer_->SetHasAlpha(true) once if IsAlphaSupported(). But the more I think about it...this loop has to run anyway since bmp's alpha can't be known without looking at the pixel values. And since the loop is running anyway, we aren't really able to trim any O(n) out. I think the main focus of this patch should be removing O(n) where we can. Since this isn't an option here, I think I actually want to leave BMP's existing behavior of correcting when it finds it.
https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:186: // Initialize the frame if necessary. Some GIFs insert do-nothing frames, On 2017/05/31 14:27:50, scroggo_chromium wrote: > Should we consider the case of a do-nothing frame? In that case, I think the > alpha should be the same as the prior frame. I imagine there might be two types of do-nothing frames. One type would be filled with 100% transparent pixels. In that case, there would be a transparent pixel index and we treat the do-nothing frame as transparent (even if the previous frame wasn't). The other type of do-nothing frame might replace some pixels with their exact same color values. We again would follow the transparent pixel index, which may or may not be set in this case. For example, the frame rect might be a 1x1 pixel and it uses the previous color so the frame would appear to not have alpha. (It could also be all the way up to the full area, replacing the whole frame with itself.) In either case, we would do some work to process the do-nothing frame. If we mark the do-nothing frame as transparent unconditionally this is only different for the second case (the 1x1 previous-color frame example). I'm not sure I see any benefit to marking this frame transparent. Additionally, wouldn't this require detecting that it is a do-nothing frame? I suspect I misunderstand what a do-nothing frame means here.
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 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/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:272: has_alpha_channel_ = (channels == 4); On 2017/05/31 14:27:50, scroggo_chromium wrote: > if channels == 3, we know that the image is opaque. Just checking -- you aren't pointing this out as something you want me to change, right? Right now it looks like the constructor and this line are the only lines that set has_alpha_channel_. So this is now the critical line that determines if we report alpha or not. The now-removed current_buffer_saw_alpha_ used to also influence whether we reported alpha or not.
> The first case is O(n) for the size of the color palette. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. nit: This is a lot easier to read with line breaks > The first case is O(n) for the size of the color palette. What is "the first case"? The transparent pixel is unused? Detecting that requires looking at all the pixels. > After timing [1], we see that the image decodes slow > down marginally because setting the alpha bit was > effectively prefetching memory. Is this still true with the switch to SkGifCodec? If not, why not switch directly to SkGifCodec without this intermediate? > Raster times were slower for small images and faster for large images. The description is still lacking a why. Is it because we sped up larger images? What defines a large image versus a small image? https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:831: } else { On 2017/05/31 19:40:37, cblume wrote: > On 2017/05/31 14:27:50, scroggo_chromium wrote: > > Why did you remove "if (alpha != 255)" here? I think the overall goal of this > > change is the following: > > - if we can do O(1) work to determine an image is opaque, mark it opaque > > - if we have to do O(n) work to determine that the image is opaque, we skip > that > > work > > > > Following that logic, it seems like we should really be calling > > buffer_->SetHasAlpha(true) once if IsAlphaSupported(). > > > > Instead, IIUC, you're now calling SetHasAlpha(true) even if alpha is 255. > > You are right that we should ideally call buffer_->SetHasAlpha(true) once if > IsAlphaSupported(). > > But the more I think about it...this loop has to run anyway since bmp's alpha > can't be known without looking at the pixel values. And since the loop is > running anyway, we aren't really able to trim any O(n) out. > > I think the main focus of this patch should be removing O(n) where we can. Since > this isn't an option here, I think I actually want to leave BMP's existing > behavior of correcting when it finds it. I don't understand the distinction - we have to loop for the other decoders, too, so why change them and not this one? https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:186: // Initialize the frame if necessary. Some GIFs insert do-nothing frames, On 2017/05/31 20:35:05, cblume wrote: > On 2017/05/31 14:27:50, scroggo_chromium wrote: > > Should we consider the case of a do-nothing frame? In that case, I think the > > alpha should be the same as the prior frame. > > I imagine there might be two types of do-nothing frames. > > One type would be filled with 100% transparent pixels. > In that case, there would be a transparent pixel index and we treat the > do-nothing frame as transparent (even if the previous frame wasn't). > > The other type of do-nothing frame might replace some pixels with their exact > same color values. > We again would follow the transparent pixel index, which may or may not be set > in this case. > For example, the frame rect might be a 1x1 pixel and it uses the previous color > so the frame would appear to not have alpha. (It could also be all the way up to > the full area, replacing the whole frame with itself.) > > > In either case, we would do some work to process the do-nothing frame. > > If we mark the do-nothing frame as transparent unconditionally this is only > different for the second case (the 1x1 previous-color frame example). I'm not > sure I see any benefit to marking this frame transparent. > > Additionally, wouldn't this require detecting that it is a do-nothing frame? > > I suspect I misunderstand what a do-nothing frame means here. Notice the comment - a "do-nothing" frame means we never reached "HaveDecodedRow()". So we didn't get a signal that the frame was empty (maybe we did? maybe there were no LZW blocks?), so we have to call InitFrameBuffer - otherwise, this frame won't have an allocated bitmap. It would be trivial to detect here - the status is FrameEmpty (prior to calling InitFrameBuffer). (The status would be something else if InitFrameBuffer had already been called.) As the phrase "do-nothing" suggests, a "do-nothing" frame does nothing. If the frame is independent, this would be completely transparent. (I think that would only work for the first frame.) Otherwise it would only change anything if the prior frame is RestoreBGColor - then it would do some erasing (see InitFrameBuffer). In any of these cases, the alpha value is already set how you want it. Are "do-nothing" frames used in practice? I do not know. I see the original bug here [1], but for all I know that is a targeted exploit. But if we're going to support them (the alternative would be to call SetFailed()), it seems like we might as well set their alpha properly. [1] https://bugs.webkit.org/show_bug.cgi?id=22885 https://codereview.chromium.org/2756463003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2756463003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:364: // treated as transparent. reference a bug here?
Description was changed from ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) for the size of the color palette. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. [1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz... R=scroggo@chromium.org BUG=702059 ========== to ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) over the frame's pixel data. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. [1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz... R=scroggo@chromium.org BUG=702059 ==========
Description was changed from ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: - a gif frame's palette includes transparency but no pixels use that transparent palette entry, - an animation frame completely covering the previous frame & having no transparent pixels, and - an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. The first case is O(n) over the frame's pixel data. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. [1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz... R=scroggo@chromium.org BUG=702059 ========== to ========== Remove opaque alpha channel special case The image decoders which support alpha have a special case to handle situations where the alpha channel is unused. This can come up from a handful of cases but the net effect is we are detecting if the image was actually opaque, despite the image being encoded to say that it wasn't. Some scenarios where this could happen are: 1.) a gif frame's palette includes transparency but no pixels use that transparent palette entry, 2.) an animation frame completely covering the previous frame & having no transparent pixels, and 3.) an animation frame partially covering a previous frame, but the previous frame containing no visible transparent pixels. Scenario #1 is O(n) over the frame's pixel data. After timing [1], we see that the image decodes slow down marginally because setting the alpha bit was effectively prefetching memory. Raster times were slower for small images and faster for large images. [1] Requires @google.com account https://docs.google.com/a/google.com/spreadsheets/d/1kzJmvn7hEONCcMlwi4sZnTSz... R=scroggo@chromium.org BUG=702059 ==========
On 2017/05/31 21:12:15, scroggo_chromium wrote: > > The first case is O(n) for the size of the color palette. After timing [1], we > see that the image decodes slow down marginally because setting the alpha bit > was effectively prefetching memory. Raster times were slower for small images > and faster for large images. > > nit: This is a lot easier to read with line breaks Done. > > The first case is O(n) for the size of the color palette. > > What is "the first case"? The transparent pixel is unused? Detecting that > requires looking at all the pixels. Oops, fixed. > > After timing [1], we see that the image decodes slow > > down marginally because setting the alpha bit was > > effectively prefetching memory. > > Is this still true with the switch to SkGifCodec? If not, why not switch > directly to SkGifCodec without this intermediate? No, this timing is independent of SkGifCodec. It is only before & after this patch. If SkGifCodec lands first I'll rebase this patch for the other image decoders. I would be happy to switch directly to SkGifCodec without this intermediate. In fact, I no longer consider this an intermediate. Double in-fact, I would be happy to abandon this patch since it seems like we accept this patch's behavior as part of SkCodec. I think that might be the play here. The other image decoders might be slightly easier to read with this patch but that benefit is insignificant compared to other things I want to work on. > > Raster times were slower for small images and faster for large images. > > The description is still lacking a why. Is it because we sped up larger images? > What defines a large image versus a small image? Once I investigate further to find out why, I'll update.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> I would be happy to switch directly to SkGifCodec without this intermediate. > In fact, I no longer consider this an intermediate. > > Double in-fact, I would be happy to abandon this patch since it seems like we > accept this patch's behavior as part of SkCodec. > I think that might be the play here. > The other image decoders might be slightly easier to read with this patch but > that benefit is insignificant compared to other things I want to work on. Sgtm. https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:272: has_alpha_channel_ = (channels == 4); On 2017/05/31 21:05:29, cblume wrote: > On 2017/05/31 14:27:50, scroggo_chromium wrote: > > if channels == 3, we know that the image is opaque. > > Just checking -- you aren't pointing this out as something you want me to > change, right? > > Right now it looks like the constructor and this line are the only lines that > set has_alpha_channel_. > So this is now the critical line that determines if we report alpha or not. > > The now-removed current_buffer_saw_alpha_ used to also influence whether we > reported alpha or not. I don't want you to change it, but I thought we might take advantage of it. Now that you've removed current_buffer_saw_alpha_, we never report that the image is opaque. But we could: if (!has_alpha_channel_) { buffer.SetHasAlpha(false); } (It's a little more complex than that - you'll need to consider any frames that it blends with. Same for GIF, although SkGifCodec already considers this.) This assumes that there's an advantage to marking the image as opaque, which isn't clear from your timing data.
There is an interesting update here. I still consider this patch abandoned since it will be adopted piece-wise with the SkCodec transition. However, there was mention of always using the alpha path if it was indeed faster. It turns out, we do that. Software raster path [1] and GPU raster path [2] both end up disregarding the alpha flag and creating new SkImage & SkImageInfos which always use the alpha path. Also, apparently I never sent the message below about a change I made. I decided not to remove the comment, but this patch is abandoned and the change below is NOT new. [1] https://cs.chromium.org/chromium/src/cc/tiles/software_image_decode_cache.cc?... [2] https://cs.chromium.org/chromium/src/cc/tiles/gpu_image_decode_cache.cc?dr&l=... https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (left): https://codereview.chromium.org/2756463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:946: buffer.SetHasAlpha(true); On 2017/05/31 14:27:50, scroggo_chromium wrote: > This removal is related, but isn't called out in the change description. It > won't change anything (has_alpha_ is initialized to false, and the ImageFrame is > considered to have alpha until it's FrameComplete, regardless of has_alpha). Do > we already have a test that verifies this? I just ran it before & after on the bots. It looks like this is not tested. Aside from that, you are right that it is unrelated. It makes more sense to leave it and correctly mark a partial image as having transparency. I've added it back in. Since that was the only change to this file, the file was removed from the list of changes.
Message was sent while issue was closed.
Marking closed since patch is abandoned. |
