Remove opaque alpha channel special case

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

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 128 matching lines @@
   // Decode the data.
   if (!decoding_and_mask_ && !PastEndOfImage(0) &&
       !DecodePixelData((info_header_.bi_compression != RLE4) &&
                        (info_header_.bi_compression != RLE8) &&
                        (info_header_.bi_compression != RLE24)))
     return false;
 
   // 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);
     coord_.SetY(is_top_down_ ? 0 : (parent_->Size().Height() - 1));
 
     // The AND mask is stored as 1-bit data.
     info_header_.bi_bit_count = 1;
 
     decoding_and_mask_ = true;
   }
@@ skipping 650 matching lines @@
       // RGB data.  Decode pixels one at a time, left to right.
       while (coord_.X() < end_x) {
         const uint32_t pixel = ReadCurrentPixel(bytes_per_pixel);
 
         // Some BMPs specify an alpha channel but don't actually use it
         // (it contains all 0s).  To avoid displaying these images as
         // fully-transparent, decode as if images are fully opaque
         // 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),
                 GetComponent(pixel, 2), alpha);
       }
     }
 
     // Success, keep going.
     decoded_offset_ += padded_num_bytes;
     if (in_rle)
       return kSuccess;
     MoveBufferToNextRow();
   }
 
   // Finished decoding whole image.
   return kSuccess;
 }
 
 void BMPImageReader::MoveBufferToNextRow() {
   coord_.Move(-coord_.X(), is_top_down_ ? 1 : -1);
 }
 
 }  // namespace blink