Reenable warnings in src/codec

src/codec/SkCodec_libgif.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodec_libgif.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 12 matching lines @@
             memcmp(GIF87_STAMP, buf, GIF_STAMP_LEN) == 0 ||
             memcmp(GIF89_STAMP, buf, GIF_STAMP_LEN) == 0)
         {
             return true;
         }
     }
     return false;
 }
 
 /*
- * Warning reporting function
- */
-static void gif_warning(const char* msg) {
-    SkCodecPrintf("Gif Warning: %s\n", msg);
-}
-
-/*
  * Error function
  */
 static SkCodec::Result gif_error(const char* msg, SkCodec::Result result = SkCodec::kInvalidInput) {
     SkCodecPrintf("Gif Error: %s\n", msg);
     return result;
 }
 
 
 /*
  * Read function that will be passed to gif_lib
@@ skipping 131 matching lines @@
         return false;
     }
     // If reading the image descriptor is successful, the image count will be
     // incremented.
     SkASSERT(gif->ImageCount >= 1);
 
     if (nullptr != codecOut) {
         SkISize size;
         SkIRect frameRect;
         if (!GetDimensions(gif, &size, &frameRect)) {
             gif_error("Invalid gif size.\n");

[scroggo, 2015/10/15 12:49:54]

I think this should return false. Otherwise, we go on to read size and
frameRect, even though they have not been initialized. If we do not return
false, we'll go down to create a new codec and return true, even though the
dimensions were not valid (and set to zero, in the fix down below).

[msarett, 2015/10/15 13:06:41]

Of course, this is missing a return statement.  I was confused about why the
compiler was complaining about uninitialized values below.

         }
         bool frameIsSubset = (size != frameRect.size());
 
         // Determine the recommended alpha type.  The transIndex might be valid if it less
         // than 256.  We are not certain that the index is valid until we process the color
         // table, since some gifs have color tables with less than 256 colors.  If
         // there might be a valid transparent index, we must indicate that the image has
         // alpha.
         // In the case where we must support alpha, we have the option to set the
         // suggested alpha type to kPremul or kUnpremul.  Both are valid since the alpha
@@ skipping 28 matching lines @@
         return codec;
     }
     return nullptr;
 }
 
 SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream, GifFileType* gif,
         uint32_t transIndex, const SkIRect& frameRect, bool frameIsSubset)
     : INHERITED(srcInfo, stream)
     , fGif(gif)
     , fSrcBuffer(new uint8_t[this->getInfo().width()])
+    , fFrameRect(frameRect)
     // If it is valid, fTransIndex will be used to set fFillIndex.  We don't know if
     // fTransIndex is valid until we process the color table, since fTransIndex may
     // be greater than the size of the color table.
     , fTransIndex(transIndex)
     // Default fFillIndex is 0.  We will overwrite this if fTransIndex is valid, or if
     // there is a valid background color.
     , fFillIndex(0)
-    , fFrameRect(frameRect)
     , fFrameIsSubset(frameIsSubset)
+    , fSwizzler(NULL)
     , fColorTable(NULL)
-    , fSwizzler(NULL)
 {}
 
 bool SkGifCodec::onRewind() {
     GifFileType* gifOut = nullptr;
     if (!ReadHeader(this->stream(), nullptr, &gifOut)) {
         return false;
     }
 
     SkASSERT(nullptr != gifOut);
     fGif.reset(gifOut);
@@ skipping 99 matching lines @@
 
     // Ensure that the decode dimensions are large enough to contain the frame
     width = SkTMax(width, frameWidth + frameLeft);
     height = SkTMax(height, frameHeight + frameTop);
 
     // All of these dimensions should be positive, as they are encoded as unsigned 16-bit integers.
     // It is unclear why giflib casts them to ints.  We will go ahead and check that they are
     // in fact positive.
     if (frameLeft < 0 || frameTop < 0 || frameWidth < 0 || frameHeight < 0 || width <= 0 ||
             height <= 0) {
+        size->set(0, 0);

[scroggo, 2015/10/15 12:49:53]

I take it this fixes a warning that we read these variables even though they
were uninitialized? I think this is the wrong fix (see comments at the call
site). The header states that these variables will be set if GetDimensions
returns true. If it returns false, this function has no obligation to initialize
them, and more importantly, the caller should not read them.

[msarett, 2015/10/15 13:06:40]

Acknowledged.

+        frameRect->setXYWH(0, 0, 0, 0);
         return false;
     }
 
     frameRect->setXYWH(frameLeft, frameTop, frameWidth, frameHeight);
     size->set(width, height);
     return true;
 }
 
 void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr,
         int* inputColorCount) {
@@ skipping 207 matching lines @@
 
 int SkGifCodec::onOutputScanline(int inputScanline) const {
     if (fGif->Image.Interlace) {
         if (inputScanline < fFrameRect.top() || inputScanline >= fFrameRect.bottom()) {
             return inputScanline;
         }
         return get_output_row_interlaced(inputScanline - fFrameRect.top(), fFrameRect.height());
     }
     return inputScanline;
 }