Implement buffer{Sub}Data overloads with sub source.

third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
index b3eabd11b014fe35add63b9d0e4bf2b2c390d7cd..362bdccc3a3e66943b7ac68ad9949a284a4a7d9d 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
@@ -41,6 +41,109 @@ GLsync syncObjectOrZero(const WebGLSync* object)
     return object ? object->object() : nullptr;
 }
 
+class SubArrayBufferView {
+public:
+    SubArrayBufferView(WTF::ArrayBufferView::ViewType viewType, void* viewBaseAddress, long long viewByteLength, GLuint subOffset, GLuint subLength)

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Is "long long" the best type to use for viewByteLength? Is there a better, more
explicit one, like int64_t?

[Zhenyao Mo, 2016/08/20 03:44:23]

This is to match all the other byteLength types in this class - probably change
them all in a separate CL.

+        : m_viewType(viewType)
+        , m_viewBaseAddress(viewBaseAddress)
+        , m_viewByteLength(viewByteLength)
+        , m_subOffset(subOffset)
+        , m_subLength(subLength)

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Since initialize() returns void, what about moving the call to the constructor,
and passing viewType, viewBaseAddress, etc. as arguments to it? That would
eliminate them as members and would reduce confusion about the possible states
of this class.

[Zhenyao Mo, 2016/08/20 03:44:23]

That was my original design, but I remmebered we are not supposed to put heavy
computation into class constructor.  If you feel it's OK, I can revised.

[Ken Russell (switch to Gerrit), 2016/08/20 04:03:34]

I think it'd be OK, and quite a bit simpler.

[Zhenyao Mo, 2016/08/23 00:22:42]

Done.

+        , m_initialized(false)
+        , m_subBaseAddress(nullptr)
+        , m_subByteLength(0)
+        , m_valid(false)
+    {
+    }
+
+    bool isValid()
+    {
+        initialize();

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Per above, suggest moving the call to initialize() to the constructor.

+        return m_valid;
+    }
+
+    void* baseAddress() const
+    {
+        DCHECK(m_initialized);
+        return m_subBaseAddress;
+    }
+
+    long long byteLength() const
+    {
+        DCHECK(m_initialized);
+        return m_subByteLength;
+    }
+
+private:
+    void initialize()
+    {
+        if (m_initialized)
+            return;
+        m_initialized = true;
+        size_t typeSize = getViewTypeSize(m_viewType);
+        DCHECK_GE(8u, typeSize);
+        long long byteLength = 0;
+        if (m_subLength) {
+            // type size is at most 8, so no overflow.
+            byteLength = m_subLength * typeSize;
+        }
+        long long byteOffset = 0;
+        if (m_subOffset) {
+            // type size is at most 8, so no overflow.
+            byteOffset = m_subOffset * typeSize;
+        }
+        CheckedInt<long long> total = byteOffset;
+        total += byteLength;
+        if (!total.isValid() || total.value() > m_viewByteLength) {
+            return;
+        }

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Should there be a separate check like:

  if (!byteLength && byteOffset >= m_viewByteLength)
    return?

The intent would be to handle the case of an attempt to upload 0 bytes from the
byte just beyond the end of the view. I might have gotten this proposed check
wrong.

[Zhenyao Mo, 2016/08/20 03:44:23]

Looking at bufferSubData, where the offset is beyond the buffer object, even
with size 0, we still generate an INVALID_VALUE.  Here we should be consistent
with that.

[Ken Russell (switch to Gerrit), 2016/08/20 04:05:55]

As long as the tests verify this, sounds good. It's not obvious to me that this
is what's enforced for zero-length uploads.

+        if (!byteLength) {
+            byteLength = m_viewByteLength - byteOffset;
+        }
+        uint8_t* data = static_cast<uint8_t*>(m_viewBaseAddress);
+        data += byteOffset;

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Any possibility of overflow on the address? Should this addition be done using
CheckedInt too? Or at least a DCHECK that data + byteOffset >=
m_viewBaseAddress, in unsigned arithmetic?

[Zhenyao Mo, 2016/08/20 03:44:23]

We already checked byteOffset is within the ArrayBufferView bounds, so this will
not overflow.

+        m_subBaseAddress = data;
+        m_subByteLength = byteLength;
+        m_valid = true;
+    }
+
+    static size_t getViewTypeSize(WTF::ArrayBufferView::ViewType viewType)
+    {
+        switch (viewType) {
+        case WTF::ArrayBufferView::TypeInt8:
+        case WTF::ArrayBufferView::TypeUint8:
+        case WTF::ArrayBufferView::TypeUint8Clamped:
+            return 1;
+        case WTF::ArrayBufferView::TypeInt16:
+        case WTF::ArrayBufferView::TypeUint16:
+            return 2;
+        case WTF::ArrayBufferView::TypeInt32:
+        case WTF::ArrayBufferView::TypeUint32:
+        case WTF::ArrayBufferView::TypeFloat32:
+            return 4;
+        case WTF::ArrayBufferView::TypeFloat64:
+            return 8;
+        case WTF::ArrayBufferView::TypeDataView:
+            return 1;
+        default:
+            NOTREACHED();
+            return 0;
+        }
+    }
+
+    WTF::ArrayBufferView::ViewType m_viewType;
+    void* m_viewBaseAddress;
+    long long m_viewByteLength;
+    GLuint m_subOffset;
+    GLuint m_subLength;
+
+    bool m_initialized;

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

I think it's possible and desirable to eliminate all of these data members; see
above.

[Zhenyao Mo, 2016/08/20 03:44:23]

Now I think again, we can actually just do this in a function, so no need for a
class.  Will revise.

+
+    void* m_subBaseAddress;
+    long long m_subByteLength;
+    bool m_valid;
+};
+
 } // namespace
 
 // These enums are from manual pages for glTexStorage2D/glTexStorage3D.
