Bug 87094

Summary: GeneratorGeneratedImage should cache images for the non-tiled case
Product: WebKit Reporter: Jin Yang <jin.a.yang>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: bdakin, dglazkov, dino, simon.fraser, thorton, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91454    
Bug Blocks: 91453    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
LayoutTest none

Description Jin Yang 2012-05-22 01:17:06 PDT
no pattern draw should also be cached.
Comment 1 Jin Yang 2012-05-22 01:36:08 PDT
Created attachment 143226 [details]
Patch
Comment 2 Tim Horton 2012-05-22 01:50:24 PDT
Comment on attachment 143226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143226&action=review

The reason I didn't immediately do this before was because I wasn't sure what the overhead of the adding the intermediate image buffer would be; have you tested that?

> Source/WebCore/ChangeLog:8
> +        No new tests.Performance optimization. 

Space after the first period please.
Comment 3 Jin Yang 2012-05-22 01:52:10 PDT
Hi Tim,

I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!

Jin
Comment 4 Tim Horton 2012-05-22 01:55:14 PDT
(In reply to comment #3)
> Hi Tim,
> 
> I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
> and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!
> 
> Jin

I don't have review, so I can't help you too much. The change looks fine otherwise.

I'm glad to see it helps a benchmark, that's excellent!
Comment 5 Jin Yang 2012-05-22 02:04:36 PDT
Created attachment 143234 [details]
Patch
Comment 6 Jin Yang 2012-05-22 02:10:37 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Hi Tim,
> > 
> > I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
> > and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!
> > 
> > Jin
> 
> I don't have review, so I can't help you too much. The change looks fine otherwise.
> 
> I'm glad to see it helps a benchmark, that's excellent!

Yes, thanks for your former great work, the Bayou benchmarks boost 50% with litter code change!
Comment 7 WebKit Review Bot 2012-05-22 03:35:13 PDT
Comment on attachment 143234 [details]
Patch

Attachment 143234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12748376

New failing tests:
fast/gradients/css3-linear-angle-gradients.html
fast/gradients/border-image-gradient-sides-and-corners.html
fast/borders/border-image-scaled-gradient.html
fast/gradients/css3-radial-gradients2.html
fast/gradients/css3-linear-right-angle-gradients.html
fast/gradients/css3-repeating-linear-gradients.html
fast/gradients/css3-radial-gradients.html
fast/gradients/css3-repeating-radial-gradients.html
fast/css/transformed-mask.html
Comment 8 WebKit Review Bot 2012-05-22 03:35:18 PDT
Created attachment 143251 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 9 Tim Horton 2012-05-22 08:31:50 PDT
(In reply to comment #7)
> (From update of attachment 143234 [details])
> Attachment 143234 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12748376
> 
> New failing tests:
> fast/gradients/css3-linear-angle-gradients.html
> fast/gradients/border-image-gradient-sides-and-corners.html
> fast/borders/border-image-scaled-gradient.html
> fast/gradients/css3-radial-gradients2.html
> fast/gradients/css3-linear-right-angle-gradients.html
> fast/gradients/css3-repeating-linear-gradients.html
> fast/gradients/css3-radial-gradients.html
> fast/gradients/css3-repeating-radial-gradients.html
> fast/css/transformed-mask.html

Jin: these will need new baselines.
Comment 10 Simon Fraser (smfr) 2012-05-22 08:33:04 PDT
Comment on attachment 143234 [details]
Patch

Patch looks OK but please investigate the EWS failures.
Comment 11 Tim Horton 2012-05-22 09:02:47 PDT
(In reply to comment #10)
> (From update of attachment 143234 [details])
> Patch looks OK but please investigate the EWS failures.

They're all super minor single-digit color deltas.

Conveniently, I think this patch will also fix https://bugs.webkit.org/show_bug.cgi?id=19942 in a somewhat roundabout way.
Comment 12 Jin Yang 2012-05-22 21:56:56 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 143234 [details] [details])
> > Patch looks OK but please investigate the EWS failures.
> 
> They're all super minor single-digit color deltas.
> 
> Conveniently, I think this patch will also fix https://bugs.webkit.org/show_bug.cgi?id=19942 in a somewhat roundabout way.

I have checked the result, just as you said, they are all super minor single-digit color deltas. I think that's because the former approach draws the gradient on the fly and our patch caches the gradient image first and then tiles the image to destination, this way may lose little pixel precision.

I am not familiar with the rebaseline process. Could you suggest the guide to do rebaseline? Thanks
Comment 13 Jin Yang 2012-05-23 20:47:14 PDT
Created attachment 143718 [details]
Patch
Comment 14 Jin Yang 2012-05-24 00:40:04 PDT
Created attachment 143752 [details]
Patch
Comment 15 Jin Yang 2012-05-24 07:56:07 PDT
HiSimon,

I have updated the patch with adding the test expectations. The rebaseline should after the patch is in. Could you please review the patch? Thanks!
Jin
Comment 16 Simon Fraser (smfr) 2012-05-29 10:16:54 PDT
Comment on attachment 143752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143752&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests.Performance optimization.

Missing space. You need some explanation of the change in the changlog here.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:55
> +    // Tile the image buffer to the destination

This is the untiled case, so this comment is inaccurate.
Comment 17 Jin Yang 2012-05-30 06:50:28 PDT
Created attachment 144796 [details]
Patch
Comment 18 Jin Yang 2012-05-30 06:51:46 PDT
updated the patch according to simon's comments
Comment 19 Simon Fraser (smfr) 2012-05-30 09:09:35 PDT
Comment on attachment 144796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144796&action=review

> Source/WebCore/ChangeLog:3
> +        GeneratorGeneratedImage should cache images for no pattern draw

"for no pattern draw" is confusing here. It should be "for the non-tiled drawing case" or something.

> Source/WebCore/ChangeLog:9
> +        By compared to generate image on the fill, it will loss litter pixel precision

I'm not sure what "loss litter" means. Do you mean "lose a little"?

You should mention the performance gains.
Comment 20 Jin Yang 2012-05-31 02:03:16 PDT
Created attachment 145020 [details]
Patch
Comment 21 Jin Yang 2012-05-31 02:27:56 PDT
(In reply to comment #19)
> (From update of attachment 144796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144796&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        GeneratorGeneratedImage should cache images for no pattern draw
> 
> "for no pattern draw" is confusing here. It should be "for the non-tiled drawing case" or something.
> 
> > Source/WebCore/ChangeLog:9
> > +        By compared to generate image on the fill, it will loss litter pixel precision
> 
> I'm not sure what "loss litter" means. Do you mean "lose a little"?
> 
> You should mention the performance gains.

Thanks for the comments. Corrected the typo and description.
Comment 22 Simon Fraser (smfr) 2012-05-31 08:35:51 PDT
Comment on attachment 145020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145020&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests needed. We cache the generated image if generator is not changed.

The "No new tests needed" should be on a separate line after the description of the change.

> Source/WebCore/ChangeLog:10
> +        and several layout tests should be rebaselined.With this patch, the IE test drive

Space after .
Comment 23 Jin Yang 2012-06-01 02:06:44 PDT
Created attachment 145245 [details]
Patch
Comment 24 Jin Yang 2012-06-01 02:09:58 PDT
(In reply to comment #22)
> (From update of attachment 145020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145020&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests needed. We cache the generated image if generator is not changed.
> 
> The "No new tests needed" should be on a separate line after the description of the change.
> 
> > Source/WebCore/ChangeLog:10
> > +        and several layout tests should be rebaselined.With this patch, the IE test drive
> 
> Space after .

updated the patch.
Comment 25 Simon Fraser (smfr) 2012-06-01 08:25:36 PDT
Comment on attachment 145245 [details]
Patch

Note that related code is being cleaned up in bug 88063.
Comment 26 Jin Yang 2012-06-06 02:52:30 PDT
Created attachment 145970 [details]
Patch
Comment 27 Jin Yang 2012-06-06 02:53:27 PDT
(In reply to comment #26)
> Created an attachment (id=145970) [details]
> Patch

uploaded the patch, and also did clean up.
Comment 28 Jin Yang 2012-06-06 19:07:11 PDT
Created attachment 146177 [details]
Patch
Comment 29 Jin Yang 2012-06-06 19:09:05 PDT
seems test_expectation need to rebase, so uploaded the rebased patch.
Comment 30 WebKit Review Bot 2012-06-07 05:58:41 PDT
Comment on attachment 146177 [details]
Patch

Attachment 146177 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12911249

New failing tests:
fast/reflections/reflection-with-zoom.html
Comment 31 WebKit Review Bot 2012-06-07 05:58:56 PDT
Created attachment 146268 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 32 Jin Yang 2012-06-07 23:41:26 PDT
Created attachment 146490 [details]
Patch
Comment 33 Jin Yang 2012-06-11 05:41:48 PDT
(In reply to comment #32)
> Created an attachment (id=146490) [details]
> Patch

Hi Simon,

The updated patch is ready, please help review it. Thanks
Comment 34 Jin Yang 2012-06-11 18:18:48 PDT
Hi Simon,

It seems the patch was not committed. I added the commit-queue? flag.

Jin
Comment 35 Simon Fraser (smfr) 2012-06-11 18:31:52 PDT
Comment on attachment 146490 [details]
Patch

You should set cq? when you request review.
Comment 36 WebKit Review Bot 2012-06-11 20:04:57 PDT
Comment on attachment 146490 [details]
Patch

Clearing flags on attachment: 146490

Committed r120033: <http://trac.webkit.org/changeset/120033>
Comment 37 WebKit Review Bot 2012-06-11 20:05:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 WebKit Review Bot 2012-07-16 17:37:54 PDT
Re-opened since this is blocked by 91454
Comment 39 Dana Jansens 2012-07-16 17:39:39 PDT
Created attachment 152656 [details]
LayoutTest

here's a LayoutTest to demonstrate the CSS background gradient breakage.