Bug 34458 – Refactor texImage2D and texSubImage2D taking Image to use common code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The Chromium port of WebGL does not implement texSubImage2D taking an Image as parameter.

[Kenneth Russell]

It turns out that with a small amount of additional refactoring, both the Safari and Chromium WebGL implementations can use the same code for texImage2D and texSubImage2D taking Images, fixing three problems at once. Changing the synopsis to reflect this.

[Kenneth Russell]

Created attachment 48670 [details]
Patch

Please see ChangeLog for detailed description of patch.

Note to reviewers: I originally added multiple GraphicsContext3DUtil classes (GraphicsContext3DUtilMac, GraphicsContext3DUtilSkia) but this added more complexity than it was worth. I hope the few #ifdefs in GraphicsContext3DUtil are acceptable.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:94:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:117:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:138:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/GraphicsContext3DUtil.cpp:227:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/GraphicsContext3DUtil.h:88:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/GraphicsContext3DUtil.h:90:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 8


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 48670 [details]
Patch

This patch accidentally contained a file I didn't mean to modify. Obsoleting it.

[Kenneth Russell]

Created attachment 48672 [details]
Patch

[Chris Marrin]

I like the idea behind this. But it seems like it might be better to refactor GraphicsContext3D as a base class with these functions included in it. Then it can be subclassed for the Mac, and Chromium versions. I haven't looked closely enough to know if this would have complications. But it would allow us to further add common code down the road.

[Kenneth Russell]

