Bug 43218 – [chromium] Make the GLES2 texture map generic and teach ImageSkia and ImageBufferSkia about GLES2

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Make the GLES2 texture map generic and teach ImageSkia and ImageBufferSkia about GLES2

[James Robinson]

Created attachment 63005 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 63005 [details]
Patch

WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476
 +  GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height)
ewwww... it is really easy to have casting errors when passing class pointers
to a function expecting a void*.  this can be especially nasty when multiple
inheritance is involved.  we've been burned by things like this before.
what do you think about defining a common base class that all types that
can be passed here must implement?  or, how about a struct/union that has
member pointers for each supported pointer type?

WebCore/platform/graphics/chromium/GLES2Canvas.h:52
 +  typedef HashMap<const void *, RefPtr<GLES2Texture> > TextureHashMap;
nit: no space between "void" and "*".  there are several others like this one.

otherwise, this LGTM

[Darin Fisher (:fishd, Google)]

(In reply to comment #2)
> (From update of attachment 63005 [details])
> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476
>  +  GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height)
> ewwww... it is really easy to have casting errors when passing class pointers
> to a function expecting a void*.  this can be especially nasty when multiple
> inheritance is involved.  we've been burned by things like this before.
> what do you think about defining a common base class that all types that
> can be passed here must implement?  or, how about a struct/union that has
> member pointers for each supported pointer type?

or even simpler:  provide overrides of createTexture and getTexture for each supported input pointer type.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 63005 [details])
> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:476
>  +  GLES2Texture* GLES2Canvas::createTexture(const void* ptr, GLES2Texture::Format format, int width, int height)
> ewwww... it is really easy to have casting errors when passing class pointers
> to a function expecting a void*.  this can be especially nasty when multiple
> inheritance is involved.  we've been burned by things like this before.
> what do you think about defining a common base class that all types that
> can be passed here must implement?  or, how about a struct/union that has
> member pointers for each supported pointer type?

Since this change is my fault, I'll try to justify it:

The texture hash map only uses the pointer as an opaque key.  It's never dereferenced, nor is it cast to anything else.  So there's no danger of casting errors.  The only danger of a caller passing in a pointer of the wrong type would be that no texture would be found, or a newly-created texture would be associated with the wrong pointer.  But errors like that would be quickly noticed due to bad performance.

The original reason was for constness:  there was a const_cast in ImageSkia.cpp I wanted to get rid of, and the compiler was giving me grief with "const NativeImagePtr" as the hash key.  I think I would have had to define a NativeImagePtrConst for each platform.

Another reason for making it opaque is that it would discourage anyone from making any member/fn calls dependent on the platform-specific NativeImagePtr.  I was also concerned that NativeImagePtr might not work in a hash map on all platforms.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
...
> Since this change is my fault, I'll try to justify it:
> 
> The texture hash map only uses the pointer as an opaque key.  It's never dereferenced, nor is it cast to anything else.  So there's no danger of casting errors.  The only danger of a caller passing in a pointer of the wrong type would be that no texture would be found, or a newly-created texture would be associated with the wrong pointer.  But errors like that would be quickly noticed due to bad performance.

Hmm... that's exactly the kind of thing I was hoping we could avoid.  The result is not a crash implicating a certain piece of code, instead it is just degraded performance.  In the past, code like this led to memory leaks that were hard to track down.  I'm just saying that the pattern is to be avoided.


> The original reason was for constness:  there was a const_cast in ImageSkia.cpp I wanted to get rid of, and the compiler was giving me grief with "const NativeImagePtr" as the hash key.  I think I would have had to define a NativeImagePtrConst for each platform.
> 
> Another reason for making it opaque is that it would discourage anyone from making any member/fn calls dependent on the platform-specific NativeImagePtr.  I was also concerned that NativeImagePtr might not work in a hash map on all platforms.

I see.  I did not realize that there were other reasons to hide the fact that it was a NativeImagePtr from this code.

[Darin Fisher (:fishd, Google)]

Comment on attachment 63005 [details]
Patch

R=me but please consider alternatives that avoid risky void*.

CQ- for the style issues mentioned earlier.

[James Robinson]

Created attachment 63100 [details]
Patch

[James Robinson]

As it turns out NativeImagePtr works fine as a hash key, so switched back to using that.  The only possible issue is that on WinCE NativeImagePtr is typedef'd to RefPtr instead of a raw pointer.  However, I can't imagine this code ever running on WinCE anyway so I think it's fine to ignore that for now.

[Darin Fisher (:fishd, Google)]

Comment on attachment 63100 [details]
Patch

OK, cool.  R=me

[Stephen White]

(In reply to comment #8)
> As it turns out NativeImagePtr works fine as a hash key, so switched back to using that.  The only possible issue is that on WinCE NativeImagePtr is typedef'd to RefPtr instead of a raw pointer.  However, I can't imagine this code ever running on WinCE anyway so I think it's fine to ignore that for now.

Great.  My bad -- I was led astray by the const NativeImageSkia* in ImageSkia (which as James pointed out to me, is unnecessary -- non-const works fine here).

Just FYI, the problem I had with HashMap turned out to be that "const NativeImagePtr" actually maps to "NativeImageSkia* const", not "const NativeImageSkia*", and the DefaultHash function used by HashMap has no specialization for T * const.  This could be fixed many ways, but keeping it non-const is probably the easiest.

[James Robinson]

Comment on attachment 63100 [details]
Patch

Clearing flags on attachment: 63100

Committed r64374: <http://trac.webkit.org/changeset/64374>

[James Robinson]

All reviewed patches have been landed.  Closing bug.