Remove opaque alpha channel special case

third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp

Index: third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp b/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
index 4426dfd7af5dff491a2e3b93404f2ee0c944a1eb..9f026d986d544ee16ca2a3e8cde1e402bb90c949 100644
--- a/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
@@ -146,7 +146,7 @@ bool BMPImageReader::DecodeBMP(bool only_size) {
   // If the image has an AND mask and there was no alpha data, process the
   // mask.
   if (is_in_ico_ && !decoding_and_mask_ &&
-      ((info_header_.bi_bit_count < 16) || !bit_masks_[3] ||
+      ((info_header_.bi_bit_count < 16) || !IsAlphaSupported() ||
        !seen_non_zero_alpha_pixel_)) {
     // Reset decoding coordinates to start of image.
     coord_.SetX(0);
@@ -817,20 +817,21 @@ BMPImageReader::ProcessingResult BMPImageReader::ProcessNonRLEData(
         // until we actually see a non-zero alpha value; at that point,
         // reset any previously-decoded pixels to fully transparent and
         // continue decoding based on the real alpha channel values.
-        // As an optimization, avoid calling SetHasAlpha(true) for
-        // images where all alpha values are 255; opaque images are
-        // faster to draw.
-        int alpha = GetAlpha(pixel);
-        if (!seen_non_zero_alpha_pixel_ && !alpha) {
-          seen_zero_alpha_pixel_ = true;
-          alpha = 255;
-        } else {
-          seen_non_zero_alpha_pixel_ = true;
-          if (seen_zero_alpha_pixel_) {
-            buffer_->ZeroFillPixelData();
-            seen_zero_alpha_pixel_ = false;
-          } else if (alpha != 255)
-            buffer_->SetHasAlpha(true);
+        int alpha = 255;
+        if (IsAlphaSupported()) {
+          alpha = GetAlpha(pixel);
+          if (!seen_non_zero_alpha_pixel_ && !alpha) {
+            seen_zero_alpha_pixel_ = true;
+            alpha = 255;
+          } else {
+            seen_non_zero_alpha_pixel_ = true;
+            if (seen_zero_alpha_pixel_) {
+              buffer_->ZeroFillPixelData();
+              seen_zero_alpha_pixel_ = false;
+            } else {

[scroggo_chromium, 2017/05/31 14:27:50]

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.

[cblume, 2017/05/31 19:40:37]

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.

[scroggo_chromium, 2017/05/31 21:12:15]

I don't understand the distinction - we have to loop for the other decoders,
too, so why change them and not this one?

+              buffer_->SetHasAlpha(true);
+            }
+          }
         }
 
         SetRGBA(GetComponent(pixel, 0), GetComponent(pixel, 1),