Bug 51498 - Avoid decoding images twice in texImage2D
Summary: Avoid decoding images twice in texImage2D
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-22 14:53 PST by Zhenyao Mo
Modified: 2011-04-01 15:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.82 KB, patch)
2011-03-30 22:07 PDT, John Bauman
no flags Details | Formatted Diff | Diff
This only helps when the image is opaque or the texture is premultiplied, but that's pretty often. (2.82 KB, patch)
2011-03-30 22:11 PDT, John Bauman
no flags Details | Formatted Diff | Diff
add CG support (6.09 KB, patch)
2011-04-01 11:35 PDT, John Bauman
no flags Details | Formatted Diff | Diff
add CG support (6.14 KB, patch)
2011-04-01 11:43 PDT, John Bauman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2010-12-22 14:53:44 PST
Maybe postpone the decoding of an image until it's accessed?

This needs to be very careful as it affects the HTMLImageElement behavior.
Comment 1 Kenneth Russell 2010-12-22 15:07:21 PST
Could you expand upon the bug description? In the situation where we are supposed to skip the browser's normal color processing (i.e, premultiplication of alpha), then a WebGL-specific decode step is necessary. What could be avoided is the "normal" decode step for the HTMLImageElement, since these images likely won't be displayed on the page; that step could be made lazy.
Comment 2 John Bauman 2011-03-30 22:07:49 PDT
Created attachment 87666 [details]
Patch
Comment 3 John Bauman 2011-03-30 22:11:25 PDT
Created attachment 87667 [details]
This only helps when the image is opaque or the texture is premultiplied, but that's pretty often.
Comment 4 Zhenyao Mo 2011-03-31 06:56:16 PDT
This looks good.  Are you planning to do the similar thing on the Mac/cg port also?
Comment 5 John Bauman 2011-03-31 10:35:32 PDT
Ideally yes, although CGImage doesn't have that nice isOpaque function, so figuring that out might be a bit more difficult (or not, I'm not sure if it would be safe for BitmapImage::frameHasAlphaAtIndex to be public).
Comment 6 Kenneth Russell 2011-03-31 13:48:57 PDT
Comment on attachment 87667 [details]
This only helps when the image is opaque or the texture is premultiplied, but that's pretty often.

This looks good, but could you please either:
 - File another bug about fixing the CG port, or
 - Look further into how to make this work for CG

Another option would be to try to pass down a flag from a higher level about whether alpha is present. In particular, for JPEGs there is definitely no alpha channel.

Also, do you have any measurements of how much faster this makes texture uploads to WebGL when the new code path is taken?
Comment 7 John Bauman 2011-04-01 09:04:17 PDT
It looks like making BitmapImage::frameHasAlphaAtIndex public will work, so I'll just add CG support to this patch.

Times for doing texImage2D from 1000 images go from 5200 ms (jpeg to premultiplied), 4200 ms (jpeg to non-premultiplied), 9300 ms (png to premultiplied) and 8400 ms (png to nonpremultiplied) down to 1900ms, 2400ms, 4000ms, 8400 ms, respectively. On skia we even get a performance gain for pngs that have alpha channels but no transparent pixels, but unfortunately we can't get that gain on CG.
Comment 8 Kenneth Russell 2011-04-01 09:38:57 PDT
(In reply to comment #7)
> It looks like making BitmapImage::frameHasAlphaAtIndex public will work, so I'll just add CG support to this patch.
> 
> Times for doing texImage2D from 1000 images go from 5200 ms (jpeg to premultiplied), 4200 ms (jpeg to non-premultiplied), 9300 ms (png to premultiplied) and 8400 ms (png to nonpremultiplied) down to 1900ms, 2400ms, 4000ms, 8400 ms, respectively. On skia we even get a performance gain for pngs that have alpha channels but no transparent pixels, but unfortunately we can't get that gain on CG.

That's fantastic. Thanks for tracking down this horrible inefficiency.
Comment 9 Zhenyao Mo 2011-04-01 09:58:45 PDT
Great job.  Once the patch lands, could you please open another bug to track the "png to nonpremultiplied" optimization?  We still decode twice on these situations and we don't have to.
Comment 10 Kenneth Russell 2011-04-01 11:27:04 PDT
(In reply to comment #9)
> Great job.  Once the patch lands, could you please open another bug to track the "png to nonpremultiplied" optimization?  We still decode twice on these situations and we don't have to.

The difficulty there will be getting the PNG decoded in the background with the options desired for WebGL. The background decompression premultiplies the alpha channel into the color channels. I don't think we can change that without imposing a severe performance penalty for all web content.
Comment 11 Zhenyao Mo 2011-04-01 11:31:02 PDT
We might be able to make the decoding on demand.  So if an image is never displayed in the browser and it's only used by WebGL as texture source, then we only need to decode it once.

However, I am not sure how this on-demand decoding will effect the web performance at the moment.  Haven't really talked to many people yet, except for James Robinson a while ago, and he thought it's quite possible.
Comment 12 John Bauman 2011-04-01 11:35:57 PDT
Created attachment 87886 [details]
add CG support
Comment 13 WebKit Review Bot 2011-04-01 11:38:51 PDT
Attachment 87886 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 John Bauman 2011-04-01 11:43:38 PDT
Created attachment 87889 [details]
add CG support
Comment 15 Kenneth Russell 2011-04-01 13:37:07 PDT
Comment on attachment 87889 [details]
add CG support

Looks great. Thanks for optimizing the CG path too.
Comment 16 WebKit Commit Bot 2011-04-01 15:26:11 PDT
The commit-queue encountered the following flaky tests while processing attachment 87889 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2011-04-01 15:28:27 PDT
Comment on attachment 87889 [details]
add CG support

Clearing flags on attachment: 87889

Committed r82728: <http://trac.webkit.org/changeset/82728>
Comment 18 WebKit Commit Bot 2011-04-01 15:28:34 PDT
All reviewed patches have been landed.  Closing bug.