Ganesh: images upload to GPU performance fix (skip copying encoded data)

third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp

 /*
  * Copyright (C) 2012 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:
  * 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 36 matching lines @@
     ASSERT(buffer->size() >= m_readPosition);
     Vector<RefPtr<SharedBuffer>> newBufferQueue;
 
     const char* segment = 0;
     while (size_t length = buffer->getSomeData(segment, m_readPosition)) {
         m_readPosition += length;
         newBufferQueue.append(SharedBuffer::create(segment, length));
     }
 
     MutexLocker locker(m_mutex);
+    if (m_allDataReceived && newBufferQueue.size()) {

[chrishtr, 2015/12/03 17:51:51]

Is this code reachable in one of your testcases? Not sure why this is necessary.
Apologies if this has already been in the code review thread and I missed it.

[aleksandar.stojiljkovic, 2015/12/03 18:04:14]

No, it is not reachable and it mustn't be. It wasn't checked here before though.
It is necessary as at this point SkData could have been returned and underlying
data shouldn't be changed (appending more data and calling data() could cause
change. Just wanted to prevent future changes in other parts triggering
erroneous situation - hence the ASSERT(false).

[chrishtr, 2015/12/03 18:32:00]

If it is not something that can happen, just put in 
ASSERT(!m_allDataReceived || newBufferQueue.empty());

We should try to avoid adding defensive code.

If the call would be a security/crash bug then RELEASE_ASSERT may be
appropriate.

[aleksandar.stojiljkovic, 2015/12/03 21:17:09]

Done.

+        // This prevents from unexisting use case now - if all data was previously
+        // received, don't append more to it.
+        ASSERT(false);
+        newBufferQueue.clear();
+        return;
+    }
+
     m_newBufferQueue.appendVector(newBufferQueue);
     newBufferQueue.clear();
     m_allDataReceived = allDataReceived;
 }
 
 void ThreadSafeDataTransport::data(SharedBuffer** buffer, bool* allDataReceived)
 {
     ASSERT(buffer);
     ASSERT(allDataReceived);
     Vector<RefPtr<SharedBuffer>> newBufferQueue;
     {
         MutexLocker lock(m_mutex);
         m_newBufferQueue.swap(newBufferQueue);
         *allDataReceived = m_allDataReceived;
     }
     for (size_t i = 0; i < newBufferQueue.size(); ++i)
         m_readBuffer->append(newBufferQueue[i].get());
     *buffer = m_readBuffer.get();
 }
 
 bool ThreadSafeDataTransport::hasNewData()
 {
     MutexLocker lock(m_mutex);
     return !m_newBufferQueue.isEmpty();
 }
 
 } // namespace blink