Fix caching bug in JPEGImageDecoder

Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  *
  * Portions are Copyright (C) 2001-6 mozilla.org
  *
  * Other contributors:
  *   Stuart Parmenter <stuart@mozilla.com>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ skipping 374 matching lines @@
         // We updated |next_input_byte|, so we need to update |m_lastByteSet|
         // so we know not to update |m_restartPosition| again.
         m_lastSetByte = m_info.src->next_input_byte;
     }
 
     bool fillBuffer()
     {
         if (m_needsRestart) {
             m_needsRestart = false;
             m_nextReadPosition = m_restartPosition;
-        } else if (m_lastSetByte != m_info.src->next_input_byte) {
-            // next_input_byte was updated by jpeg, meaning that it found a restart position.
-            m_restartPosition = m_nextReadPosition - m_info.src->bytes_in_buffer;
+        } else {
+            updateRestartPosition();
         }
 
         const char* segment;
         const unsigned bytes = m_data->getSomeData(segment, m_nextReadPosition);
         if (bytes == 0) {
-            // We had to suspend. When we resume, we will need to start from the restart position.
-            m_needsRestart = true;
-            // Let libjpeg know that the buffer needs to be refilled.
-            m_info.src->bytes_in_buffer = 0;
-            m_info.src->next_input_byte = nullptr;
-            m_lastSetByte = nullptr;
+            suspend();
             return false;
         }
 
         m_nextReadPosition += bytes;
         m_info.src->bytes_in_buffer = bytes;
         const JOCTET* nextByte = reinterpret_cast_ptr<const JOCTET*>(segment);
         m_info.src->next_input_byte = nextByte;
         m_lastSetByte = nextByte;
         return true;
     }
 
     void setData(SharedBuffer* data)
     {
         if (m_data.get() == data)
             return;
 
         m_data = data;
 
         // If a restart is needed, the next call to fillBuffer will read from the new SharedBuffer.
         if (m_needsRestart)
             return;
 
         // Otherwise, empty the buffer, and leave the position the same, so fillBuffer continues
         // reading from the same position in the new SharedBuffer.
         m_nextReadPosition -= m_info.src->bytes_in_buffer;
         m_info.src->bytes_in_buffer = 0;

[scroggo_chromium, 2015/09/18 21:07:45]

I considered calling suspend() here, which shares these three lines of code. (It
also sets m_needsRestart to true, but it is guaranteed to be true at this point,
so no harm done.)

I ended up not doing it because it seems like we're not "suspending" in this
case. Maybe it should be renamed something like "clearBuffer" or "emptyBuffer",
since it does the opposite of "fillBuffer". Then it might make sense to call it
here.

[Peter Kasting, 2015/09/19 00:51:34]

I like the idea of renaming this clearBuffer() and calling it here.

[scroggo_chromium, 2015/09/21 18:21:27]

Done.
I was confused here - m_needsRestart is guaranteed to be false here, and we do
not want it to get set to true. So I have moved the line

    m_needsRestart = true;

outside of the newly named clearBuffer(), so it can be used in the places it
should be used, but not here.

         m_info.src->next_input_byte = nullptr;
         m_lastSetByte = nullptr;
     }
 
     bool decode(bool onlySize)
     {
         // We need to do the setjmp here. Otherwise bad things will happen
         if (setjmp(m_err.setjmp_buffer))
             return m_decoder->setFailed();
 
@@ skipping 79 matching lines @@
 
             // Don't allocate a giant and superfluous memory buffer when the
             // image is a sequential JPEG.
             m_info.buffered_image = jpeg_has_multiple_scans(&m_info);
             if (m_info.buffered_image) {
                 m_err.pub.emit_message = emit_message;
                 m_err.num_corrupt_warnings = 0;
             }
 
             if (onlySize) {
+                updateRestartPosition();

[Peter Kasting, 2015/09/19 00:51:34]

You should probably have comments here about why you need to do these.

[scroggo_chromium, 2015/09/21 18:21:28]

Done.

(My instinct is include lots of comments, but previous Blink code reviews have
encouraged me to remove many of them. I seem to have corrected too far in the
other direction.)

+                suspend();
                 return true;
             }
         // FALL THROUGH
 
         case JPEG_START_DECOMPRESS:
             // Set parameters for decompression.
             // FIXME -- Should reset dct_method and dither mode for final pass
             // of progressive JPEG.
             m_info.dct_method = JDCT_ISLOW;
             m_info.dither_mode = JDITHER_FS;
@@ skipping 131 matching lines @@
         // We currently only support color profiles for RGB profiled images.
         ASSERT(rgbData == qcms_profile_get_color_space(inputProfile));
         qcms_data_type dataFormat = hasAlpha ? QCMS_DATA_RGBA_8 : QCMS_DATA_RGB_8;
         // FIXME: Don't force perceptual intent if the image profile contains an intent.
         m_transform = qcms_transform_create(inputProfile, dataFormat, deviceProfile, dataFormat, QCMS_INTENT_PERCEPTUAL);
         qcms_profile_release(inputProfile);
     }
 #endif
 
 private:
+    void updateRestartPosition()
+    {
+        if (m_lastSetByte != m_info.src->next_input_byte) {
+            // next_input_byte was updated by jpeg, meaning that it found a restart position.
+            m_restartPosition = m_nextReadPosition - m_info.src->bytes_in_buffer;
+        }
+    }
+
+    void suspend()
+    {
+        // We had to suspend. When we resume, we will need to start from the restart position.
+        m_needsRestart = true;
+        // Let libjpeg know that the buffer needs to be refilled.
+        m_info.src->bytes_in_buffer = 0;
+        m_info.src->next_input_byte = nullptr;
+        m_lastSetByte = nullptr;
+    }
+
     RefPtr<SharedBuffer> m_data;
     JPEGImageDecoder* m_decoder;
     // True if we need to back up to m_restartPosition.
     bool m_needsRestart;
     // If libjpeg needed to restart, this is the position to restart from.
     unsigned m_restartPosition;
     // This is the position where we will read from, unless there is a restart.
     unsigned m_nextReadPosition;
     // This is how we know to update the restart position. It is the last value
     // we set to next_input_byte. libjpeg will update next_input_byte when it
@@ skipping 360 matching lines @@
     // has failed.
     if (!m_reader->decode(onlySize) && isAllDataReceived())
         setFailed();
 
     // If decoding is done or failed, we don't need the JPEGImageReader anymore.
     if (isComplete(this, onlySize) || failed())
         m_reader.clear();
 }
 
 }