Remove opaque alpha channel special case

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

 /*
  * Copyright (C) 2006 Apple Computer, 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:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 141 matching lines @@
   // The condition check is taken out of the loop to enhance performance.
   // This optimization reduces decoding time by about 15% for a 3MB image.
   if (write_transparent_pixels) {
     for (; row_begin != row_end; ++row_begin, ++current_address) {
       const size_t source_value = *row_begin;
       if ((source_value != transparent_pixel) &&
           (source_value < color_table.size())) {
         *current_address = color_table_iter[source_value];
       } else {
         *current_address = 0;
-        current_buffer_saw_alpha_ = true;
       }
     }
   } else {
     for (; row_begin != row_end; ++row_begin, ++current_address) {
       const size_t source_value = *row_begin;
       if ((source_value != transparent_pixel) &&
           (source_value < color_table.size()))
         *current_address = color_table_iter[source_value];
-      else
-        current_buffer_saw_alpha_ = true;
     }
   }
 
   // Tell the frame to copy the row data if need be.
   if (repeat_count > 1)
     buffer.CopyRowNTimes(x_begin, x_end, y_begin, y_end);
 
   buffer.SetPixelsChanged(true);
   return true;
 }
 
 bool GIFImageDecoder::ParseCompleted() const {
   return reader_ && reader_->ParseCompleted();
 }
 
 bool GIFImageDecoder::FrameComplete(size_t frame_index) {
   // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,

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

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.

[cblume, 2017/05/31 20:35:05]

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.

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

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

   // in which case we never reach HaveDecodedRow() before getting here.
   if (!InitFrameBuffer(frame_index))
     return SetFailed();
 
-  if (!current_buffer_saw_alpha_)
-    CorrectAlphaWhenFrameBufferSawNoAlpha(frame_index);
+  ImageFrame& buffer = frame_buffer_cache_[frame_index];
 
-  frame_buffer_cache_[frame_index].SetStatus(ImageFrame::kFrameComplete);
+  const GIFFrameContext* frame_context = reader_->FrameContext(frame_index);
+  const size_t transparent_pixel = frame_context->TransparentPixel();
+  const GIFColorMap::Table& color_table =
+      frame_context->LocalColorMap().IsDefined()
+          ? frame_context->LocalColorMap().GetTable()
+          : reader_->GlobalColorMap().GetTable();
+  buffer.SetHasAlpha(transparent_pixel <= color_table.size());
+
+  buffer.SetStatus(ImageFrame::kFrameComplete);
 
   return true;
 }
 
 void GIFImageDecoder::ClearFrameBuffer(size_t frame_index) {
   if (reader_ && frame_buffer_cache_[frame_index].GetStatus() ==
                      ImageFrame::kFramePartial) {
     // Reset the state of the partial frame in the reader so that the frame
     // can be decoded again when requested.
     reader_->ClearDecodeState(frame_index);
@@ skipping 54 matching lines @@
 
   if (!reader_) {
     reader_ = WTF::MakeUnique<GIFImageReader>(this);
     reader_->SetData(data_);
   }
 
   if (!reader_->Parse(query))
     SetFailed();
 }
 
-void GIFImageDecoder::OnInitFrameBuffer(size_t frame_index) {
-  current_buffer_saw_alpha_ = false;
-}
-
 bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t frame_index) const {
   DCHECK(frame_index < frame_buffer_cache_.size());
   return frame_buffer_cache_[frame_index].GetDisposalMethod() !=
          ImageFrame::kDisposeOverwritePrevious;
 }
 
 }  // namespace blink