[webkit-reviews] review denied: [Bug 28018] Need to implement Canvas3d/WebGL for 3D rendering : [Attachment 34146] Patch 2 of 4: New Canvas 3D files
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 5 12:11:48 PDT 2009
Oliver Hunt <oliver at apple.com> has denied Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 28018: Need to implement Canvas3d/WebGL for 3D rendering
https://bugs.webkit.org/show_bug.cgi?id=28018
Attachment 34146: Patch 2 of 4: New Canvas 3D files
https://bugs.webkit.org/attachment.cgi?id=34146&action=review
------- Additional Comments from Oliver Hunt <oliver at apple.com>
> +static inline PassRefPtr<CanvasNumberArray>
toCanvasNumberArray(JSC::ExecState* exec, const JSValue& value)
This is fairly inefficient, and will allow non-array types, eg:
toCanvasArray({length:5}) will produce a CanvasFloatArray of 5 NaNs
> +{
> + if (!value.isObject())
> + return 0;
> +
> + JSObject* array = asObject(value);
> + int length = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> + RefPtr<CanvasNumberArray> numberArray =
CanvasNumberArray::create(length);
> + for (int i = 0; i < length; ++i) {
> + float value = array->get(exec, i).toFloat(exec);
Because this is a generic accessor function (and because toFloat may throw) you
need:
JSValue v = array->get(exec, i);
if (exec->hadException())
return 0;
float value = v.toFloat(exec);
if (exec->hadException())
return 0;
> + numberArray->data()[i] = value;
> + }
> +
> + return numberArray;
> +}
If you disallow non-array types (i'm not sure if you want to do this or not)
then you can devirtualise and remove the get
> +
> +JSValue JSCanvasRenderingContext3D::glBufferData(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned target = args.at(0).toInt32(exec);
> + unsigned usage = args.at(2).toInt32(exec);
These may throw -- and the values are untrusted (they may not even be present)
> + RefPtr<CanvasNumberArray> numberArray = toCanvasNumberArray(exec,
args.at(1));
numberArray may be null where is this handled?
> +
> +JSValue JSCanvasRenderingContext3D::glBufferSubData(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned target = args.at(0).toInt32(exec);
> + unsigned offset = args.at(1).toInt32(exec);
ditto
> +JSValue JSCanvasRenderingContext3D::glVertexAttrib(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned indx = args.at(0).toInt32(exec);
ditto
> +// void glVertexAttribPointer (in unsigned long indx, in long size, in
unsigned long type, in boolean normalized, in unsigned long stride, in
CanvasNumberArray array);
> +JSValue JSCanvasRenderingContext3D::glVertexAttribPointer(JSC::ExecState*
exec, JSC::ArgList const& args)
> +{
> + unsigned indx = args.at(0).toInt32(exec);
> + unsigned size = args.at(1).toInt32(exec);
> + unsigned type = args.at(2).toInt32(exec);
> + unsigned normalized = args.at(3).toInt32(exec);
> + bool stride = args.at(4).toBoolean(exec);
ditto
> +JSValue JSCanvasRenderingContext3D::glUniformMatrix(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned location = args.at(0).toInt32(exec);
> + unsigned count = args.at(1).toInt32(exec);
> + bool transpose = args.at(2).toBoolean(exec);
> +
ditto
> + if (!args.at(3).isObject())
> + return jsUndefined();
> +
> + WebKitCSSMatrix* matrix = toWebKitCSSMatrix(args.at(3));
> + if (matrix)
> + impl()->glUniformMatrix(location, transpose, matrix);
> + else {
> + JSObject* array = asObject(args.at(3));
Same as before are you sure we want to allow non-array arguments?
> +JSValue JSCanvasRenderingContext3D::glUniformf(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned location = args.at(0).toInt32(exec);
> +
> + switch(args.size()) {
> + case 2:
> + if (args.at(1).isObject()) {
> + RefPtr<CanvasNumberArray> numberArray =
toCanvasNumberArray(exec, args.at(1));
> + impl()->glUniform(location, numberArray.get());
> + }
> + else
> + impl()->glUniform(location, (float)
args.at(1).toNumber(exec));
toNumber may throw, should be using a C++ cast (woo! a style comment! that'll
annoy smfr :D)
> + break;
> + case 3:
> + impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> + (float) args.at(2).toNumber(exec));
> + break;
> + case 4:
> + impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> + (float) args.at(2).toNumber(exec),
> + (float) args.at(3).toNumber(exec));
> + break;
> + case 5:
> + impl()->glUniform(location, (float) args.at(1).toNumber(exec),
> + (float) args.at(2).toNumber(exec),
> + (float) args.at(3).toNumber(exec),
> + (float) args.at(4).toNumber(exec));
> + break;
You should probably throw an exception if the numbe rof arguments doesn't match
any expected count.
> +JSValue JSCanvasRenderingContext3D::glUniformi(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
Same as before, i really wish we have auto codegen for variadic arguments
> +JSValue JSCanvasRenderingContext3D::glDrawElements(JSC::ExecState* exec,
JSC::ArgList const& args)
> +{
> + unsigned mode = args.at(0).toInt32(exec);
> + unsigned type = args.at(1).toInt32(exec);
> +
> + unsigned int count = 0;
> +
> + // If the third param is not an object, it is a number, which is the
count.
> + // In this case if there is a 4th param, it is the offset. If there is
no
> + // 4th param, the offset is 0
> + if (!args.at(2).isObject()) {
> + count = args.at(2).toInt32(exec);
> + unsigned int offset = (args.size() > 3) ? args.at(3).toInt32(exec) :
0;
> + impl()->glDrawElements(mode, count, type, (void*) offset);
> + } else {
> + void* passedIndices = 0;
passedIndices should just be an OwnFastMallocPtr, and you should use fastMalloc
in place of malloc.
It would be better if you used tryFastMalloc, and null check the result so we
don't crash if malloc fails.
> + JSObject* array = asObject(args.at(2));
> + count = array->get(exec, Identifier(exec, "length")).toInt32(exec);
> +
> + if (type == GL_UNSIGNED_BYTE) {
> + unsigned char* indices = (unsigned char*) malloc(count *
sizeof(unsigned short));
fastmAlloc should be used instead of malloc, also should use c style cast.
> + } else if (type == GL_UNSIGNED_SHORT) {
> + unsigned short* indices = (unsigned short*) malloc(count *
sizeof(unsigned short));
ditto
> + } else
TypeError should be thrown on argument mismatch
> + return jsUndefined();
> +
> + impl()->glDrawElements(mode, count, type, passedIndices);
> + free(passedIndices);
OwnFastMallocPtr saves manually calling free
> + }
> +
> + return jsUndefined();
> +}
> +
> +// void texImage2DHTML(in unsigned long target, in unsigned long level, in
HTMLImageElement image);
> +JSValue JSCanvasRenderingContext3D::texImage2D(ExecState* exec, const
ArgList& args)
> +{
> + CanvasRenderingContext3D* context = impl();
> + unsigned target = args.at(0).toInt32(exec);
> + unsigned level = args.at(1).toInt32(exec);
exception and untrusted args again
> +
> + // The image parameter can be a <img> or <canvas> element.
> + JSValue value = args.at(2);
> + if (!value.isObject())
> + return throwError(exec, TypeError);
> + JSObject* o = asObject(value);
> +
> + ExceptionCode ec = 0;
> + if (o->inherits(&JSHTMLImageElement::s_info)) {
> + HTMLImageElement* imgElt =
static_cast<HTMLImageElement*>(static_cast<JSHTMLElement*>(o)->impl());
> + context->texImage2D(target, level, imgElt, ec);
> + } else if (o->inherits(&JSHTMLCanvasElement::s_info)) {
> + HTMLCanvasElement* canvas =
static_cast<HTMLCanvasElement*>(static_cast<JSHTMLElement*>(o)->impl());
> + context->texImage2D(target, level, canvas, ec);
<video> should also be allowed here
> +// void texSubImage2DHTML(in unsigned long target, in unsigned long level,
in unsigned long xoff, in unsigned long yoff, in unsigned long width, in
unsigned long height, in HTMLImageElement image);
> +JSValue JSCanvasRenderingContext3D::texSubImage2D(ExecState* exec, const
ArgList& args)
> +{
> + CanvasRenderingContext3D* context = impl();
> + unsigned target = args.at(0).toInt32(exec);
> + unsigned level = args.at(1).toInt32(exec);
> + unsigned xoff = args.at(2).toInt32(exec);
> + unsigned yoff = args.at(3).toInt32(exec);
> + unsigned width = args.at(4).toInt32(exec);
> + unsigned height = args.at(5).toInt32(exec);
ditto
> +
> + ExceptionCode ec = 0;
> + if (o->inherits(&JSHTMLImageElement::s_info)) {
> + HTMLImageElement* imgElt =
static_cast<HTMLImageElement*>(static_cast<JSHTMLElement*>(o)->impl());
> + context->texSubImage2D(target, level, xoff, yoff, width, height,
imgElt, ec);
> + } else if (o->inherits(&JSHTMLCanvasElement::s_info)) {
> + HTMLCanvasElement* canvas =
static_cast<HTMLCanvasElement*>(static_cast<JSHTMLElement*>(o)->impl());
> + context->texSubImage2D(target, level, xoff, yoff, width, height,
canvas, ec);
ditto
> +JSValue JSHTMLCanvasElement::getContext(JSC::ExecState* exec, JSC::ArgList
const& args)
> +{
> + HTMLCanvasElement* imp = static_cast<HTMLCanvasElement*>(impl());
> + const UString& contextId = args.at(0).toString(exec);
> +
> + JSValue result = jsUndefined();
> +
> + if (contextId == "2d")
> + result = toJS(exec,
WTF::getPtr(reinterpret_cast<CanvasRenderingContext2D*>(imp->getContext(context
Id))));
> +#if ENABLE(3D_CANVAS)
> + else if (contextId == "webkit-3d")
> + result = toJS(exec,
WTF::getPtr(reinterpret_cast<CanvasRenderingContext3D*>(imp->getContext(context
Id))));
> +#endif
> +
> + return result;
> +}
This makes me sad, we should probably make toJS(CanvasRenderingContext*) do the
coercion there, rather than making this a custom binding.
> Index: WebCore/html/CanvasByteArray.cpp
> + String CanvasByteArray::toString() const
> + {
> + String s = "[ ";
> + for (size_t i = 0; i < m_data.size(); ++i) {
> + if (i != 0)
> + s += ", ";
> + s += String::number(m_data[i]);
> + }
> +
> + s += " ]";
> + return s;
> + }
> + [DontEnum] DOMString toString();
Most other DOM objects do not have toString implementations, so this seems
unusual. Its syntax also does not match the syntax standard array toString, i
would suggest removing this.
> + virtual void _deleteObject(Platform3DObject);
why the _?
> + String CanvasNumberArray::toString() const
> + {
> + String s = "[ ";
> + for (size_t i = 0; i < m_data.size(); ++i) {
> + if (i != 0)
> + s += ", ";
> + s += String::number(m_data[i]);
> + }
> +
> + s += " ]";
> + return s;
> + }
> + [DontEnum] DOMString toString();
Same again for CanvasNumberArray. I have a feeling that these are sufficiently
similar that we probably jut want
template <typename T> class CanvasArray {
...
};
class CanvasByteArray : CanvasArray<uint8> { ... }
class CanvasNumberArray : CanvasArray<float> { ... }
> +
> + /* ClearBufferMask */
> + const unsigned int GL_DEPTH_BUFFER_BIT = 0x00000100;
...
> + const unsigned int GL_MAX_RENDERBUFFER_SIZE = 0x84E8;
I thought that we had decided that most of these constants would not be present
as they are unused and unnecessary?
> + void glActiveTexture (in unsigned long texture);
Textures aren't identified by numeric idents so why is this here? Also why the
gl prefixes this doesn't seem to match the spec
> +}
> +
> +-(void)drawInCGLContext:(CGLContextObj)glContext
pixelFormat:(CGLPixelFormatObj)pixelFormat
forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp
*)timeStamp
> +{
> + CGRect frame = [self frame];
> +
> + // draw the FBO into the layer
> + glViewport(0, 0, frame.size.width, frame.size.height);
> + glMatrixMode(GL_PROJECTION);
> + glLoadIdentity();
> + glOrtho(-1, 1, -1, 1, -1, 1);
> + glMatrixMode(GL_MODELVIEW);
> + glLoadIdentity();
> +
> + glEnable(GL_TEXTURE_2D);
> + glBindTexture(GL_TEXTURE_2D, m_texture);
> +
> + glBegin(GL_TRIANGLE_FAN);
> + glTexCoord2f(0, 0);
> + glVertex2f(-1, -1);
> + glTexCoord2f(1, 0);
> + glVertex2f(1, -1);
> + glTexCoord2f(1, 1);
> + glVertex2f(1, 1);
> + glTexCoord2f(0, 1);
> + glVertex2f(-1, 1);
> + glEnd();
> +
> + glBindTexture(GL_TEXTURE_2D, 0);
> + glDisable(GL_TEXTURE_2D);
We draw the fbo with a triangle fan? say what?
> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================
> --- WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp (revision 0)
> +++ WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp (revision 0)
> @@ -0,0 +1,1570 @@
> +/*
> + * Copyright (C) 2003, 2004, 2005, 2006, 2007 Apple Inc. All rights
reserved.
2009
There are plenty of C-style casts in the remaining code and uses of malloc/free
instead of fastMalloc/fastFree
More information about the webkit-reviews
mailing list