(In reply to comment #6)
> I like the idea behind this. But it seems like it might be better to refactor
> GraphicsContext3D as a base class with these functions included in it. Then it
> can be subclassed for the Mac, and Chromium versions. I haven't looked closely
> enough to know if this would have complications. But it would allow us to
> further add common code down the road.

I agree that it would generally be better to express the GraphicsContext3D API as a set of virtual functions. However, this will be a very large change and one which should probably be separate from the addition of any new functionality or other refactorings such as this one. I think we should add the utility class for now.

[Oliver Hunt]

(In reply to comment #7)
> (In reply to comment #6)
> > I like the idea behind this. But it seems like it might be better to refactor
> > GraphicsContext3D as a base class with these functions included in it. Then it
> > can be subclassed for the Mac, and Chromium versions. I haven't looked closely
> > enough to know if this would have complications. But it would allow us to
> > further add common code down the road.
> 
> I agree that it would generally be better to express the GraphicsContext3D API
> as a set of virtual functions. However, this will be a very large change and
> one which should probably be separate from the addition of any new
> functionality or other refactorings such as this one. I think we should add the
> utility class for now.

Why would these be virtual functions?  That is not the model that we use for platform specific behaviour in WebKit.  Platform specifics are compile time based, not runtime so there is no reason for them to ever be implemented in terms of polymorphism.

The correct thing to do is to have GraphicsContext3D.cpp that contains the shared implementations, and then GraphicsContext3DPlatformName.cpp for platform specific code.  We already do this for GraphicsContext and numerous other classes.

[Chris Marrin]

Yeah, this is my bad. I was actually suggesting a virtual API. As Oliver points out that's the wrong way to do these platform specific classes. So I agree with Oliver, just putting these common functions into a GraphicsContext3D.cpp would work just fine. Then you'd just have 2 different .cpp file providing different functions for the common GraphicsContext3D class.

[Kenneth Russell]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I like the idea behind this. But it seems like it might be better to refactor
> > > GraphicsContext3D as a base class with these functions included in it. Then it
> > > can be subclassed for the Mac, and Chromium versions. I haven't looked closely
> > > enough to know if this would have complications. But it would allow us to
> > > further add common code down the road.
> > 
> > I agree that it would generally be better to express the GraphicsContext3D API
> > as a set of virtual functions. However, this will be a very large change and
> > one which should probably be separate from the addition of any new
> > functionality or other refactorings such as this one. I think we should add the
> > utility class for now.
> 
> Why would these be virtual functions?  That is not the model that we use for
> platform specific behaviour in WebKit.  Platform specifics are compile time
> based, not runtime so there is no reason for them to ever be implemented in
> terms of polymorphism.
> 
> The correct thing to do is to have GraphicsContext3D.cpp that contains the
> shared implementations, and then GraphicsContext3DPlatformName.cpp for platform
> specific code.  We already do this for GraphicsContext and numerous other
> classes.

OK, I understand your point about the virtual functions.

I'll work on adding a GraphicsContext3D.cpp in platform/graphics/ containing
the common functions, and separating out the CG- and Skia-specific portions
into new .cpp files as well.

[Kenneth Russell]

Created attachment 48685 [details]
Revised patch

Based on review feedback, placed entry points on GraphicsContext3D class. Common code is in platform/graphics/GraphicsContext3D.cpp; platform-specific entry points in platform/graphics/cg/GraphicsContext3DCG.cpp and platform/graphics/skia/GraphicsContext3DSkia.cpp.

Retested Safari on Mac OS X and Chromium on Mac OS X and Windows.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 48685 [details]
Revised patch

r=me

[Kenneth Russell]

Created attachment 48688 [details]
Revised patch

Thanks for earlier review.

Revised patch fixing minor style issue in GraphicsContext3DCG.cpp.

[Oliver Hunt]

Comment on attachment 48688 [details]
Revised patch

Whoops, i forgot to comment on the comments originally -- we really don't comment in this manner, in general if your code needs that degree of commentary the code should probably be reworked.

That said all of this code is simple, and so this commentary is simply unnecessary, and merely makes it difficult to read the code proper.  Basically there shouldn't be more than about two sentences, tops, for the getImageData or processImageData definitions.

Anyhoo, while reading through the patch 

I also have some concerns about the CG implementation of getImageData, I don't believe you should be stripping colour space information away (which is essentially what you're currently doing).  The target context should simply be set to either sRGB or LinearRGB (if this is not specified in the WebGL spec, it should be) so the there's a consistent colour space.  I think as a byproduct of such logic it becomes sensible to simply use as ImageBuffer to do the image to byte conversion in which case that can also be cross paltform, and it also saves us from having a separate implementation of [un]premultiplying.

Sorry for both the delay of this response, and for the substantial change in review :-/

[Kenneth Russell]

(In reply to comment #15)
> (From update of attachment 48688 [details])
> Whoops, i forgot to comment on the comments originally -- we really don't
> comment in this manner, in general if your code needs that degree of commentary
> the code should probably be reworked.
> 
> That said all of this code is simple, and so this commentary is simply
> unnecessary, and merely makes it difficult to read the code proper.  Basically
> there shouldn't be more than about two sentences, tops, for the getImageData or
> processImageData definitions.

I believe the comments are appropriate in length and that all of the description is warranted. Desktop OpenGL, for example, does not require that the image rows are tightly packed.

> Anyhoo, while reading through the patch 
> 
> I also have some concerns about the CG implementation of getImageData, I don't
> believe you should be stripping colour space information away (which is
> essentially what you're currently doing).  The target context should simply be
> set to either sRGB or LinearRGB (if this is not specified in the WebGL spec, it
> should be) so the there's a consistent colour space.  I think as a byproduct of
> such logic it becomes sensible to simply use as ImageBuffer to do the image to
> byte conversion in which case that can also be cross paltform, and it also
> saves us from having a separate implementation of [un]premultiplying.

I spent quite a bit of time making the test case work correctly on Mac OS X. Specifying a destination color space of kCGColorSpaceGenericRGB or kCGColorSpaceGenericRGBLinear caused the input image's colors (which are (255, 0, 0) and (0, 255, 0)) to be distorted. The only solution I could find which did not change the input colors was to use the image's color space as the color space for the destination bitmap context, which is what the original code used to do.

I don't see any Image -> ImageBuffer conversion in the WebKit API.

I plan to continue working on this code, in particular to go back to the SharedBuffer containing the compressed image data and run an ImageDecoder on it to get back untouched RGBA data. However, I strongly believe that the refactoring I have done here is a good step forward as it brings Safari and Chromium to parity in texture uploading, and I believe it should be checked in.

> Sorry for both the delay of this response, and for the substantial change in
> review :-/

Please reconsider.

[Oliver Hunt]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 48688 [details] [details])
> > Whoops, i forgot to comment on the comments originally -- we really don't
> > comment in this manner, in general if your code needs that degree of commentary
> > the code should probably be reworked.
> > 
> > That said all of this code is simple, and so this commentary is simply
> > unnecessary, and merely makes it difficult to read the code proper.  Basically
> > there shouldn't be more than about two sentences, tops, for the getImageData or
> > processImageData definitions.
> 
> I believe the comments are appropriate in length and that all of the
> description is warranted. Desktop OpenGL, for example, does not require that
> the image rows are tightly packed.

Look through the webcore codebase you will see that there are places where the code is substantially more complex yet does not have this level of commentary.  The comments are currently excessive.

> 
> > Anyhoo, while reading through the patch 
> > 
> > I also have some concerns about the CG implementation of getImageData, I don't
> > believe you should be stripping colour space information away (which is
> > essentially what you're currently doing).  The target context should simply be
> > set to either sRGB or LinearRGB (if this is not specified in the WebGL spec, it
> > should be) so the there's a consistent colour space.  I think as a byproduct of
> > such logic it becomes sensible to simply use as ImageBuffer to do the image to
> > byte conversion in which case that can also be cross paltform, and it also
> > saves us from having a separate implementation of [un]premultiplying.
> 
> I spent quite a bit of time making the test case work correctly on Mac OS X.
> Specifying a destination color space of kCGColorSpaceGenericRGB or
> kCGColorSpaceGenericRGBLinear caused the input image's colors (which are (255,
> 0, 0) and (0, 255, 0)) to be distorted. The only solution I could find which
> did not change the input colors was to use the image's color space as the color
> space for the destination bitmap context, which is what the original code used
> to do.
> 
> I don't see any Image -> ImageBuffer conversion in the WebKit API.

ImageBuffer gives a generic bitmap context to draw the image to, basically what you're doing in the CG code path, and that could be used as the default code path -- conceivably with a !PLATFOMR(SKIA) guard if it's sufficiently fast to load the raw image data in skia, although i would still be concerned about colour space conversion -- if the spec does not currently specify image colour space it should, SVG uses LinearRGB but i suspect sRGB would be more efficient as in the general case i think that will save conversion (i believe the assumption is image without colour profile == srgb)

> 
> I plan to continue working on this code, in particular to go back to the
> SharedBuffer containing the compressed image data and run an ImageDecoder on it
> to get back untouched RGBA data. However, I strongly believe that the
> refactoring I have done here is a good step forward as it brings Safari and
> Chromium to parity in texture uploading, and I believe it should be checked in.

I am unsure that getting the untouched data is actually what is wanted, as i said above we will want to ensure a uniform colour space

> 
> > Sorry for both the delay of this response, and for the substantial change in
> > review :-/
> 
> Please reconsider.

[Kenneth Russell]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 48688 [details] [details] [details])
> > > Whoops, i forgot to comment on the comments originally -- we really don't
> > > comment in this manner, in general if your code needs that degree of commentary
> > > the code should probably be reworked.
> > > 
> > > That said all of this code is simple, and so this commentary is simply
> > > unnecessary, and merely makes it difficult to read the code proper.  Basically
> > > there shouldn't be more than about two sentences, tops, for the getImageData or
> > > processImageData definitions.
> > 
> > I believe the comments are appropriate in length and that all of the
> > description is warranted. Desktop OpenGL, for example, does not require that
> > the image rows are tightly packed.
> 
> Look through the webcore codebase you will see that there are places where the
> code is substantially more complex yet does not have this level of commentary. 
> The comments are currently excessive.

I'll trim down the comments.

> > 
> > > Anyhoo, while reading through the patch 
> > > 
> > > I also have some concerns about the CG implementation of getImageData, I don't
> > > believe you should be stripping colour space information away (which is
> > > essentially what you're currently doing).  The target context should simply be
> > > set to either sRGB or LinearRGB (if this is not specified in the WebGL spec, it
> > > should be) so the there's a consistent colour space.  I think as a byproduct of
> > > such logic it becomes sensible to simply use as ImageBuffer to do the image to
> > > byte conversion in which case that can also be cross paltform, and it also
> > > saves us from having a separate implementation of [un]premultiplying.
> > 
> > I spent quite a bit of time making the test case work correctly on Mac OS X.
> > Specifying a destination color space of kCGColorSpaceGenericRGB or
> > kCGColorSpaceGenericRGBLinear caused the input image's colors (which are (255,
> > 0, 0) and (0, 255, 0)) to be distorted. The only solution I could find which
> > did not change the input colors was to use the image's color space as the color
> > space for the destination bitmap context, which is what the original code used
> > to do.
> > 
> > I don't see any Image -> ImageBuffer conversion in the WebKit API.
> 
> ImageBuffer gives a generic bitmap context to draw the image to, basically what
> you're doing in the CG code path, and that could be used as the default code
> path -- conceivably with a !PLATFOMR(SKIA) guard if it's sufficiently fast to
> load the raw image data in skia, although i would still be concerned about
> colour space conversion -- if the spec does not currently specify image colour
> space it should, SVG uses LinearRGB but i suspect sRGB would be more efficient
> as in the general case i think that will save conversion (i believe the
> assumption is image without colour profile == srgb)
> 
> > 
> > I plan to continue working on this code, in particular to go back to the
> > SharedBuffer containing the compressed image data and run an ImageDecoder on it
> > to get back untouched RGBA data. However, I strongly believe that the
> > refactoring I have done here is a good step forward as it brings Safari and
> > Chromium to parity in texture uploading, and I believe it should be checked in.
> 
> I am unsure that getting the untouched data is actually what is wanted, as i
> said above we will want to ensure a uniform colour space

