Bug 31562 - Implement all cases of texImage2d and texSubImage2d
Summary: Implement all cases of texImage2d and texSubImage2d
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
: 31493 (view as bug list)
Depends on:
Blocks: 32040 33241
  Show dependency treegraph
 
Reported: 2009-11-16 13:42 PST by Chris Marrin
Modified: 2010-01-05 18:36 PST (History)
6 users (show)

See Also:


Attachments
Patch with test case (46.23 KB, patch)
2009-12-16 10:57 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff
Updated patch including Chris Marrin's changes and changes for chromium. (68.79 KB, patch)
2009-12-16 20:51 PST, Rachel Petterson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-11-16 13:42:38 PST
All variants that take an element (or ImageData) as the last param should not have width and height parameters.
Comment 1 Chris Marrin 2009-12-01 14:26:13 PST
In addition to getting rid of the errant params, the texSubImage2D calls are not yet implemented. Might as well do it all at once
Comment 2 Chris Marrin 2009-12-01 16:05:44 PST
Some versions of texImage2D are not implemented and no versions of texSubImage2D are implemented
Comment 3 Chris Marrin 2009-12-16 10:57:40 PST
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
Comment 4 WebKit Review Bot 2009-12-16 11:00:01 PST
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
Comment 5 Simon Fraser (smfr) 2009-12-16 11:12:42 PST
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.
Comment 6 Darin Adler 2009-12-16 11:15:09 PST
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.
Comment 7 Kenneth Russell 2009-12-16 11:16:46 PST
There was already ongoing work in https://bugs.webkit.org/show_bug.cgi?id=31493 to implement this.
Comment 8 Chris Marrin 2009-12-16 11:23:54 PST
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.
Comment 9 Simon Fraser (smfr) 2009-12-16 11:26:45 PST
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.
Comment 10 Kenneth Russell 2009-12-16 11:55:59 PST
(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?
Comment 11 Rachel Petterson 2009-12-16 20:51:12 PST
Created attachment 45033 [details]
Updated patch including Chris Marrin's changes and changes for chromium.
Comment 12 Rachel Petterson 2009-12-16 20:52:23 PST
I've put up a combined patch which includes Chris's changes and also works with Chromium.
Comment 13 Chris Marrin 2009-12-17 11:08:45 PST
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.
Comment 14 Eric Seidel (no email) 2009-12-28 23:12:00 PST
Wondering if this ever got landed?
Comment 15 Chris Marrin 2010-01-05 12:07:56 PST
*** Bug 31493 has been marked as a duplicate of this bug. ***
Comment 16 Chris Marrin 2010-01-05 12:34:53 PST
(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
Comment 17 Chris Marrin 2010-01-05 12:41:44 PST
Landed in http://trac.webkit.org/changeset/52821
Comment 18 Chris Marrin 2010-01-05 15:43:11 PST
Testcases for patch landed in http://trac.webkit.org/changeset/52834
Comment 19 Dimitri Glazkov (Google) 2010-01-05 16:41:08 PST
This broke Chromium:

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

Please fix?
Comment 20 Eric Seidel (no email) 2010-01-05 16:56:23 PST
It passed the Chromium EWS: http://webkit-commit-queue.appspot.com/patch/44995, but that was 2 weeks ago.
Comment 21 Kenneth Russell 2010-01-05 16:58:32 PST
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.