@@ -183,6 +286,57 @@ void WebGL2RenderingContextBase::initializeNewContext()
     WebGLRenderingContextBase::initializeNewContext();
 }
 
+void WebGL2RenderingContextBase::bufferData(GLenum target, DOMArrayBufferView* srcData, GLenum usage, GLuint srcOffset, GLuint length)
+{
+    if (isContextLost())
+        return;
+    DCHECK(srcData);

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Maybe worth adding a comment that this is guaranteed non-null by the DOM
bindings.

[Zhenyao Mo, 2016/08/20 03:44:23]

Will do.

+    SubArrayBufferView subView(srcData->type(), srcData->baseAddress(), srcData->byteLength(), srcOffset, length);

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Perhaps the SubArrayBufferView constructor should just take the ArrayBufferView*
as argument, and fetch all of these properties from it. It would simplify the
call sites.

[Zhenyao Mo, 2016/08/20 03:44:23]

The reason I didn't pass DOMArrayBufferView is that it's a JS object, so if we
keep it as a class member, the class becomes complicated.  Since we can revise
it to a function, this is no longer an concern.

+    if (!subView.isValid()) {
+        synthesizeGLError(GL_INVALID_VALUE, "bufferData", "srcOffset + length too large");
+        return;
+    }
+    bufferDataImpl(target, subView.byteLength(), subView.baseAddress(), usage);
+}
+
+void WebGL2RenderingContextBase::bufferData(GLenum target, long long size, GLenum usage)
+{
+    WebGLRenderingContextBase::bufferData(target, size, usage);
+}
+
+void WebGL2RenderingContextBase::bufferData(GLenum target, DOMArrayBuffer* data, GLenum usage)
+{
+    WebGLRenderingContextBase::bufferData(target, data, usage);
+}
+
+void WebGL2RenderingContextBase::bufferData(GLenum target, DOMArrayBufferView* data, GLenum usage)
+{
+    WebGLRenderingContextBase::bufferData(target, data, usage);
+}
+
+void WebGL2RenderingContextBase::bufferSubData(GLenum target, GLintptr dstByteOffset, DOMArrayBufferView* srcData, GLuint srcOffset, GLuint length)
+{
+    if (isContextLost())
+        return;
+    DCHECK(srcData);
+    SubArrayBufferView subView(srcData->type(), srcData->baseAddress(), srcData->byteLength(), srcOffset, length);
+    if (!subView.isValid()) {
+        synthesizeGLError(GL_INVALID_VALUE, "bufferSubData", "srcOffset + length too large");
+        return;
+    }
+    bufferSubDataImpl(target, dstByteOffset, subView.byteLength(), subView.baseAddress());
+}
+
+void WebGL2RenderingContextBase::bufferSubData(GLenum target, long long offset, DOMArrayBuffer* data)
+{
+    WebGLRenderingContextBase::bufferSubData(target, offset, data);
+}
+
+void WebGL2RenderingContextBase::bufferSubData(GLenum target, long long offset, const FlexibleArrayBufferView& data)
+{
+    WebGLRenderingContextBase::bufferSubData(target, offset, data);
+}
+
 void WebGL2RenderingContextBase::copyBufferSubData(GLenum readTarget, GLenum writeTarget, long long readOffset, long long writeOffset, long long size)
 {
     if (isContextLost())
@@ -219,34 +373,34 @@ void WebGL2RenderingContextBase::copyBufferSubData(GLenum readTarget, GLenum wri
     contextGL()->CopyBufferSubData(readTarget, writeTarget, static_cast<GLintptr>(readOffset), static_cast<GLintptr>(writeOffset), static_cast<GLsizeiptr>(size));
 }
 
-void WebGL2RenderingContextBase::getBufferSubData(GLenum target, long long offset, DOMArrayBuffer* returnedData)
+void WebGL2RenderingContextBase::getBufferSubData(GLenum target, long long srcByteOffset, DOMArrayBufferView* dstData, GLuint dstOffset, GLuint length)
 {
+    const char* funcName = "getBufferSubData";
     if (isContextLost())
         return;
-
-    if (!returnedData) {
-        synthesizeGLError(GL_INVALID_VALUE, "getBufferSubData", "ArrayBuffer cannot be null");
+    if (!validateValueFitNonNegInt32(funcName, "srcByteOffset", srcByteOffset)) {
         return;
     }
-
-    if (!validateValueFitNonNegInt32("getBufferSubData", "offset", offset)) {
+    WebGLBuffer* buffer = validateBufferDataTarget(funcName, target);
+    if (!buffer)
         return;
+    DCHECK(dstData);
+    SubArrayBufferView subView(dstData->type(), dstData->baseAddress(), dstData->byteLength(), dstOffset, length);
+    if (!subView.isValid()) {
+        synthesizeGLError(GL_INVALID_VALUE, funcName, "dstOffset + length too large");

[Ken Russell (switch to Gerrit), 2016/08/20 03:05:35]

Return too, correct?

[Zhenyao Mo, 2016/08/20 03:44:23]

Ah thanks for catching this.

     }
 
-    WebGLBuffer* buffer = validateBufferDataTarget("getBufferSubData", target);
-    if (!buffer)
-        return;
-    if (offset + returnedData->byteLength() > buffer->getSize()) {
-        synthesizeGLError(GL_INVALID_VALUE, "getBufferSubData", "buffer overflow");
+    if (srcByteOffset + subView.byteLength() > buffer->getSize()) {
+        synthesizeGLError(GL_INVALID_VALUE, funcName, "buffer overflow");
         return;
     }
 
-    void* mappedData = contextGL()->MapBufferRange(target, static_cast<GLintptr>(offset), returnedData->byteLength(), GL_MAP_READ_BIT);
+    void* mappedData = contextGL()->MapBufferRange(target, static_cast<GLintptr>(srcByteOffset), subView.byteLength(), GL_MAP_READ_BIT);
 
     if (!mappedData)
         return;
 
-    memcpy(returnedData->data(), mappedData, returnedData->byteLength());
+    memcpy(subView.baseAddress(), mappedData, subView.byteLength());
 
     contextGL()->UnmapBuffer(target);
 }