The WebGL working group has discussed this at length and it is a requirement that the incoming image data be untouched. In 3D algorithms it is common to place non-color data into color and alpha channels of images -- for example, normal maps, or high and low bytes of 16-bit per channel environment maps. Performing color correction, or automatically premultiplying the alpha channel into the color channels, will cause incorrect results.

This code is an intermediate step toward that goal but I would like to put it in place before making further changes.

> > > Sorry for both the delay of this response, and for the substantial change in
> > > review :-/
> > 
> > Please reconsider.

[Kenneth Russell]

Created attachment 48836 [details]
Revised patch

Trimmed comments for new public APIs in GraphicsContext3D.

[Kenneth Russell]

Could I please get a review of this patch? I am blocked on other bug fixes waiting for this to land.

[Oliver Hunt]

> The WebGL working group has discussed this at length and it is a requirement
> that the incoming image data be untouched. In 3D algorithms it is common to
> place non-color data into color and alpha channels of images -- for example,
> normal maps, or high and low bytes of 16-bit per channel environment maps.
> Performing color correction, or automatically premultiplying the alpha channel
> into the color channels, will cause incorrect results.

If the image is intended to be data then it should not have an associated colour space, so colour matching won't effect it.  The SVG spec uses images as data sources, and specifies that all processing should be done in linear rgb, eg. data is colour matched appropriately.

