Bug 31562 – Implement all cases of texImage2d and texSubImage2d

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

All variants that take an element (or ImageData) as the last param should not have width and height parameters.

[Chris Marrin]

In addition to getting rid of the errant params, the texSubImage2D calls are not yet implemented. Might as well do it all at once

[Chris Marrin]

Some versions of texImage2D are not implemented and no versions of texSubImage2D are implemented

[Chris Marrin]

Created attachment 44995 [details]
Patch with test case

This is everything except video textures. I will do that in a separate patch to keep it smaller

[WebKit Review Bot]

Attachment 44995 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:409:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1

[Simon Fraser (smfr)]

Comment on attachment 44995 [details]
Patch with test case

> Index: WebCore/ChangeLog
> ===================================================================

> +        This implements all cases except HTMLVideoElement. That will come in
> +        a subsequent patch. It also does not implement the flipY or premultiplyAlpha 
> +        flags which is covered in https://bugs.webkit.org/show_bug.cgi?id=32620. 
> +        The code also does not check for buffer overruns. That is covered in 
> +        https://bugs.webkit.org/show_bug.cgi?id=32619.

I don't think your changelog needs to explain what isn't done yet, but it should say more about
what this particular set of changes is doing.



>  JSValue JSWebGLRenderingContext::texImage2D(ExecState* exec, const ArgList& args)
>  { 
> -    if (args.size() < 3)
> +    if (args.size() < 3 || args.size() > 9)
>          return throwError(exec, SyntaxError);
> -
> +    
>      ExceptionCode ec = 0;
> +    
>      WebGLRenderingContext* context = static_cast<WebGLRenderingContext*>(impl());    
>      unsigned target = args.at(0).toInt32(exec);
>      if (exec->hadException())    
>          return jsUndefined();
> -        
> +    
>      unsigned level = args.at(1).toInt32(exec);
>      if (exec->hadException())    
>          return jsUndefined();
>      
...
> -            
> +        
> +        // This must be the WebGLArray case
>          unsigned internalformat = args.at(2).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();
> -
> +        
>          unsigned width = args.at(3).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();
> -
> +        
>          unsigned height = args.at(4).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();
> -
> +        
>          unsigned border = args.at(5).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();
> -
> +        
>          unsigned format = args.at(6).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();
> -
> +        
>          unsigned type = args.at(7).toInt32(exec);
>          if (exec->hadException())    
>              return jsUndefined();

Lots of whitespace changes here.

[Darin Adler]

Comment on attachment 44995 [details]
Patch with test case

> -    if (args.size() < 3)
> +    if (args.size() < 3 || args.size() > 9)
>          return throwError(exec, SyntaxError);

For most JavaScript functions we allow and ignore extra arguments, to match the behavior of functions built into the JavaScript language itself. Is there a reason not to do that for this function? Is there a test suite that checks for this behavior?

It might be a case of matching Firefox. They seem not to tolerate extra arguments. Maybe we should change our approach?

> +        if (!value.isObject())
> +            return throwError(exec, SyntaxError);

Is SyntaxError the right exception for this? What about TYPE_MIMSATCH_ERR? Is there a test that covers it?

> +        bool flipY = (args.size() > 3) ? args.at(3).toBoolean(exec) : false;
> +        bool premultiplyAlpha = (args.size() > 4) ? args.at(4).toBoolean(exec) : false;

There's no need to check args.size here. Extra arguments act as undefined, and undefined converts to false.

> +        if (o->inherits(&JSImageData::s_info)) {
> +            ImageData* obj = static_cast<ImageData*>(static_cast<JSImageData*>(o)->impl());
> +            context->texImage2D(target, level, obj, flipY, premultiplyAlpha, ec);
> +        } else if (o->inherits(&JSHTMLImageElement::s_info)) {
> +            HTMLImageElement* obj = static_cast<HTMLImageElement*>(static_cast<JSHTMLImageElement*>(o)->impl());
> +            context->texImage2D(target, level, obj, flipY, premultiplyAlpha, ec);
> +        } else if (o->inherits(&JSHTMLCanvasElement::s_info)) {
> +            HTMLCanvasElement* obj = static_cast<HTMLCanvasElement*>(static_cast<JSHTMLCanvasElement*>(o)->impl());
> +            context->texImage2D(target, level, obj, flipY, premultiplyAlpha, ec);
> +        } else if (o->inherits(&JSHTMLVideoElement::s_info)) {
> +            HTMLVideoElement* obj = static_cast<HTMLVideoElement*>(static_cast<JSHTMLVideoElement*>(o)->impl());
> +            context->texImage2D(target, level, obj, flipY, premultiplyAlpha, ec);
> +        } else
> +            ec = TYPE_MISMATCH_ERR;

I would name these local variables things like "data" and "element" rather then "obj".

> +        JSValue value = args.at(8);
> +            
> +        // For this case passing 0 (for a null array) is allowed
> +        if (!value.isObject())

But !isObject() is not the same as checking for 0. There are many types of JavaScript values that are not objects. This includes undefined, null, numbers, strings. So we probably need a more specific check here and some test cases.

> +    if (ec != 0)
> +        setDOMException(exec, ec);

You don't need to check for 0 before calling setDOMException -- it's cleaner to just call it unconditionally.

> Index: WebCore/html/canvas/CanvasPixelArray.h
> ===================================================================
> --- WebCore/html/canvas/CanvasPixelArray.h	(revision 52204)
> +++ WebCore/html/canvas/CanvasPixelArray.h	(working copy)
> @@ -61,7 +61,8 @@ namespace WebCore {
>  
>          unsigned char get(unsigned index) const
>          {
> -            return m_data->get(index);
> +            unsigned char c;
> +            return m_data->get(index, c) ? c : 0;
>          }

What is this change for? The change log does not make it clear what it fixes. Is there a test case for this?

I stopped reviewing because I saw Simon did.

[Kenneth Russell]

There was already ongoing work in https://bugs.webkit.org/show_bug.cgi?id=31493 to implement this.

[Chris Marrin]

Ken, what do you think is the right way to go forward? I can land my patch and let Rachel add any work she is doing to that. That is probably best since my JS binding logic for the ImageData case is pretty wrapped up in the others. 

I will wait for your response before landing anything.

[Simon Fraser (smfr)]

Comment on attachment 44995 [details]
Patch with test case

Also, consider using OwnFastMallocPtr, rather than passing back fastmalloc'd pointers that need to be freed by the caller.

[Kenneth Russell]

(In reply to comment #8)
> Ken, what do you think is the right way to go forward? I can land my patch and
> let Rachel add any work she is doing to that. That is probably best since my JS
> binding logic for the ImageData case is pretty wrapped up in the others. 
> 
> I will wait for your response before landing anything.

I'd like to propose that Rachel pick up your patch and merge it into her current work. As it stands your patch will break the Chromium build since WebKit/chromium/src/GraphicsContext3D.cpp wasn't updated, and neither were the V8 bindings. Rachel will try to get this merged and re-uploaded by the end of tomorrow, and failing that I will help and get the combined patch uploaded by COB Friday. Does that sound acceptable?

[Rachel Petterson]

Created attachment 45033 [details]
Updated patch including Chris Marrin's changes and changes for chromium.

[Rachel Petterson]

I've put up a combined patch which includes Chris's changes and also works with Chromium.

[Chris Marrin]

Thanks Rachel. I'll take your additions, add in the issues from Simon, Darin and the style bot and land the patch. I may have to hold off till tomorrow though.

[Eric Seidel (no email)]

Wondering if this ever got landed?

[Chris Marrin]

*** Bug 31493 has been marked as a duplicate of this bug. ***

[Chris Marrin]

(In reply to comment #6)
> (From update of attachment 44995 [details])
> > -    if (args.size() < 3)
> > +    if (args.size() < 3 || args.size() > 9)
> >          return throwError(exec, SyntaxError);
> 
> For most JavaScript functions we allow and ignore extra arguments, to match the
> behavior of functions built into the JavaScript language itself. Is there a
> reason not to do that for this function? Is there a test suite that checks for
> this behavior?
> 
> It might be a case of matching Firefox. They seem not to tolerate extra
> arguments. Maybe we should change our approach?

I've done it this way because I was matching some other code. I think it's reasonable to do it this way, at least in this case. And yes, the testcase I'm checking in tests to see that an exception is thrown with a bad number of params.

> 
> > +        if (!value.isObject())
> > +            return throwError(exec, SyntaxError);
> 
> Is SyntaxError the right exception for this? What about TYPE_MIMSATCH_ERR? Is
> there a test that covers it?

There is a test that checks for the exception, but it doesn't check the type. I've changed it to a TypeError.
 
> > +        JSValue value = args.at(8);
> > +            
> > +        // For this case passing 0 (for a null array) is allowed
> > +        if (!value.isObject())
> 
> But !isObject() is not the same as checking for 0. There are many types of
> JavaScript values that are not objects. This includes undefined, null, numbers,
> strings. So we probably need a more specific check here and some test cases.

I've changed it to follow this clause on value.isNull(), otherwise any other non-object type throws TypeError.

> 
> > Index: WebCore/html/canvas/CanvasPixelArray.h
> > ===================================================================
> > --- WebCore/html/canvas/CanvasPixelArray.h	(revision 52204)
> > +++ WebCore/html/canvas/CanvasPixelArray.h	(working copy)
> > @@ -61,7 +61,8 @@ namespace WebCore {
> >  
> >          unsigned char get(unsigned index) const
> >          {
> > -            return m_data->get(index);
> > +            unsigned char c;
> > +            return m_data->get(index, c) ? c : 0;
> >          }
> 
> What is this change for? The change log does not make it clear what it fixes.
> Is there a test case for this?

That is cruft and has been removed from the patch

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/52821

[Chris Marrin]

Testcases for patch landed in http://trac.webkit.org/changeset/52834

[Dimitri Glazkov (Google)]

This broke Chromium:

http://build.chromium.org/buildbot/waterfall.fyi/waterfall?builder=Webkit%20(webkit.org)

Please fix?

[Eric Seidel (no email)]

It passed the Chromium EWS: http://webkit-commit-queue.appspot.com/patch/44995, but that was 2 weeks ago.

[Kenneth Russell]

The problem is that this patch was committed by hand rather than via the commit queue, and the Chromium half of the patch was left out.

We are going to fix this under a new bug. I'm re-marking this as Resolved, Fixed.