That said, failing to account for colour profiles in images is a giant leap backwards for graphics processing, i'll bring this up on the list, and for now you can simply target sRGB as i believe that's what the absence of profile information is intended to imply.

I will say however that I believe the problem here is the assumption that texture is synonymous with <image>.  It's not.  Textures are occasionally used to provide data, does not mean that all textures are filled with information from images.  Once we get ES to incorporate the type array stuff, then we can drop the WebGL specific typed-arrays, and Anne has said the moment there's ES provides them he'll update XHR to provide arraybuffer or whatever access.

[Kenneth Russell]

(In reply to comment #21)
> > The WebGL working group has discussed this at length and it is a requirement
> > that the incoming image data be untouched. In 3D algorithms it is common to
> > place non-color data into color and alpha channels of images -- for example,
> > normal maps, or high and low bytes of 16-bit per channel environment maps.
> > Performing color correction, or automatically premultiplying the alpha channel
> > into the color channels, will cause incorrect results.
> 
> If the image is intended to be data then it should not have an associated
> colour space, so colour matching won't effect it.  The SVG spec uses images as
> data sources, and specifies that all processing should be done in linear rgb,
> eg. data is colour matched appropriately.
> 
> That said, failing to account for colour profiles in images is a giant leap
> backwards for graphics processing, i'll bring this up on the list, and for now
> you can simply target sRGB as i believe that's what the absence of profile
> information is intended to imply.
> 
> I will say however that I believe the problem here is the assumption that
> texture is synonymous with <image>.  It's not.  Textures are occasionally used
> to provide data, does not mean that all textures are filled with information
> from images.  Once we get ES to incorporate the type array stuff, then we can
> drop the WebGL specific typed-arrays, and Anne has said the moment there's ES
> provides them he'll update XHR to provide arraybuffer or whatever access.

Can we *please* decouple this discussion from the review of this patch? The code in this patch (in particular the color space handling on the Mac) is identical to the previous code; it is just a refactoring. I am blocked on further texture-related bug fixing until this lands.

[Oliver Hunt]

Comment on attachment 48836 [details]
Revised patch

r=me, but file a bug on the colour matching saga

[WebKit Commit Bot]

Comment on attachment 48836 [details]
Revised patch

Clearing flags on attachment: 48836

Committed r54907: <http://trac.webkit.org/changeset/54907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

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